4
\$\begingroup\$

I'm reading a CSV using Haskell. I'm not sure if this is the appropriate way to do it.

This is what I'm doing:

  1. Read rows from a CSV -> return lazy byte string
  2. Parse the headers and rows from the CSV to a tuple -> (headers, [Stock])
  3. Remove the headers -> [Stock]
  4. Filter the stocks that are "Common Stock" -> [Stock]
  5. Print the resulting stocks

Any feedback on how to write better Haskell code is appreciated!

The code is a Stack project, you can find the project and instructions on how to run it: here

I read this section of Stephen Diehl's guide before writing the code: What I wish I knew when learning Haskell

Here is the code to read the CSV file. The main function is printStocks.

{-# LANGUAGE OverloadedStrings #-}

module Lib (printStocks) where

import Control.Monad
import qualified Data.ByteString.Lazy as BL
import Data.Csv
import qualified Data.Vector as V

-- data type to model a stock
data Stock = Stock
  { code :: String,
    name :: String,
    country :: String,
    exchange :: String,
    currency :: String,
    instrumentType :: String
  }
  deriving (Show)

instance FromNamedRecord Stock where
  parseNamedRecord record =
    Stock
      <$> record .: "Code"
      <*> record .: "Name"
      <*> record .: "Country"
      <*> record .: "Exchange"
      <*> record .: "Currency"
      <*> record .: "Type"

-- type synonyms to handle the CSV contents
type ErrorMsg = String

type CsvData = (Header, V.Vector Stock)

-- Function to read the CSV
parseCSV :: FilePath -> IO (Either ErrorMsg CsvData)
parseCSV filePath = do
  contents <- BL.readFile filePath
  return $ decodeByName contents

-- Discard headers from CsvData
removeHeaders :: CsvData -> V.Vector Stock
removeHeaders = snd

-- Check if the given element is a Common Stock
isStock :: Stock -> Bool
isStock stock = instrumentType stock == "Common Stock"

filterStocks :: V.Vector Stock -> V.Vector Stock
filterStocks = V.filter isStock

-- Print the stocks from the CSV file
printStocks :: FilePath -> IO ()
printStocks filePath =
  parseCSV filePath
    >>= print . fmap (filterStocks . removeHeaders)
\$\endgroup\$
2
\$\begingroup\$

Looks great overall, especially how you've included instructions on running it, even with sample data, makes this a great submission.


The first thing I've noticed, is the error handling for opening files. parseCsv for example checks for the existence of the file - that's a big hint that the function doesn't work in all circumstances as expected, like if the file exists, but isn't readable (try chmod a-r test-resources/empty-file.csv and see how it results in an uncaught exception *** Exception: test-resources/empty-file.csv: openBinaryFile: permission denied (Permission denied)). From the signature I'd actually expect this to be handled via the Either:

parseCsv filePath = do
  result <- try $ BL.readFile filePath
  return $ case result of
             Left (exception :: IOException) -> Left $ show exception
             Right contents -> decodeByName contents

I'm sure that could be done nicer, this one would also need ScopedTypeVariables enabled.


IMO Csv looks odd, but the package is already using the name, I guess that's fine.

Some of the comments could be better, like parseCsv saying "Function to read the CSV" - well, yes, we can see that from the name already. Maybe "Read raw CSV data from a file", similar to readStocks.


I'd probably inline the local function in filterStocks, because I don't think it adds much clarity over an anonymous function:

filterStocks = V.filter (\instrument -> instrumentType instrument == "Common Stock")

Since you're already using fmap liberally I also thought about the following:

filterStocks = V.filter $ fmap (== "Common Stock") instrumentType

However, I actually think it makes it worse in terms of readability.

Similarly, readStocks with its fmap . fmap is a bit too complicated for me to follow. I'd argue that expanding it a bit might be better for understanding for the next reader.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.