search

zexa / interview-task

A comission calculator
0 stars 0 forks source link

CSV parser doesn't have forced format requirement #3

Open Dragas opened 5 years ago

Dragas commented 5 years ago

CSVParser seems to be initialized with CSVParser#format field. Personally I would move this out to a constructor instead.

zexa commented 5 years ago

Hello @Dragas ,

The idea for CSVParser#format is that it's an optional method that can be cast before CSVParser#parse or CSVParser#parseByLine.

CSVParser is designed as a reusable class, which we don't have to reinitialize in order to parse new files. Instead, we would simply pass new files to the CSVParser#setFile method.

The setFile method also provides useful functions such as returning false if the file is not found.

Since the format method should be a per-file variable called after setFile, I believe that it would be harmful to introduce it to the constructor as it would introduce side-effects or sacrifice useful debugging information.

I do agree that it would be more comfortable to have a method that would abstract format and setFile and perhaps even parse or parseByLine into one, but I'm not quite sure what structure I'd like to have quite yet.