2
\$\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 Answers 2

Reset to default
3
\$\begingroup\$

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

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\$
4
  • \$\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\$ Commented Aug 16, 2014 at 1:01
  • 1
    \$\begingroup\$ It would be a bit uglier: reduce(Hash.new(0)) { |counter, item| counter[item] += 1; counter }. \$\endgroup\$ Commented Aug 16, 2014 at 7:49
  • \$\begingroup\$ Ah, you're right, of course. Hadn't thought it through \$\endgroup\$ Commented Aug 16, 2014 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\$ Commented Mar 9, 2015 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\$

You must log in to answer this question.