current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite
1

I'm looking to get some feedback on this program I wrote - ways I could make it simpler or best practices methods.

Chapter 7:

Let's write a program which asks us to type in as many words as we want (one word per line, continuing until we just press Enter on an empty line), and which then repeats the words back to us in alphabetical order. OK?

...

Assignment: Write the program we talked about at the very beginning of this chapter. Hint: There's a lovely array method which will give you a sorted version of an array: sort. Use it!

Here's the program I wrote:

puts "This program will take your words and sort them alphabetically"
puts ""
puts "Type in a word and press enter. When you are done, press enter on an empty line to launch program."

inputArray = []
while (inputWord = gets.chomp) != ""
    inputArray.push inputWord
    puts "Current List is: " + inputArray.join(', ')
end
print "Your list in alphabetical order is: " + inputArray.sort.join(', ') + "."
share|improve this question
2  
Ruby prefers snake_case over camelCase for variable names. That's all I see. Is the program's I/O dictated by the requirement, or can it be changed? I ask because the program could be made simpler if, for example, EOF could signal the end of input rather than a blank line. –  Wayne Conrad May 12 at 5:14
add comment

2 Answers

up vote 3 down vote accepted

  1. As Wayne Conrad pointed out in the comments, you should really use snake_case in Ruby. I'd also just call the array words instead of input_array. While input_array describes what sort of variable it is, it doesn't describe its content, or hint at its purpose.

  2. The Ruby convention is 2 spaces of indentation. Not a tab, not 4 spaces.

  3. Use a heredoc for the intro text:

    puts <<END_INFO
This program will take your words and sort them alphabetically

Type in a word and press enter. When you are done, press enter on an empty line to launch program.
END_INFO

    While we're on the subject of the info text: The second line doesn't make much sense. The program is already running when you see that message; there's nothing to "launch".

  4. use String#empty? and until instead of "manually" comparing against an empty string

  5. Use the shovel operator << instead of push. They do the same thing (except push can take several arguments), but the shovel operator is the more common way of doing things.

  6. Why use print in the last line? Use puts instead, so you'll get a newline after your program exits.

  7. Use string interpolation ("#{...}") for simple stuff

I end up with this

puts <<INFO
This program will take your words and sort them alphabetically
Type in a word and press enter. When you are done, press enter on an empty line to launch program.
INFO

words = []

until (word = gets.chomp).empty?
  words << word
  puts "Current list is: #{words.join(", ")}."
end

puts "Your list in alphabetical order is: #{words.sort.join(", ")}."

You can even reduce it down to:

[].tap do |words|
  until (word = gets.chomp).empty?
    words << word
    puts "Current List is: #{words.join(", ")}."
  end
end.tap do |words|
  puts "Your list in alphabetical order is: #{words.sort.join(", ")}."
end

Or even this (which still completes the stated task):

[].tap do |words|
  until (word = gets.chomp).empty?
    words << word
  end
end.sort.each { |word| puts word }
share|improve this answer
    
Thanks Flambino! A ton of good points that I hadn't seen before. I'm fairly new to ruby so I don't know all the best practices yet. 1. The snake case is a good call, I've had some experience in java so I was very used to camel case. 2. Sublime Text creates the tab indentation for me but is it fairly standard to have only 2 indents? 3. Yes! I've seen << been used over push so thanks for confirming that it's standard. 4.Haven't really touched on string interpolation yet, i'll have to look into that, but it looks extremely useful. Thanks again! –  Jimmy May 12 at 22:15
    
@Jimmy The 2-spaces indentation is the Ruby standard, really (see the Ruby docs, for instance, or something like this style guide). You can configure Sublime's (and any other proper editor) indentation any way you want. In Sublime's case, there's a menu in the bottom of the window, as I recall, which'll let you set the width and type of indentation it should use. –  Flambino May 13 at 8:21
add comment
up vote 3 down vote

Flambino has already pointed out the issues in your code, so I'll just show how to tackle the problem with a different approach. A declarative and functional code (as opposed to imperative) favours immutability and known abstractions over manual control flow and in-place updates. Uusing lazy from Ruby 2 and String#present? from active_support (so we can write the more declarative code we can) it would look like this:

require 'active_support/core_ext/string'
sorted_words = $stdin.lines.lazy.map(&:strip).take_while(&:present?).sort
puts("Your list in alphabetical order is: #{sorted_words.join(', ')}.")
share|improve this answer
1  
I always forget about #take_while. Very nice! –  Wayne Conrad May 12 at 17:45
    
@WayneConrad Ditto :) –  Flambino May 13 at 8:08
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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