Return to Revisions

Bike shedding

to get the bike shedding out of the way: i prefer tibbe's style guide. in that spirit, have blank lines between instance declarations, use a newline after an instance where. and please stick to 80 char lines.

you might like to order your functions somehow. the order is imo pretty chaotic.

to the more substantial review.

General advice

Compile with -Wall (in cabal: ghc-options: -Wall), so that ghc tells you many things that are potentially bad.

There is also hlint, but that did not yield anything substantial here.

Avoid partial functions

Listen to advices regarding non-exhaustive pattern matches!

In particular don't use partial record accessors:

data LispVal = ...
             | Func { params  :: [String]
                    , vararg  :: Maybe String
                    , body    :: [LispVal]
                    , closure :: Env 
                    }

Here params :: LispVal -> [String] will fail for ever LispVal that is not a function. Better use a separate type and let the sum type simple. e.g.:

data LispVal = ...
             | Func LispFunc

data LispFunc = LispFunc { params :: ... }

Use meaningful names

Parser is a constructor for LispError. Better use ParseError. Especially because there is a type Parser (from parsec) as well.

Show and Read instances should be reasonable

They should be inverses to each other and should read like what you write in ghci. For your use case, better not use a Show instance at all but rename showVal to prettyLispVal or similar.

Get rid of meaningless type synonyms (ThrowError, IOThrowError)

They obscure what's going on. see e.g. readOrThrow

readOrThrow parser input = case parse parser "lisp" input of          
    Left err  -> throwError $ Parser err
    Right val -> return val

is the same as

readOrThrow parser input = left Parser $ parse parser "lisp"

(with left :: (a -> b) -> Either a c -> Either b c from Control.Arrow, but easily defined by itself).

Use Applicative style when parsing

e.g. parseVector reads so much better with

parseVector = listArr <$> (string "#(" *> sepBy parseExpr spaces1 <* char ')')
  where
    listArr l = Vector $ listArray (0, length l - 1) l

Avoid shadow bindings

e.g. in makeFunc

Supply type signatures for all toplevel functions

Use pure values and functions whenever possible

Is there a reason you use an IORef [(String, IORef LispVal)] instead of simply [(String, LispVal)] (or better a HashMap from unordered-containers)?

Don't unneccessarily use monadic functions. e.g. f = return $ ....

Try to reject invalid scheme programs before interpreting them

you can have a second pass over your AST after you parsed it to weed out invalid terms or enrich the AST to include conditionals, etc. that way, your eval function won't have to deal with too many things at once.

you might also like to look into advanced concepts like HOAS.