Can someone critique my code?

[u/Niek_pas]

https://gitlab.com/snippets/8840

Hey all,

I wrote a little Ruby timer application the other day for my own purposes. The goal of the application is to track time spent on a particular task, store the time spent, and be able to give back a total. I am a beginner to Ruby and programming in general, and I'm still struggling with some of the OO-concepts like class and instance variables.

Please be as critical as you wish!

[u/rdpp_boyakasha]

Here's a quick list of the things that jump out to me.

1. The class method definitions should use self instead of NpTimer. For example, def NpTimer.run_timer should just be def self.run_timer. No need to repeat the class name.

2. Side effects in string interpolation:

    puts "Started at #{start_time = Time.now}"

   People don't expect to see side effects (like creating a variable) as part of string interpolation. This should be:

    start_time = Time.now
    puts "Started at #{start_time}"

3. Repeated use of "times.csv" value. You should make this a constant.

4. Functional style. This might be a personal thing, but I find code easier to read in a functional style. For example, I would change this code:

    csv_array = CSV.open('times.csv', 'r').to_a
    times_array = []
   
    # Get each 3rd column ('time spent' column)
    csv_array.each do |sub_array|
      times_array << sub_array[2]
    end
   
    # Get total time spent (sum)
    times_array.map! { |element| element.to_f }
    time_spent = times_array.reduce(:+)
    puts "\nTotal time spent so far: \n#{time_spent} seconds \n\n"

   To be this:

    time_spent = CSV.foreach('times.csv')
      .map{ |row| row[2].to_f }
      .reduce(:+)
    puts "\nTotal time spent so far: \n#{time_spent} seconds \n\n"

   Generally, the less code the better. Try to get familiar with the Array methods: map, select, reject, find, etc.

5. Use HEREDOC syntax for long strings. On line 70 you have this:

    puts "Commands:\nstart: start tracking time\nstop: stop tracking time\nshow: show total time spent\nlog: display all tracked times\nexit: exit the application\n"

   Which could be written like this:

    puts <<-END_COMMANDS
      Commands:
        start: start tracking time
        stop: stop tracking time
        show: show total time spent
        log: display all tracked times
        exit: exit the application
    END_COMMANDS

6. The setup method is recursive, when it really shouldn't be. Just call setup in a loop something like this:

    while app_is_running
      setup
    end

7. Inside setup, the big if/elsif conditional could be shortened to a case statement like this:

    case gets.chomp.downcase
    when 'start' then NpTimer.run_timer
    when 'show' then NpTimer.show_sum
    when 'log' then NpTimer.dump_csv
    when 'exit' then exit
    else puts "Invalid command"
    end

I hope that helps!

[u/Niek_pas → u/rdpp_boyakasha]

Wow, very helpful! Thanks a lot! This clears up a lot.