2
\$\begingroup\$

The code creates a table and adds rows and columns.

  • Does it make sense to write the code like this?
  • Would you design it differently?
  • Is there a better OOP approach to this?
class Table
  attr_reader :grid

  def initialize(rows: , columns:)
    @grid = construct(rows, columns)
  end

  def construct(rows, columns)
    raise 'arguments must be arrays' unless rows.is_a?(Array) && columns.is_a?(Array)
    table = []
    rows = [] if rows.nil?
    rows.each do |row|
      table << columns.map do |c| c end
    end
    table
  end
end

Here are the specs which describe what it is doing:

require 'spec_helper'
require_relative '..\test'

RSpec.describe Table do 

  it 'is empty' do
    table = Table.new(rows: [], columns: [])
    expect(table.grid).to eq []
  end

  it 'raises if no array is given' do
    expect { 
      Table.new(rows: [], columns: 1) }.to raise_error RuntimeError
    expect { 
      Table.new(rows: 1, columns:[]) }.to raise_error RuntimeError
  end

  it 'has several rows and columns' do
    row = double('row')
    column = double('column')
    col = Column.new.value = 14
    table = Table.new(rows: [1, 3, 3], columns: [1, col])
    expect(table.grid).to eq [
      [1, 2], 
      [1, 2], 
      [1, 2]
    ]
  end
end

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

Refactored Code

This is a possible way to refactor the code to meet the style guidelines of Ruby using Rubocop.

# frozen_string_literal: true

# A simple table with rows and cols
class Table
  attr_reader :grid

  def initialize(rows:, columns:)
    @grid = construct(rows, columns)
  end

  def construct(rows, columns)
    unless rows.is_a?(Array) && columns.is_a?(Array)
      raise 'arguments must be arrays'
    end

    table = []
    rows = [] if rows.nil?
    rows.each do |_row|
      table << columns.map { |c| c }
    end
    table
  end
end

Style Considerations

A guard clause should be appended wih an empty line. Also, since the line is over 80 characters, split it up in multi-line.

raise 'arguments must be arrays' unless rows.is_a?(Array) && columns.is_a?(Array)
table = []
unless rows.is_a?(Array) && columns.is_a?(Array)
  raise 'arguments must be arrays'
end

table = []

You have an unused block argument row and the map should be rewritten using {..}.

rows.each do |row|
  table << columns.map do |c| c end
end
rows.each do |_row|
  table << columns.map { |c| c }
end

General Guidelines and Conventions

The tool also complained about the following guidelines.

  • use 2 instead of 4 white spaces for indentation
  • remove any trailing white space
  • remove space before comma
  • add a frozen string literal comment justification
  • add top level class documentation comment
\$\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.