1
\$\begingroup\$

I am quite new to Ruby, coming from a JavaScript background. I wrote this simple Ruby script that finds whether two strings are anagrams or not.

To run it: ruby ruby_anagrams.rb 'script1' 'script2'

I am looking for comments and a code review on my script. Can I do anything better? Is this good Rubyism?

# removing whitespace (in case of multi-word anagrams), converting to lowercase and getting
# array representations:
string_1_arr = ARGV[0].gsub(/\s+/, "").downcase.split("")
string_2_arr = ARGV[1].gsub(/\s+/, "").downcase.split("")

if string_1_arr.size != string_2_arr.size
  puts "Not anagrams"
  exit
end

string_1_arr.each do |c|
  print c, " ",string_2_arr, "\n" # debug statement, but left here 'cuz the output looks cool

  # just delete would delete all occurences of the same letter, e.g. "food" would become
  # "fd" if we delete 'o', which is obviously wrong. Have to use delete_at(index) instead
  if i = string_2_arr.index(c)
    string_2_arr.delete_at i 
  else
    puts "Not anagrams"
    exit
  end
end

puts "Anagrams!"
\$\endgroup\$
2
\$\begingroup\$

You should abstract the code to avoid duplication. Also, use a counter to get a O(n) algorithm:

module Enumerable # or require 'facets/enumerable/frequency'
  def frequency
    each_with_object(Hash.new(0)) { |item, counter| counter[item] += 1 }  
  end
end

def anagrams?(s1, s2)
  frequency = proc { |s| s.gsub(/\s+/, "").downcase.chars.frequency }
  frequency.(s1) == frequency.(s2)
end

if ARGV.size != 2
  $stderr.puts("Need two words")
  exit(1)
elsif anagrams?(*ARGV)
  puts("Anagrams")
else
  puts("Not anagrams")
end
\$\endgroup\$
  • \$\begingroup\$ +1 nice! Not knowing facet's frequency, I was thinking something along the lines of group_by and then mapping values by count - same result, but a roundabout way of getting there compared to yours, so I didn't pursue it, and just used Jerry Coffin's suggestion. Btw, wouldn't reduce be more appropriate than each_with_object? \$\endgroup\$ – Flambino Aug 16 '14 at 1:01
  • 1
    \$\begingroup\$ It would be a bit uglier: reduce(Hash.new(0)) { |counter, item| counter[item] += 1; counter }. \$\endgroup\$ – tokland Aug 16 '14 at 7:49
  • \$\begingroup\$ Ah, you're right, of course. Hadn't thought it through \$\endgroup\$ – Flambino Aug 16 '14 at 7:53
  • \$\begingroup\$ The tradeoff made here is for increased memory usage (for the hash) to work in O(n) time. If you had limited memory you could do s1.downcase.gsub(/\s/,'').chars.sort and compare the two arrays, for in-place memory but O(n log n) complexity. \$\endgroup\$ – Devon Parsons Mar 9 '15 at 13:08
2
\$\begingroup\$

Jerry Coffin's answer is spot on regarding how one might solve this task more efficiently. I'll just look at your code, as-is

  • I see duplication. You have to do the same thing to both strings, so make it method:

    def characters_in_string(string)
      string.downcase.gsub(/\s/, '').split
    end
    
  • There's also some duplication in that there are 2 exit points for your script. Again, it could be cleaner to wrap the "meat" of your script in a method that simply returns a boolean (for instance, anagrams?(string1, string2)).

  • You might want to check ARGC before doing anything. If only 1 string is passed to the script, you can skip everything else.

  • Just to follow linux/unix scripting conventions, you could consider exiting with a non-zero status, if the anagram check fails. I know it's not really required for this but it's good practice nonetheless.


Using Jerry Coffin's approach, you get something like

def characters_in_string(string)
  string.downcase.gsub(/\s/, '').chars.sort
end

def anagrams?(string1, string2)
  characters_in_string(string1) == characters_in_string(string2)
end

if anagrams?(ARGV[0], ARGV[1])
  puts "Anagrams!"
else
  puts "Not anagrams"
  exit 1
end

I've left out the argument checking as an exercise to the reader

\$\endgroup\$

Your Answer

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