6
\$\begingroup\$

I am requesting a review for my solution to this problem from exercism.io. I'm posting it here since there is not always input on solution submitted through exercism.

The problem is as stated:

Calculate the Hamming difference between two DNA strands.

A mutation is simply a mistake that occurs during the creation or copying of a nucleic acid, in particular DNA. Because nucleic acids are vital to cellular functions, mutations tend to cause a ripple effect throughout the cell. Although mutations are technically mistakes, a very rare mutation may equip the cell with a beneficial attribute. In fact, the macro effects of evolution are attributable by the accumulated result of beneficial microscopic mutations over many generations.

The simplest and most common type of nucleic acid mutation is a point mutation, which replaces one base with another at a single nucleotide.

By counting the number of differences between two homologous DNA strands taken from different genomes with a common ancestor, we get a measure of the minimum number of point mutations that could have occurred on the evolutionary path between the two strands.

This is called the 'Hamming distance'.

It is found by comparing two DNA strands and counting how many of the nucleotides are different from their equivalent in the other string.

GAGCCTACTAACGGGAT
CATCGTAATGACGGCCT
^ ^ ^  ^ ^    ^^

The Hamming distance between these two DNA strands is 7.

The tests for the problem were provided:

require 'minitest/autorun'
require_relative 'hamming'

# Common test data version: 2.0.1 f79dfd7
class HammingTest < Minitest::Test
  def test_empty_strands
    # skip
    assert_equal 0, Hamming.compute('', '')
  end

  def test_identical_strands
    # skip
    assert_equal 0, Hamming.compute('A', 'A')
  end

  def test_long_identical_strands
    # skip
    assert_equal 0, Hamming.compute('GGACTGA', 'GGACTGA')
  end

  def test_complete_distance_in_single_nucleotide_strands
    # skip
    assert_equal 1, Hamming.compute('A', 'G')
  end

  def test_complete_distance_in_small_strands
    # skip
    assert_equal 2, Hamming.compute('AG', 'CT')
  end

  def test_small_distance_in_small_strands
    # skip
    assert_equal 1, Hamming.compute('AT', 'CT')
  end

  def test_small_distance
    # skip
    assert_equal 1, Hamming.compute('GGACG', 'GGTCG')
  end

  def test_small_distance_in_long_strands
    # skip
    assert_equal 2, Hamming.compute('ACCAGGG', 'ACTATGG')
  end

  def test_non_unique_character_in_first_strand
    # skip
    assert_equal 1, Hamming.compute('AAG', 'AAA')
  end

  def test_non_unique_character_in_second_strand
    # skip
    assert_equal 1, Hamming.compute('AAA', 'AAG')
  end

  def test_same_nucleotides_in_different_positions
    # skip
    assert_equal 2, Hamming.compute('TAG', 'GAT')
  end

  def test_large_distance
    # skip
    assert_equal 4, Hamming.compute('GATACA', 'GCATAA')
  end

  def test_large_distance_in_off_by_one_strand
    # skip
    assert_equal 9, Hamming.compute('GGACGGATTCTG', 'AGGACGGATTCT')

  end

  def test_disallow_first_strand_longer
    # skip
    assert_raises(ArgumentError) { Hamming.compute('AATG', 'AAA') }

  end

  def test_disallow_second_strand_longer
    # skip
    assert_raises(ArgumentError) { Hamming.compute('ATA', 'AGTG') }

  end

And here is the solution I came up with to solve this problem:

class Hamming
  def self.compute(original, copy)
    unless original.length == copy.length
      raise(ArgumentError)
    end

    hamming = 0
    copy_char = copy.split('')
    original.each_char.with_index do |character, index|
      unless copy_char[index] == character
        hamming += 1
      end
    end
      hamming
  end
end

Please critique and/or suggest how I can improve this solution using Ruby with explanations.

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

1) Start using Rubocop gem.

2) Prefer guard clauses & stop execution early in a method. (Also, added 2 more conditions.):

raise ArgumentError unless original.length == copy.length
return 0 if original.empty? && copy.empty?
return 0 if original == copy

3) I would prefer Ruby's #char over #each_char in this case:

original = original.chars
copy     = copy.chars

4) I would prefer Ruby's #zip & #count over #with_index to have a better readability:

strands1.zip(strands2).count { |a, b| a != b }

At the end, it would look something like this:

class Hamming    
  def self.compute(strand1, strand2)
    # Error handling and edge cases
    raise ArgumentError unless strand1.length == strand2.length
    return 0 if strand1.empty? && strand2.empty?
    return 0 if strand1 == strand2

    strands1, strands2 = strand1.chars, strand2.chars
    # Merge elements of two arrays and compare arrays of array.
    strands1.zip(strands2).count { |a, b| a != b }
  end
end
\$\endgroup\$
  • 1
    \$\begingroup\$ I recommend discarding the two special cases, since the general case would handle them just fine. The special cases just add clutter for no performance benefit. \$\endgroup\$ – 200_success Jun 16 '18 at 6:58
  • \$\begingroup\$ @200_success aren't they required for first three test cases to pass? \$\endgroup\$ – ogirginc Jun 17 '18 at 8:11
0
\$\begingroup\$

You might like to consider the downside of all of the array/enumerable methods – poor performance, and not necessarily better looking code.

Sometimes, simpler can be better.

original.length.times.reject { |idx| original[idx] == other[idx] }.size

This creates one array, whereas decomposing two string of 15 characters each into array and then manipulating them can easily create 15 arrays and 30 strings during the processing. What if you have strings of 10,000 characters?

If your code does not have to create new objects then I'd suggest at least considering not doing so.

ps. if you knew that the strings were ASCII then you could ...

original.bytesize.times.reject { |i| original.getbyte(i) == other.getbyte(i) }.size
\$\endgroup\$

Your Answer

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

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