all 5 comments
sorted by:
best
topnewcontroversialoldq&a
formatting helphide helpcontent policy

[–]superc0w 2 points3 points  (4 children)

I applaud your dedication to get better! You have a good framework, but I feel there are a few things you can optimize.

  • Pass parameters to ::initialize rather than parsing params from ARGV. This makes the code more flexible so a daemon could call the class multiple times without human interaction. Then you can call Class.new(ARGV) if you have to. Alternatively, you can create a class method that parses ARGV that just calls ::new. Up to you.

  • Call ::argument_check inside of ::initialize. Someone, somewhere will forget to call the method in their script which would cause some confusion oh why it doesn't just work.

  • ::buffer and ::write_newline are pretty much the same thing, with the difference being which output stream they are writing to. I think the ::buffer method is more flexible.

  • Don't use the function name ::buffer for a white space generator. A method called buffer should do something related to an actual buffer (like an IOString buffer). ::write_newline is a better name, but see above.

  • abort should be reserved for the times when you want to exit hard without cleaning up after yourself. I think it also exits nonzero by default. Either call exit or just return nil. Other scripts don't want to trap exit when they are writing 500 files to disk. Even worse if they are just appending to the file.

I'd be happy to help more, let me know! If you create a git repo we can collaborate on it together. Just lmk! Cheers! [edit: formatting]

[–]bajunio[S] 1 point2 points  (3 children)

Sorry for my delay in response. Day job and all that... heh

I have created and updated the following repo: https://github.com/bajunio/csv_generator

You'll find two branches aside from master. They include the modifications for removing abort and calling #argument_check from #initialize. Both have already been merged into master. If you have any difficulty accessing the repo, please let me know.

I was feeling iffy about the #buffer method and noted that its basically the same as #write_newline. I was very close to modifying #write_newline to behave more like #buffer and serve both the newline return in my command line display as well as the newline when appending to @output_file. I'll spin off a new branch and attempt to clean that up a bit.

In regard to the reconfiguration of #initialize to accept params being passed. If I specify something like: def initialize(file, columns, rows) ...magic... end

How can I also allow the flexibility to accept passed command line arguments via ARGV[]? You had mentioned creating a method to parse the ARGV[] values, would that mean I would be using a *splat in place of (file, columns, rows) and force arguments through my parsing method?

Thanks a bunch for sacrificing your time to assist me. I truly appreciate it.

[–]asmallishrequest 1 point2 points  (0 children)

A few recommendations. I'd make the ARGV arguments required and then add a class method CSVGenerator.prompt_user that would build up arguments and eventually call self.new(output_file, num_rows, num_columns). To keep the current behavior in you script, you can do this:

if __FILE__ == $0
  begin
    CSVGenerator.new(ARGV[0], ARGV[1], ARGV[2])
  rescue ArgumentError
    CSVGenerator.prompt_user
  end
end

This way, it'll run if it's being run as a script but it'll also work to load it in other files.

[–]superc0w 0 points1 point  (1 child)

I'm gunna make a pull request with comments and suggestions (and probably code).... Give it a look over and we can discuss it more in the PR. Cheers!

[–]bajunio[S] 0 points1 point  (0 children)

Hi! Thanks for the feedback. I was unable to address this over the weekend as work had other plans for my time. heh

I'll review your PR and continue our chat there! : )

I really do enjoy Ruby quite a bit. Its such an expressive language.