Binary converter and sufficient tests

Question

I just finished writing a basic binary conversion class in Ruby. I'm curious to see if anyone has suggestions for improvements. In particular, I'm looking for the cleanest, shortest, most succinct code possible, while staying away from hacky shortcut stuff.

In creating this I wanted to use a minimum of functions that come with Ruby by default, as that would sort of defeat the purpose of this as a practice program. For instance, the entire thing could have been summed up with a quick line like this.

"101".to_i(2) ##> 5

But I wanted to do most of the math by hand, so to speak.

There is one place in the get_bits method I did use a string, and I'm not too happy about it. However it was the only way I could think of to get the bits into an array. If there's another way to do so without splitting a string, I'd be glad to hear it.

Here's the class.

class Converter
 def initialize binary_number
 @binary = binary_number
 @bits = get_bits
 @total = 0
 self
 end
 def to_base_ten
 validate_binary
 @bits.reverse.each_with_index do |bit, power|
 append_to_total bit_value(bit, power)
 end
 get_total
 end
 private
 def get_bits
 @binary.to_s.split('').map do |bit|
 bit.to_i
 end
 end
 def validate_binary
 @bits.each do |bit|
 raise "Invalid binary!" unless bit == 0 || bit == 1
 end
 end
 def bit_value bit, power
 is_set?(bit) ? two_to_power_of(power) : 0
 end
 def two_to_power_of power
 total = 1
 power.times do
 total = total * 2
 end
 total
 end
 def is_set? bit
 bit == 1
 end
 def append_to_total amount
 @total = @total + amount
 end
 def get_total
 @total
 end
end

I also used RSpec to test it as best I could, but I won't post the specs here, just because they're long and not directly related to the class itself. If you want to inspect those too, they can be found on GitHub.

So there ya have it. Any thoughts are greatly appreciated. Clean code forever!

Answer 1

1. The name Converter is pretty ambiguous. It could mean anything.

2. I'd suggest doing less work in the initializer, and more lazy evaluation.

3. Skip that self line in the initializer - it's an initializer; it'll always return self (or, technically, it doesn't matter what the initializer returns, since you'll be calling new and that will return the new instance. Point is, skip that line)

4. Use Enumerable#inject to do the sum

5. Use String#chars instead of String#split

6. It'd probably be better to raise right away in the initializer if the string isn't a binary number. Also better to raise an ArgumentError while you're at it.

7. You can check the string with a simple regular expression: /^[01]*$/

8. Your #two_to_power_of method can be replaced with 2 ** n

9. Don't use prefixes like is_ and get_ in your method names (specifically, is_set?, get_bits and get_total); it's not Ruby's style. For instance, the nil? method isn't called is_nil?, and the chars method mentioned above isn't called get_chars.
   All methods return something so a get_ is redundant, and a ? suffix is a substitute for an is_ prefix, just like a = suffix is a substitute for a set_ prefix.

10. Favor accessor methods over directly accessing instance variables. Such indirections allow for better internal decoupling. That is, as soon as you start accessing a value through a method, you make your class more flexible. You can begin with simple attr_reader-synthesized methods, which will of course not do anything special, but should you ever need or want to add more logic, you need only change the accessor method(s). In this case, however, a first step would be to simply use fewer instance variables (see #2)

11. A class method might be nice for quick conversions, so you don't always have to deal with instantiating an object. E.g. a class method could allow you to say Converter.convert("101010") and get 42 directly. It'd just be a shortcut for Converter.new("101010").to_base_ten, but that's still a nice shortcut to have.

12. Might be nice if you could specify endianness of the binary number, instead of assuming it's always little endian.

In the end, I get something like this, if we want memoization of the converted values:

class BinaryNumberConverter
 attr_reader :binary
 def self.convert(string, little_endian = true)
 self.new(string).base_ten(little_endian)
 end
 def initialize(binary)
 if binary =~ /^[01]*$/
 @binary = binary
 else
 raise ArgumentError.new("'#{binary}' is not a valid binary number")
 end
 end
 def base_ten(little_endian = true)
 little_endian ? little_endian_value : big_endian_value
 end
 private
 def little_endian_value
 @little_endian ||= convert(binary.reverse)
 end
 def big_endian_value
 @big_endian ||= convert(binary)
 end
 def convert(string)
 string.chars.each_with_index.inject(0) do |sum, digit|
 char, index = digit
 sum += char == "1" ? 2 ** index : 0
 end
 end
end

Usage:

instance = BinaryNumberConverter.new("101010")
instance.base_ten # => 42
instance.base_ten(false) # => 21
BinaryNumberConverter.convert("101010") # => 42
BinaryNumberConverter.convert("101010", false) # => 21

Of course, it'd be a lot simpler to just extend String rather than make an entire class:

class String
 def bin_to_dec(little_endian = true)
 raise "'#{self}' is not a valid binary number" unless self =~ /^[01]*$/
 digits = little_endian ? reverse.chars : chars
 digits.each_with_index.inject(0) do |sum, digit|
 char, index = digit
 sum += char == "1" ? 2 ** index : 0
 end
 end
end

And you get

"101010".bin_to_dec #=> 42
"101010".bin_to_dec(false) #=> 21

No memoization, but I'd call that pretty clean.

---

Took a quick look at your tests, and it's a bit overkill (also, you should use the expect syntax of rspec; the should syntax is old-school).

I'd actually just do something like

number = rand(12345) # can be anything really
string = number.to_s(2)
expect(string.bin_to_dec).to eq(number)

And call it good. You'll note I'm using to_s(2) above, but here it makes sense. We have to assume that Ruby works anyway, so in this case, it's a perfect yardstick. Similarly, to check big endian conversion, we can use to_i(2) with a clear conscience

string = rand(12345).to_s(2).reverse
expect(string.bin_to_dec(false)).to eq(string.to_i(2))

Add a test for the exception raising, and you've tested everything.

---

On another note, you say you "wanted to use a minimum of functions that come with Ruby by default as that would sort of defeat the purpose of this as a practice program". But I'd recommend you use as much built-in functionality as possible, especially in your practice programs (in this case stopping short of using String#to_i, of course).

Learning any language is usually less about learning the language itself (as in syntax), as it is about learning the conventions, idioms, and the built-in goodies. Intentionally doing things "the hard way" will probably teach you the wrong lessons. And the code you write won't teach you conventions and idioms, because it's unconventional to make things hard for yourself and no idioms exist for it.

Besides, at first, the easy solution will actually be the difficult one, because the easy solution is the one that requires experience. But brute-forcing your way though a problem won't earn you that experience and will actually teach you a lot less than trying to use the language to your advantage.

• David Harkness: Can you please expand on points 9 and 10? All the others get a resounding +1 from me.
• Flambino: @DavidHarkness Thanks! I've fleshed it out a bit more; let me know if it's still lacking
• David Harkness: Nope, that was perfect.
• Scotty C.: Thanks for the excellent answer. There's a lot of useful information there.