[Haskell-beginners] Feedback on my first, small project (piece).

[Mateusz Kowalczyk]

On 08/06/2014 12:46 AM, Henk-Jan van Tuyl wrote:
> On Tue, 05 Aug 2014 22:08:51 +0200, Dominik Bollmann  
> <dominikbollmann at gmail.com> wrote:
> 
> :
>> The code is available on github: https://github.com/bollmann/Globber
>> When writing the program I tried to satisfy the specification as given
>> at: http://www.scs.stanford.edu/14sp-cs240h/labs/lab1.html. This Lab
>> material btw. also inspired me to try writing such a program :-).
>>
>> In order to improve my programming in Haskell, I would love to hear
>> feedback from you guys on this first small project of mine. Any comments
>> regarding style, used idioms, as well as general and specific code
>> improvements are highly appreciated. Thanks!
> :
> 
> You could change
> matchGlob' :: GlobPattern -> String -> Bool
> matchGlob' glob string
>    | null glob && null string = True
>    | null glob && not (null string) = False
>    | null string && glob == "*" = True
>    | null string && glob /= "*" = False
> matchGlob' (p:ps) (s:sx)
>    | p == '?' = matchGlob' ps sx
>    | p == '\\' = matchEscapedChar ps (s:sx)
>    | p == '*' = matchStar ps (s:sx)
>    | p == '[' = matchSet (p:ps) (s:sx)
>    | otherwise = p == s && matchGlob' ps sx
> 
> 
> to:
> matchGlob' :: GlobPattern -> String -> Bool
> matchGlob' []       string = null string
> matchGlob' glob      []    = glob == "*"
> matchGlob' ('?':ps) (s:sx) = matchGlob'       ps sx
> matchGlob' ('\\':ps) sx    = matchEscapedChar ps sx
> matchGlob' ('*':ps)  sx    = matchStar        ps sx
> matchGlob' ('[':ps)  sx    = matchSet   ('[':ps) sx
> matchGlob' (p:ps)   (s:sx) = p == s && matchGlob' ps sx
> 
> (not tested)
> 
> In function buildCharChoices, you have written
>    where isRange chars = if (length chars) == 3 && (chars !! 1) == '-' then
>                          True
>                        else
>                          False
> , this can be simplified to
>    where isRange chars = length chars == 3 && chars !! 1 == '-'
> 
> 
> Regards,
> Henk-Jan van Tuyl
> 
> 

isRange could also be written as
isRange [_, '-', _] = True
isRange _ = False

which I think makes it more obvious what you're looking for

-- 
Mateusz K.