HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyMinor

Refactoring a simple Ruby CLI program

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
refactoringsimpleprogramcliruby

Problem

I tried to help someone in Stackoverflow with a refactoring exercise. Did many changes to his original code and made a somewhat decent solution (at least in my eyes). Was thinking, whether someone can critique on my implementation.

class Theatre
  COST = { running: 3, fixed: 180 }

  attr_accessor :number_of_audience, :ticket_price

  def revenue
    @number_of_audience * @ticket_price
  end

  def total_cost
    COST[:fixed] + (@number_of_audience * COST[:running])
  end

  def net
    revenue - total_cost
  end

  def profit?
    net > 0
  end
end

class TheatreCLI
  def initialize
    @theatre = Theatre.new
  end

  def seek_number_of_attendes
    print 'Number of audience: '
    @theatre.number_of_audience = gets.chomp.to_i
  end

  def seek_ticket_price
    print 'Ticket price: '
    @theatre.ticket_price = gets.chomp.to_i
  end

  def print_revenue
    puts "Revenue for the theatre is RM #{@theatre.revenue}."
  end

  def print_profit
    message_prefix = @theatre.profit? ? 'Profit made' : 'Loss incurred'
    puts "#{message_prefix} #{@theatre.net.abs}."
  end

  def self.run
    TheatreCLI.new.instance_eval do
      seek_ticket_price
      seek_number_of_attendes

      print_revenue
      print_profit
    end
  end
end

TheatreCLI.run

Solution

It's very strange you're using instance_eval. That's not how a regular program should work and is not needed in your code.

If you want to avoid calling 4 instance methods from a class instance method, then you can create a new instance method like:

def workflow
  seek_ticket_price
  seek_number_of_attendes

  print_revenue
  print_profit
end
def self.run
  TheatreCLI.new.workflow
end


Maybe it's not a problem here, but using a lot of instance_eval for saving keystrokes looks like working around a bad API. Also my gut feeling tells me that such practice can lead to unexpected troubles if used outside class body.

Code Snippets

def workflow
  seek_ticket_price
  seek_number_of_attendes

  print_revenue
  print_profit
end
def self.run
  TheatreCLI.new.workflow
end

Context

StackExchange Code Review Q#77683, answer score: 2

Revisions (0)

No revisions yet.