← Back to Upcase

Is this a good refactoring?

(Kevin McGladdery) #1

I am working on a class that represents musical pitches. I had two methods, spell_as_sharp and spell_as_flat that were completely identical except for exactly two characters. One of those characters was just a string, but the other represented a method. spell_as_sharp subtracted one from a given number, where spell_as_flat added one. I did this…

Before:

def spell_as_sharp 
  num = self.pitch_number
  if VALID_PITCHES.has_value? num
    VALID_PITCHES.key(num)
  else
    "#{VALID_PITCHES.key(ensure_number_scale(num - 1))}#"
  end
end

def spell_as_flat 
  num = self.pitch_number
  if VALID_PITCHES.has_value? num
    VALID_PITCHES.key(num)
  else
    "#{VALID_PITCHES.key(ensure_number_scale(num + 1))}b"
  end
end

And after:

def spell_as_sharp
  respell :-, "#"
end

def spell_as_flat
  respell :+, "b"
end

def respell method, accidental
  num = self.pitch_number
  if VALID_PITCHES.has_value? num
    VALID_PITCHES.key(num)
  else
    VALID_PITCHES.key(ensure_number_scale(num.send(method, 1))) + accidental
  end
end

Is this a good idea, or have I been writing too much scheme recently? Is it idiomatic?

Thanks

edit: The entire class is here.

0 Likes

(Aaron Renner) #2

What do you think about storing the sharp in two separate hashes and then indexing indexing into the hash based on your pitch number? It removes the need for the calculations in respell.

Here’s a gist: https://gist.github.com/aaronrenner/fd816206a30c4d42914c

EDIT: Gist link wasn’t showing up

0 Likes

(Luís Ferreira) #3

You could make a bit simple (IMO):

def spell_as_sharp
  respell pitch_number - 1, "#"
end

def spell_as_flat
  respell pitch_number + 1, "b"
end

def respell num, accidental
  if VALID_PITCHES.has_value? num
    VALID_PITCHES.key(num)
  else
    VALID_PITCHES.key(ensure_number_scale(num)) + accidental
  end
end

Also you could move the strings into constants with good names.

0 Likes