https://gist.github.com/tonymorris/a0e82e603d4d32597bbc/revisions On 06/08/14 06:42, David McBride wrote:

I took a look at your main and made some changes that might help. I have not looked at your glob file at all.

1. Rather than using length args successively in multiple if/else if statements, which recalculates the value, just use a case, calculate it once and then go from there, when you can.

2. Use let or where to make new function rather than putting them all inline.

3. Instead of looping the main entirely to get all of stdin, use said new function and have it loop itself. You could also streamline this to have its own function that does not require the glob to be passed into each loop.

4. Use withFile instead of opening the file and then forgetting about the handle. If you had enough files on the commandline, you would exhaust the allowable open file handles on the system. Withfile will close the handle when you are done with each one.

5. Use mapM_ to get rid of the results that are returned by mapM. It is also faster if you are not going to use the results anyways.

6. When you are in a monad (such as IO), you can use when instead of if to optionally perform a statement, and it looks a bit prettier.

There are other minor things, but this is a pretty good start.

Here's the resulting code (I have not run it, but it should be pretty close to what you had): https://gist.github.com/anonymous/615e48004ca2eed82d0a

On Tue, Aug 5, 2014 at 4:08 PM, Dominik Bollmann mailto:dominikbollmann@gmail.com> wrote:

Hello Haskeller's,

I recently dived into Haskell and then wanted to practice it a bit! Therefore I wrote a small program that matches UNIX-style globs. The program behaves kind of like grep, just that it matches a glob and not a regular expression. And also, it offers only *very* rudimental functionality compared to grep.

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!

Cheers, Dominik. _______________________________________________ Beginners mailing list Beginners@haskell.org mailto:Beginners@haskell.org http://www.haskell.org/mailman/listinfo/beginners

_______________________________________________ Beginners mailing list Beginners@haskell.org http://www.haskell.org/mailman/listinfo/beginners

On 08/06/2014 12:46 AM, Henk-Jan van Tuyl wrote:

On Tue, 05 Aug 2014 22:08:51 +0200, Dominik Bollmann 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.