Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Ruby's Fixnum & Bignum can hold very large integers and automatically handle overflow. I decided to implement BitArray using an integer as storage. Please let me know what you think about code and functionality of a BitArray.

class BitArray
  include Enumerable

  def initialize size
    @size = size
    @field = 2**size 
  end

  def set positions
    bits = positions.kind_of?(Integer) ? [positions] : positions
    bits.each {  |position| @field |= 1 << (@size - position) } 
    self
  end

  def clear positions
    bits = positions.kind_of?(Integer) ? [positions] : positions
    bits.each {  |position| @field ^= 1 << (@size - position) } 
    self
  end  

  def get position
    (@field >> (@size - position) ).to_s(2)[-1].to_i
  end

  def each(&block)
    @field.to_s(2)[1..-1].split("").each { |bit_string| yield(bit_string.to_i) }
  end

  def to_s
    @field.to_s(2)[1..-1]
  end

  def count
    @field.to_s(2)[1..-1].split("").inject(0) {|sum,bit| sum + bit.to_i}
  end

end

And the Test

require 'minitest/autorun'
require_relative 'bit_array'

class BitArrayTest < MiniTest::Unit::TestCase

  def test_equal
    assert_equal "00000", BitArray.new(5).to_s
  end 

  def test_set
    assert_equal "00100", BitArray.new(5).set(3).to_s
    assert_equal "00010", BitArray.new(5).set(4).to_s
    assert_equal "11100", BitArray.new(5).set([1,2,3]).to_s
    assert_equal "100000000101", BitArray.new(12).set([1,10,12]).to_s
  end

  def test_clear
    assert_equal "01000", BitArray.new(5).set([1,2]).clear(1).to_s
  end

  def test_get
    assert_equal 0, BitArray.new(5).set([1,2]).get(3)
    assert_equal 0, BitArray.new(12).set([12]).get(11)
    assert_equal 0, BitArray.new(12).set([12]).get(1)
  end

  def test_count
    assert_equal 2, BitArray.new(5).set([1,2]).count
  end

  def test_iterate
    assert_equal ["0", "0", "0", "0", "0"],  BitArray.new(5).each {|n| puts n }
  end  

end
share|improve this question
    
Note: While answering your Bloom filter question, I came across a neat Fixnum method I didn't know about. See update in my answer. –  Flambino 35 mins ago

1 Answer 1

up vote 4 down vote accepted

My 2nd question on this site concerned bitmasks, actually. As @tokland correctly pointed out back then, bitmasks are really a pretty low-level technique that doesn't quite belong in a high-level language. Yes, it has its uses, but a hash or an array of symbols would solve the same kinds of problems in a more high-level, more expressive, and more readable manner.

In fact, trying to do this in a high-level language like Ruby presents some oddities, since you're using strings and integers to work with bits. And strings and integers are objects in Ruby, so you've got all these really high-level concepts and constructs involved in trying to do something with the lowest-level aspect of computing. It works, but it's like packing something tiny in a giant padded box.

Still, code's code, so:

  • Your #test_iterate doesn't quite test what you think. You're just printing to stdout. What the assertion is actually testing is the value of @field.to_s(2)[1..-1].split("") - it doesn't really test that anything is being iterated. This implementation would also pass your test, even though it never iterates anything:

    def each(&block)
      # block argument ignored, no iteration; passes tests anyway
      @field.to_s(2)[1..-1].split("")
    end
    

    With your current code, I'd write the test like:

    expected = [1, 1, 0]
    BitArray.new(3).set([1,2]).each do |bit|
      assert_equal expected.shift, bit
    end
    assert expected.empty? # check that we looped the expected number of times
    
  • You test for #get is also kinda strange. In all cases you check for 0 - how about a test where you expect to get 1 back? There really are only two possible return values, so you should probably check both.

  • Why on Earth is your class using a 1-based indexing scheme? It's an array of a sort, so why not use a zero-based index like, well, like an array would do? I know this confused me greatly.

  • Your #get method could perhaps just return a boolean. That's the closest you'll get to returning a raw bit.

    Update: I wasn't aware of this, but Fixnum actually has subscripting access to bits already. So #get can be written as simply:

    def get position
      @field[position]
    end
    
  • Your #set and #clear methods should probably just use splats - it'd let you skip the array-boxing if just a single integer's passed in:

    def set *positions # use a splat
      positions.each { |position| @field |= 1 << (@size - position) } 
      self
    end
    
  • unset might be a better name than clear - I'd expect clear to reset the entire array.

  • You're going out of your way to always calculating @size - (some position) to make sure that you fill your array "from the left". I.e. position 1 is the left-most bit, rather than the (more natural) right-most bit. However, there's really no need. Inside your class, you can do whatever you want. Your current tests mostly check the string representation, so you can just flip that, rather than flip everything else.

  • Filling from the right would also let you skip the size argument entirely; as you say, Ruby will catch overflows anyway, but you never actually check (or test) trying to get or set a bit outside the size.

  • It's also just simpler to find and flip bits if you go from the right, and use a zero-based index, since any bit is given as simply 2**position. For instance:

    def set *positions
      positions.each { |position| @field |= 2**position } 
      self
    end
    
  • You're including Enumerable, so use it. Your #count method could be written as just

    def count
      inject(0, :+)
    end
    

    since it'll use each, and each will yield either 1 or 0.

Again, this is all academic, as you're no doubt better off just using regular arrays or hashes. Still, here's a refactored version, because why not? Slightly different output for #to_s since I'm not using a size argument. And I'm using 0-based indices. But otherwise it's the same.

class BitArray
  include Enumerable

  def initialize
    @bits = 0
  end

  def set *positions
    positions.each { |position| @bits |= 2**position } 
    self
  end

  def unset *positions
    positions.each { |position| @bits ^= 2**position } 
    self
  end  

  def get position
    @bits[position]
  end

  def each(&block)
    bits = @bits
    until bits == 0
      yield bits & 1
      bits = bits >> 1
    end
  end

  def to_s
    @bits.to_s(2).reverse
  end

  def count
    inject(0, :+)
  end
end

require 'minitest/autorun'

class BitArrayTest < MiniTest::Test
  def test_equal
    assert_equal "0", BitArray.new.to_s
  end 

  def test_set
    assert_equal "0001", BitArray.new.set(3).to_s
    assert_equal "1011", BitArray.new.set(0, 2, 3).to_s
    assert_equal "0100000000101", BitArray.new.set(1, 10, 12).to_s
  end

  def test_unset
    assert_equal "101", BitArray.new.set(0, 1, 2).unset(1).to_s
  end

  def test_get
    assert_equal 1, BitArray.new.set(1, 2).get(2)
    assert_equal 0, BitArray.new.set(12).get(11)
    assert_equal 1, BitArray.new.set(12).get(12)
  end

  def test_count
    assert_equal 3, BitArray.new.set(1, 2, 5).count
  end

  def test_iterate
    expected = [1, 1, 0, 1]
    BitArray.new.set(0, 1, 3).each do |bit|
      assert_equal expected.shift, bit
    end
    assert expected.empty?
  end
end

Note that this version and its tests still don't deal with the issue of passing, say, negative indices or non-numeric indices.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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