CODEWITHSUNDEEP
haskellcoding-style
Werner Echezuria
Werner Echezuria

Reputation: 622

Improve a haskell script

I'm a newbie in Haskell and I'd like some opinions about improving this script. This is a code generator and requires a command line argument to generate the sql script.

./GenCode "people name:string age:integer"

Code:

import Data.List
import System.Environment (getArgs)

create_table :: String -> String

create_table str =  "CREATE TABLE " ++ h (words str)
        where h (x:xs) = let cab = x
                             final = xs
                         in x ++ "( " ++ create_fields xs ++ ")"

create_fields (x:xs) = takeWhile (/=':') x ++ type x ++ sig
              where sig | length xs > 0 = "," ++ create_fields xs
                        | otherwise     = " " ++ create_fields xs
create_fields []     = ""

type x | isInfixOf "string"  x = " CHARACTER VARYING"
       | isInfixOf "integer" x = " INTEGER"
       | isInfixOf "date"    x = " DATE"
       | isInfixOf "serial"  x = " SERIAL"
       | otherwise             = ""

main = mainWith 
  where mainWith = do
      args <- getArgs
  case args of
       [] -> putStrLn $ "You need one argument"
       (x:xs) -> putStrLn $ (create_table x)

Upvotes: 1

Views: 291

Answers (2)

Nefrubyr
Nefrubyr

Reputation: 7084

Don't use length xs > 0 (in sig); it needlessly counts the length of xs when all you really wanted to know is whether it's empty. Use null xs to check for a non-empty list:

...
where sig | null xs   = ... -- Empty case
          | otherwise = ... -- Non-empty case

or add an argument to sig and pattern match:

...
where sig (y:ys) = ...
      sig []     = ...

Although Nathan Sanders' advice to replace the whole recursive thing with intercalate is excellent and makes this a moot point.

You're also identifying the type by passing the whole "var:type" string into type, so it is testing

"string" `isInfixOf` "name:string"

etc.

You could use break or span instead of takeWhile to separate the name and type earlier:

create_fields (x:xs) = xname ++ type xtype ++ sig
    where
      (xname, _:xtype) = break (==':') x
      sig = ...

and then type can compare for string equality, or look up values using a Map.

A quick explanation of that use of break:

break (==':') "name:string" == ("name", ":string")

Then when binding

(xname, _:xtype) to ("name", ":string"),

 xname -> "name"
 _     -> ':'      (discarded)
 xtype -> "string"

Upvotes: 5

Nathan Shively-Sanders
Nathan Shively-Sanders

Reputation: 18389

I think you understand how to write functional code already. Here are some small style notes:

  • Haskell usually uses camelCase, not under_score_separation
  • In create_table, cabo and final are not used.
  • Usually a list-recursive function like create_fields puts the empty list case first.
  • I would not make create_fields recursive anyway. The comma-joining code is quite complicated and should be separated from the typing code. Instead do something like Data.List.intercalate "," (map create_field xs). Then create_field x can just be takeWhile (/=':') x ++ type x
  • Especially if there are a lot of types to be translated, you might put them into a map

Like so:

types = Data.Map.fromList [("string", "CHARACTER VARYING")
                          ,("integer", "INTEGER")
                          -- etc
                          ]

Then type can be Data.Maybe.fromMaybe "" (Data.Map.lookup x types)

  • Code can appear in any order, so it's nice to have main up front. (This is personal preference though)
  • You don't need mainWith.

Just say

main = do
  args <- getArgs
  case args of
    [] -> ...
  • You don't need the dollar for the calls to putStrLn. In the first call, the argument wouldn't require parentheses anyway, and in the second, you supply the parentheses. Alternatively, you could keep the second dollar and drop the parentheses.

Upvotes: 6

Related Questions