3
\$\begingroup\$

I recently was given this question for an interview application and am wondering where I went wrong. (The whole answer is contained in a skeleton which they gave me which I haven't included here so it's possible I failed due to a mistake outside the files posted here.)

EDIT: So I got feedback from the company:

Cons:

  • No use of Enumerable methods e.g. #map

  • Use of RuntimeError for validations and error messages. Handling this kind of logic without exceptions tends to be more flexible.

  • Accessing instance variables from outside the object.

  • No use of existing mocking libraries.

  • #clear test is not an effective test.

  • Everything in a single file.

  • Ruby's code conventions are not really followed.

I have two questions about this.

  1. What do they mean by "No use of Enumerable methods e.g. #map"?
  2. What should I be doing instead of using set_table for my unit tests? I created this because I wanted to be able to create a table without create_table, so my tests stay independent.

The program is run by:

bitmap_reader = BitmapEditorFileReader()
bitmap_reader.run()

The program should read a file containing string commands, and edit a 2D list based on these commands. The question is framed as a bitmap editor, i.e. the 2D list represents a bitmap image, with each element being a letter which represents a colour. The commands are as follows:

I N M - Create a new M x N image with all pixels coloured white (O).

C - Clears the table, setting all pixels to white (O).

L X Y C - Colours the pixel (X,Y) with colour C.

V X Y1 Y2 C - Draw a vertical segment of colour C in column X between rows Y1 and Y2 (inclusive).

H X1 X2 Y C - Draw a horizontal segment of colour C in row Y between columns X1 and X2 (inclusive).

S - Show the contents of the current image

class BitmapEditorFileReader
  def run(file)
    return puts "please provide correct file" if file.nil? || !File.exists?(file)

    bitmap_editor = BitmapEditor.new
    command_reader = CommandReader.new(bitmap_editor)
    File.open(file).each do |line|
      command_reader.defer_to_method(line)
    end
  end
end

class BitmapEditor
  def create_table(width, height)
    _validate_height_and_width(width, height)
    @width, @height = width, height
    @table = Array.new(height) {Array.new(width, "O")}
  end

  def clear
    _check_table_exists()
    @table.each do |row|
      row.each do |element|
        element = "O"
      end
    end
  end

  def colour_pixel(x, y, colour)
    _check_table_exists()
    x, y = _change_strings_to_integers([x, y])
    _validate_rows_and_columns([x], [y])
    @table[y-1][x-1] = colour
  end

  def vertical_line(x, y_1, y_2, colour)
    _check_table_exists()
    _validate_rows_and_columns([x], [y_1, y_2])
    min_y, max_y = [y_1, y_2].minmax
    y = min_y
    while y <= max_y
      colour_pixel(x, y, colour)
      y = y + 1
    end
  end

  def horizontal_line(x_1, x_2, y, colour)
    _check_table_exists()
    _validate_rows_and_columns([x_1, x_2], [y])
    min_x, max_x = [x_1, x_2].minmax
    x = min_x
    while x <= max_x
      colour_pixel(x, y, colour)
      x = x + 1
    end
  end

  def display
    _check_table_exists()
    @table.each do |row|
      row.each do |pixel|
        print pixel
      end
      puts ""
    end
  end

  def _check_table_exists
    if @table == nil
      raise "Table has not yet been created. Please create table before other commands"
    end
  end


  def _validate_height_and_width(width, height)
    if width <= 0 or height <= 0
      raise "Height and Width must both be greater than 0"
    end
  end

  def _validate_rows_and_columns(columns, rows)
    rows.each do |row|
      if row < 1 or row > @height
        raise "Row must be between 1 and the height (inclusive)"
      end
    end

    columns.each do |column|
      if column < 1 or column > @width
        raise "Column must be between 1 and the width (inclusive)"
      end
    end
  end

  def _change_strings_to_integers(args)
    integer_args = []
    begin
      args.each do |arg|
        int_arg = arg.to_i
        integer_args.push(int_arg)
      end
    rescue SyntaxError
      raise "All numbers must be integers"
    end

    return integer_args
  end
end


class CommandReader
  def initialize(bitmap_editor)
    @bitmap_editor = bitmap_editor
  end

  @@command_map = {
    "I" => "create_table",
    "C" => "clear",
    "L" => "colour_pixel",
    "V" => "vertical_line",
    "H" => "horizontal_line",
    "S" => "display"
  }

  @@argument_types_map = {
    "I" => ["Integer", "Integer", "String"],
    "C" => [],
    "L" => ["Integer", "Integer", "String"],
    "V" => ["Integer", "Integer", "Integer", "String"],
    "H" => ["Integer", "Integer", "Integer", "String"],
    "S" => []
  }

  def defer_to_method(command_string)
    method_code, *args = command_string.split

    method_string = @@command_map[method_code]

    argument_types = @@argument_types_map[method_code]
    args = _convert_args_types(args, argument_types)

    @bitmap_editor.send(method_string, *args)
  end

  def _convert_args_types(args, argument_types)
    arg_type = nil
    index = nil
    begin
      args.each_with_index do |arg, index|
        arg_type = argument_types[index]
        args[index] = send(arg_type, arg)
      end
    rescue ArgumentError
      raise "Argument number #{index} must be of type #{arg_type}"
    end
    return args
  end
end

Unit Tests:

require "bitmap_editor"
require "rspec"

def set_table(bitmap_editor, x, y)
  bitmap_editor.instance_variable_set(:@table, Array.new(y) {Array.new(x, "O")})
  bitmap_editor.instance_variable_set(:@width, x)
  bitmap_editor.instance_variable_set(:@height, y)
end

describe BitmapEditor do

  describe '#CreateTable' do
    before(:each) do
      @bitmap_editor = BitmapEditor.new
    end

    it 'should raise exception on negative dimension argument' do
      expect{@bitmap_editor.create_table(4, -1)}.to raise_error(RuntimeError)
    end

    it 'should raise exception on zero dimension argument' do
      expect{@bitmap_editor.create_table(4, 0)}.to raise_error(RuntimeError)
    end

    it 'should create table of given positive dimensions' do
      @bitmap_editor.create_table(4, 5)
      expect(@bitmap_editor.instance_variable_get(:@table).length).to eq(5)
      expect(@bitmap_editor.instance_variable_get(:@table)[0].length).to eq(4)
    end
  end

  describe '#Clear' do
    before(:each) do
      @bitmap_editor = BitmapEditor.new
    end

    it 'should raise exception if table not created yet' do
      expect{@bitmap_editor.clear()}.to raise_error(RuntimeError)
    end

    it 'should clear table if table created' do
      set_table(@bitmap_editor, 4, 5)
      @bitmap_editor.clear()

      @bitmap_editor.instance_variable_get(:@table).each do |row|
        row.each do |element|
          expect(element).to eq("O")
        end
      end
    end
  end

  describe '#ColourPixel' do

    before(:each) do
      @bitmap_editor = BitmapEditor.new
    end

    it 'should raise exception if table not created yet' do 
      expect{@bitmap_editor.colour_pixel(4, 5, "C")}.to raise_error(RuntimeError)
    end

    it 'should raise exception if negative index given' do
      set_table(@bitmap_editor, 4, 5)

      expect{@bitmap_editor.colour_pixel(2, -1, "C")}.to raise_error(RuntimeError)
    end

    it 'should raise exception if index is larger than table dimension' do
      set_table(@bitmap_editor, 4, 5)

      expect{@bitmap_editor.colour_pixel(2, 6, "C")}.to raise_error(RuntimeError)
    end

    it 'should change colour of element at given location' do
      set_table(@bitmap_editor, 4, 5)

      @bitmap_editor.colour_pixel(2, 2, "C")

      @bitmap_editor.instance_variable_get(:@table).each_with_index do |row, y|
        row.each_with_index do |element, x|
          if x == 1 and y == 1
            expect(element).to eq("C")
          else
            expect(element).to eq("O")
          end
        end
      end
    end
  end

  describe '#VerticalLine' do
    before(:each) do
      @bitmap_editor = BitmapEditor.new
    end

    it 'should raise exception if table not created yet' do 
      expect{@bitmap_editor.vertical_line(2, 1, 3, "C")}.to raise_exception(RuntimeError)
    end

    it 'should raise exception if negative vertical index given' do 
      set_table(@bitmap_editor, 4, 5)
      expect{@bitmap_editor.vertical_line(2, -1, 3, "C")}.to raise_exception(RuntimeError)
    end

    it 'should raise exception if negative horizontal index given' do 
      set_table(@bitmap_editor, 4, 5)
      expect{@bitmap_editor.vertical_line(-2, 1, 3, "C")}.to raise_exception(RuntimeError)
    end

    it 'should raise exception if vertical index larger than table dimension' do 
      set_table(@bitmap_editor, 4, 5)
      expect{@bitmap_editor.vertical_line(2, 1, 6, "C")}.to raise_exception(RuntimeError)
    end

    it 'should raise exception if horizontal index larger than table dimension' do 
      set_table(@bitmap_editor, 4, 5)
      expect{@bitmap_editor.vertical_line(6, 1, 3, "C")}.to raise_exception(RuntimeError)
    end

    it 'should draw vertical line at given location' do
      set_table(@bitmap_editor, 4, 5)

      @bitmap_editor.vertical_line(2, 1, 3, "C")

      @bitmap_editor.instance_variable_get(:@table).each_with_index do |row, y|
        row.each_with_index do |element, x|
          if x == 1 and y >= 0 and y <= 2
            expect(element).to eq("C")
          else
            expect(element).to eq("O")
          end
        end
      end
    end
  end

  describe '#HorizontalLine' do
    before(:each) do
      @bitmap_editor = BitmapEditor.new
    end

    it 'should raise exception if table not created yet' do 
      expect{@bitmap_editor.horizontal_line(1, 3, 2, "C")}.to raise_exception(RuntimeError)
    end

    it 'should raise exception if negative vertical index given' do 
      set_table(@bitmap_editor, 4, 5)
      expect{@bitmap_editor.horizontal_line(1, 3, -2, "C")}.to raise_exception(RuntimeError)
    end

    it 'should raise exception if negative horizontal index given' do 
      set_table(@bitmap_editor, 4, 5)
      expect{@bitmap_editor.horizontal_line(-1, 3, 2, "C")}.to raise_exception(RuntimeError)
    end

    it 'should raise exception if vertical index larger than table dimension' do 
      set_table(@bitmap_editor, 4, 5)
      expect{@bitmap_editor.horizontal_line(1, 3, 6, "C")}.to raise_exception(RuntimeError)
    end

    it 'should raise exception if horizontal index larger than table dimension' do 
      set_table(@bitmap_editor, 4, 5)
      expect{@bitmap_editor.horizontal_line(1, 6, 2, "C")}.to raise_exception(RuntimeError)
    end

    it 'should draw horizontalline at given location' do
      set_table(@bitmap_editor, 4, 5)

      @bitmap_editor.horizontal_line(1, 3, 2, "C")

      @bitmap_editor.instance_variable_get(:@table).each_with_index do |row, y|
        row.each_with_index do |element, x|
          if y == 1 and x >= 0 and x <= 2
            expect(element).to eq("C")
          else
            expect(element).to eq("O")
          end
        end
      end
    end
  end

  describe '#Display' do
    before(:each) do
      @bitmap_editor = BitmapEditor.new
    end

    it 'should raise exception if table not created yet' do
      expect{@bitmap_editor.display()}.to raise_exception(RuntimeError)
    end

    it 'should display table if table created' do
      set_table(@bitmap_editor, 4, 5)
      $stdout = StringIO.new

      @bitmap_editor.display()

      output = "OOOO\nOOOO\nOOOO\nOOOO\nOOOO\n"
      expect(output).to eq($stdout.string)
    end
  end
end

describe CommandReader do

  class MockBitmapEditor
    def initialize
      @call_record = {
        "create_table" => nil,
        "clear" => nil,
        "colour_pixel" => nil,
        "vertical_line" => nil,
        "horizontal_line" => nil,
        "display" => nil
      }
    end

    def create_table(*args)
      @call_record["create_table"] = args
    end

    def clear
      @call_record["clear"] = []
    end

    def colour_pixel(*args)
      @call_record["colour_pixel"] = args
    end

    def vertical_line(*args)
      @call_record["vertical_line"] = args
    end

    def horizontal_line(*args)
      @call_record["horizontal_line"] = args
    end

    def display
      @call_record["display"] = []
    end

    def get_args_of_call(method_string)
      return @call_record[method_string]
    end
  end

  describe '#DeferToMethod' do
    before(:each) do
      @mock_editor = MockBitmapEditor.new
      @command_reader = CommandReader.new(@mock_editor)
    end

    it 'should call create_table method' do
      command = "I 1 2"

      @command_reader.defer_to_method(command)

      expect(@mock_editor.get_args_of_call("create_table")).to eq([1, 2])
    end

    it 'should call clear method' do
      command = "C"

      @command_reader.defer_to_method(command)

      expect(@mock_editor.get_args_of_call("clear")).to eq([])
    end

    it 'should call colour_pixel method' do
      command = "L 3 4 A"

      @command_reader.defer_to_method(command)

      expect(@mock_editor.get_args_of_call("colour_pixel")).to eq([3, 4, "A"])
    end

    it 'should call vertical_line method' do
      command = "V 5 6 7 B"

      @command_reader.defer_to_method(command)

      expect(@mock_editor.get_args_of_call("vertical_line")).to eq([5, 6, 7, "B"])
    end

    it 'should call horizontal_line method' do
      command = "H 8 9 10 C"

      @command_reader.defer_to_method(command)

      expect(@mock_editor.get_args_of_call("horizontal_line")).to eq([8, 9, 10, "C"])
    end

    it 'should call print method' do
      command = "S"

      @command_reader.defer_to_method(command)

      expect(@mock_editor.get_args_of_call("display")).to eq([])
    end
  end
end
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Can you include the tests? Does it pass all the tests? \$\endgroup\$ Commented Feb 2, 2018 at 19:46
  • \$\begingroup\$ Just included...yes it passes all the tests \$\endgroup\$ Commented Feb 4, 2018 at 15:34

1 Answer 1

Reset to default
1
\$\begingroup\$

I don't know any Ruby, so I can't speak to that. I do have a few ideas on the cons you got in the feedback though:

  1. No use of Enumerable methods e.g. #map

Many languages support some type of mapping functionality; given an iterable and a function, apply the function to each element of the iterable. This is especially common in functional programming languages, but it is a useful enough feature that a lot of other languages have borrowed it as well. In your case, you have a lot of these:

File.open(file).each do |line|
  command_reader.defer_to_method(line)
end

This really is just (I assume; I don't know ruby)

iterable.each do |itemOfIterable|
    function(itemOfIterable)
end

Ruby's #map function appears to support this.

File.open(file).map { |line| command_reader.defer_to_method(line) }

Whether or not this is better is a different question; they appear to prefer it. As with all choices, you should consider if what you're doing makes sense due to efficiency, cleanliness, idiomatic-ness, how it fits the codebase's style guidelines, etc.

  1. Use of RuntimeError for validations and error messages

You could have a more descriptive error type than RuntimeError (I assume; again, no Ruby). Other than that, however, if there isn't anything that your program can do to fix the error itself it either needs to ask a human for help or quit as gently as it can. In this case I don't think there is a good way to ask for help, so outputting a useful error and quitting seems appropriate.

  1. Accessing instance variables from outside the object

I'm with them on this. You've defined the API for your class, and then you're ignoring it. Although you want to keep your tests unique, obviously some things (such as constructing an object) are hard to ignore. In this case, you should make the assumption that you've already tested the create_table function and that it works correctly, and then use it without fear in all other cases.

  1. No use of existing mocking libraries

I don't know anything about Ruby, so I don't know what those would be. That being said, it often makes sense to enable dependency injection (look it up) of things that your class depends on, but you don't necessarily want to test (for example, file IO). Mocking them allows you to create a fake version you control, so you can test that your code functions as expected as long as the other library does what it says it does. You aren't shouldn't be responsible for testing other people's code. That doesn't mean you never are, but by default you should assume that they work as documented.

  1. #clear test is not an effective test

Not positive what they mean here, sorry. I'd assume that they want more test cases.

  1. Everything is in a single file

I don't know what Ruby/this company's style guidelines are. In general, I strongly dislike the Java approach of "everything gets its own class and file"; it makes it harder to group things logically without clicking around to find where something is implemented. As long as everything makes sense to be together, and your code is still organized logically, I'm fine with everything in a single file

  1. Ruby's code conventions are not really followed.

Don't know anything about this; Python has PEP-8, which is the de-facto style guide. Ruby might have something similar

A few of the things they pointed out are definitely valid; a few others I don't know if I agree with. In general, this highlights the importance of explaining why you do things the way you do (for example, grouping everything in a single file) and justifying them. This will give you more points, and probably open up a discussion with the interviewer, rather than just doing something and assuming they agree with you.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.