3
\$\begingroup\$

Task: given two strings, that may or may not be of the same length, determine the number of characters you must delete to make the two strings anagrams of each other.

My solution:

a = gets.strip.chars
b = gets.strip.chars
len = (a+b).length
d = []

for i in 0..a.length-1
  b.each_with_index do |x, index|
    if x == a[i]
      d.push(a[i])
      b.delete_at(index)
      break
    end
  end
end

print len - d.length * 2

I'd like to see some opinions and suggestions about my code.

UPD. Just realised that that there is no point to use array 'd'. Simple counter will do the job. Am i supposed to answer my own question with this or just update this post?

a = gets.strip.chars
b = gets.strip.chars
len = (a+b).length
counter = 0

for i in 0..a.length-1
  b.each_with_index do |x, index|
    if x == a[i]
      counter += 1
      b.delete_at(index)
      break
    end
  end
end

print len - counter * 2
\$\endgroup\$
5
  • \$\begingroup\$ I can't make it run. Can you provide an example of input and expected output as well as what Ruby version are you using? \$\endgroup\$ Commented Aug 3, 2017 at 13:41
  • \$\begingroup\$ ruby 2.2.6p396 (2016-11-15 revision 56800) [x64-mingw32] \$\endgroup\$ Commented Aug 3, 2017 at 13:47
  • \$\begingroup\$ input1: debit card, input2: bad credit, output: 0 \$\endgroup\$ Commented Aug 3, 2017 at 13:52
  • \$\begingroup\$ input1: debit cardoo, input2: baddest credit, output: 6 \$\endgroup\$ Commented Aug 3, 2017 at 13:55
  • \$\begingroup\$ You answer your own question only when you have a valid answer, in this case you updated your post which is correct. I think you can replace the old code with the new code. This a code review, so we'll go back and forth so updates are expected. \$\endgroup\$ Commented Aug 3, 2017 at 14:41

2 Answers 2

1
\$\begingroup\$

Your code looks nice and clean for a beginner. There are some things that can be improved:

Structure

Put your code in a function. By doing that, you give it a name and can easily talk about it. Like this:

def distance_to_anagram(str1, str2)
  len = (a+b).length
  counter = 0
  …
  return len - counter * 2
end

Note that I left out the gets and print calls from the function. This makes the function usable in many areas, not those that read their input directly. You can also call it like this:

print distance_to_anagram("debit card", "bad credit")

Naming

The names a and b are perfect for a small piece of code.

The name counter could be more specific. What does it count? From the code you can already see that it is a counter (initialized to 0, modified using += 1), so by naming it matched, you answer the what question.

For a you use a for loop, and for b you use each_with_index. Since you do the same to both a and b, the code should reflect that. I would write it like this:

matched = 0
a.each_with_index do |ac, ai|
  b.each_with_index do |bc, bi|
    if ac == bc
      matched += 1
      b.delete_at(bi)
      break
    end
  end
end

I changed the variable names to make clear which index belongs to which string.

Performance

Your code works fine for small strings. But you should not try it for strings of a million characters, since it would take 1.000.000 * 1.000.000 steps. There are alternative algorithms that are more complicated to read and write, but will only take 2 * 1.000.000 steps. See mabe02's answer for an example.

\$\endgroup\$
1
\$\begingroup\$

I would approach this problem using hashes in order to reduce the complexity of your solution:

# anagrams.rb
require 'test/unit'

module Anagrams

  def solve(string1, string2)
    h1 = create_occurrencies_hash(string1)
    h2 = create_occurrencies_hash(string2)

    present_characters = (h1.keys + h2.keys).uniq
    resulting_hash = present_characters.each_with_object(Hash.new(0)) do |c, h|
      h[c] = (h1[c].to_i - h2[c].to_i).abs
    end
    resulting_hash.values.inject(:+)
  end

  private

  def create_occurrencies_hash(string)
    string.strip.chars.each_with_object(Hash.new(0)) { |c, h| h[c] += 1 }
  end
end

class AnagramTest < Test::Unit::TestCase
  include Anagrams

  def test_solve
    assert_equal(0, solve('debit card', 'bad credit'))
    assert_equal(0, solve('bad credit', 'debit card'))
    assert_equal(6, solve('debit cardoo', 'baddest credit'))
    assert_equal(6, solve('baddest credit', 'debit cardoo'))
  end
end

Running some tests against the cases you put in the comments, it gives:

$ ruby anagrams.rb 
Loaded suite anagrams
Started
.

Finished in 0.000519788 seconds.
-----------------------------------------------------------------------------------------------------------------------------------------------
1 tests, 4 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
-----------------------------------------------------------------------------------------------------------------------------------------------
1923.86 tests/s, 7695.45 assertions/s

I would definitely go for a clean solution like this one

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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