Skip to main content
Code Review

Return to Answer

The first thing I noticed is that you have grouped all the type signatures together, away from the function bodies. This seems very strange to me. It's way more common (and as far as I'm concerned also way more readable) to have a value's type directly before its definition.

The second thing I noticed were all the braces and semicolons. Unless you have a specific reason to use them, I'd strongly recommend making use of Haskell's significant indentation instead. Much less visual noise. I'm especially confused by the semicolon at the end of parseName's definition as parseName doesn't use do-notation and you didn't use a semicolon on any other definition which didn't use do.


You sometimes surround variables with parentheses (e.g. in values <- endBy1 (parseNameValue) (parseCmtOrSpace), but not in the line name <- between parseCmtOrSpace ... directly above it). I can't see any system to when you do this and when you don't, so it just seems random and inconsistent. So I'd recommend to get rid of the needless parentheses.

Speaking of parentheses, you might also want to get rid of some more parentheses by sprinkling a couple of $ signs in your code, to give it a more "haskellish" feel.


parseSectionHeader = do{
 name <- between (char '[') (char ']') parseName;
 return name;
}
parseIniFile = do{
 result <- many1 (parseSection);
 return result;
}

Whenever you have something of the form:

foo <- bar
return foo

You can simply just write foobar. I.e.:

parseSectionHeader = between (char '[') (char ']') parseName
parseIniFile = many1 parseSection

iniFileToMap path = do{
 result <- parseFromFile parseIniFile path;
 case (result) of
 Left err -> error (show err)
 Right xs -> return (list2Map(xs));
}

Here the indentation is off - case should not be indented further than result <-. It should be:

iniFileToMap path = do
 result <- parseFromFile parseIniFile path
 case result of
 Left err -> error $ show err
 Right xs -> return $ list2Map xs

The first thing I noticed is that you have grouped all the type signatures together, away from the function bodies. This seems very strange to me. It's way more common (and as far as I'm concerned also way more readable) to have a value's type directly before its definition.

The second thing I noticed were all the braces and semicolons. Unless you have a specific reason to use them, I'd strongly recommend making use of Haskell's significant indentation instead. Much less visual noise. I'm especially confused by the semicolon at the end of parseName's definition as parseName doesn't use do-notation and you didn't use a semicolon on any other definition which didn't use do.


You sometimes surround variables with parentheses (e.g. in values <- endBy1 (parseNameValue) (parseCmtOrSpace), but not in the line name <- between parseCmtOrSpace ... directly above it). I can't see any system to when you do this and when you don't, so it just seems random and inconsistent. So I'd recommend to get rid of the needless parentheses.

Speaking of parentheses, you might also want to get rid of some more parentheses by sprinkling a couple of $ signs in your code, to give it a more "haskellish" feel.


parseSectionHeader = do{
 name <- between (char '[') (char ']') parseName;
 return name;
}
parseIniFile = do{
 result <- many1 (parseSection);
 return result;
}

Whenever you have something of the form:

foo <- bar
return foo

You can simply just write foo. I.e.:

parseSectionHeader = between (char '[') (char ']') parseName
parseIniFile = many1 parseSection

iniFileToMap path = do{
 result <- parseFromFile parseIniFile path;
 case (result) of
 Left err -> error (show err)
 Right xs -> return (list2Map(xs));
}

Here the indentation is off - case should not be indented further than result <-. It should be:

iniFileToMap path = do
 result <- parseFromFile parseIniFile path
 case result of
 Left err -> error $ show err
 Right xs -> return $ list2Map xs

The first thing I noticed is that you have grouped all the type signatures together, away from the function bodies. This seems very strange to me. It's way more common (and as far as I'm concerned also way more readable) to have a value's type directly before its definition.

The second thing I noticed were all the braces and semicolons. Unless you have a specific reason to use them, I'd strongly recommend making use of Haskell's significant indentation instead. Much less visual noise. I'm especially confused by the semicolon at the end of parseName's definition as parseName doesn't use do-notation and you didn't use a semicolon on any other definition which didn't use do.


You sometimes surround variables with parentheses (e.g. in values <- endBy1 (parseNameValue) (parseCmtOrSpace), but not in the line name <- between parseCmtOrSpace ... directly above it). I can't see any system to when you do this and when you don't, so it just seems random and inconsistent. So I'd recommend to get rid of the needless parentheses.

Speaking of parentheses, you might also want to get rid of some more parentheses by sprinkling a couple of $ signs in your code, to give it a more "haskellish" feel.


parseSectionHeader = do{
 name <- between (char '[') (char ']') parseName;
 return name;
}
parseIniFile = do{
 result <- many1 (parseSection);
 return result;
}

Whenever you have something of the form:

foo <- bar
return foo

You can simply just write bar. I.e.:

parseSectionHeader = between (char '[') (char ']') parseName
parseIniFile = many1 parseSection

iniFileToMap path = do{
 result <- parseFromFile parseIniFile path;
 case (result) of
 Left err -> error (show err)
 Right xs -> return (list2Map(xs));
}

Here the indentation is off - case should not be indented further than result <-. It should be:

iniFileToMap path = do
 result <- parseFromFile parseIniFile path
 case result of
 Left err -> error $ show err
 Right xs -> return $ list2Map xs
Source Link
sepp2k
  • 9k
  • 2
  • 39
  • 51

The first thing I noticed is that you have grouped all the type signatures together, away from the function bodies. This seems very strange to me. It's way more common (and as far as I'm concerned also way more readable) to have a value's type directly before its definition.

The second thing I noticed were all the braces and semicolons. Unless you have a specific reason to use them, I'd strongly recommend making use of Haskell's significant indentation instead. Much less visual noise. I'm especially confused by the semicolon at the end of parseName's definition as parseName doesn't use do-notation and you didn't use a semicolon on any other definition which didn't use do.


You sometimes surround variables with parentheses (e.g. in values <- endBy1 (parseNameValue) (parseCmtOrSpace), but not in the line name <- between parseCmtOrSpace ... directly above it). I can't see any system to when you do this and when you don't, so it just seems random and inconsistent. So I'd recommend to get rid of the needless parentheses.

Speaking of parentheses, you might also want to get rid of some more parentheses by sprinkling a couple of $ signs in your code, to give it a more "haskellish" feel.


parseSectionHeader = do{
 name <- between (char '[') (char ']') parseName;
 return name;
}
parseIniFile = do{
 result <- many1 (parseSection);
 return result;
}

Whenever you have something of the form:

foo <- bar
return foo

You can simply just write foo. I.e.:

parseSectionHeader = between (char '[') (char ']') parseName
parseIniFile = many1 parseSection

iniFileToMap path = do{
 result <- parseFromFile parseIniFile path;
 case (result) of
 Left err -> error (show err)
 Right xs -> return (list2Map(xs));
}

Here the indentation is off - case should not be indented further than result <-. It should be:

iniFileToMap path = do
 result <- parseFromFile parseIniFile path
 case result of
 Left err -> error $ show err
 Right xs -> return $ list2Map xs
lang-hs

AltStyle によって変換されたページ (->オリジナル) /