3
\$\begingroup\$

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
\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – akostadinov Jan 16 '15 at 7:43
  • \$\begingroup\$ I know its not very common; but it felt more readable like this; rather than calling the methods via an instance and having to repeat it 4 times. \$\endgroup\$ – Jikku Jose Jan 16 '15 at 15:01
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Yes, it felt bad to me too. I feel like an idiot to not think of creating a method like workflow!! \$\endgroup\$ – Jikku Jose Jan 16 '15 at 18:10
  • 1
    \$\begingroup\$ @JikkuJose, well, if we all didn't do stupid things at times, there wouldn't be any need for reviews, right? :) \$\endgroup\$ – akostadinov Jan 16 '15 at 18:14
  • \$\begingroup\$ Do you see anything other than that? \$\endgroup\$ – Jikku Jose Jan 16 '15 at 18:15
  • \$\begingroup\$ @JikkuJose, I didn't spot anything else. As code looks reasonable. \$\endgroup\$ – akostadinov Jan 19 '15 at 16:00

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.