1
\$\begingroup\$

I am new to Ruby and would like some help with re-factoring my code.

class Beam
def initialize 
    puts "Please specify span of beam"
    @span = gets.chomp.to_f
    puts "How many number of point loads do you want?"
    @pointLoadNos = gets.chomp.to_i
    @pointLoad = Array.new(@pointLoadNos)
    @pointLoadDistFromLeft = Array.new(@pointLoadNos)
    @pointLoadDistFromRight = Array.new(@pointLoadNos)
    @magnitude = Array.new(@pointLoadNos) 
    @reaction = Array.new(2,0)
end
def setValues
    count = 0
    while count < @pointLoadNos
        puts "At what dist should point load #{count+1} be placed from left?"
        @pointLoadDistFromLeft[count] = gets.chomp.to_f
        if @pointLoadDistFromLeft[count] > @span
            puts "Dist of Point Load #{count+1} From Left should be less than span length of beam"
        else    
            puts "Magnitude of point load #{count+1} should be?"
            @magnitude[count] = gets.chomp.to_f
            @pointLoadDistFromRight[count] = (@span - @pointLoadDistFromLeft[count])
            count += 1
        end
    end
end
def calReactions
    i = 0
    while i < @pointLoadNos
        @reaction[0] += (@pointLoadDistFromLeft[i]*@magnitude[i])/@span
        @reaction[1] += (@pointLoadDistFromRight[i]*@magnitude[i])/@span
        i += 1
    end
    puts "Reaction at Left: #{@reaction[0]}"
    puts "Reaction at Left: #{@reaction[1]}"
end
end

beam = Beam.new
beam.setValues
beam.calReactions
\$\endgroup\$
  • 3
    \$\begingroup\$ For future reference, please give some background/context on what your code's supposed to accomplish. Yes, we can glean it from reading the code, but a simple "This class models a beam (as in a bridge or building) and calculates the forces exerted at its ends by an arbitrary number of point loads. It prompts the user for input values." would help a lot. \$\endgroup\$ – Flambino Apr 29 '13 at 21:53
4
\$\begingroup\$

First of all, you should not get user input from inside your Beam class. This ties it to the console and makes automated testing more difficult. A layered architecture is more suitable :

# The Beam class is only concerned by business logic.
# Notice how the logic and workflow are clear and readable,
# and how easy you it would be to write tests for it.
# 
class Beam

  attr_reader :span

  def initialize( span )
    @span        = span.to_f
    @load_points = {}
  end

  def add_load_point( distance, magnitude )
    distance, magnitude = distance.to_f, magnitude.to_f
    if distance < 0 || distance > span || @load_points[distance]
      raise ArgumentError, 'invalid load point' 
    end
    @load_points[distance] = magnitude
  end 

  def reactions
    @load_points.inject( left: 0, right: 0 ) do |hash, (distance, magnitude)|
      hash[:left]  += distance          * magnitude / span
      hash[:right] += (span - distance) * magnitude / span
      hash
    end
  end

end


# this module is an interface between your class and the user
# via the console. Notice that this module (could be a class,
# does not matter) is only concerned about asking questions,
# adapting user input to feed the Beam class interface, and 
# formatting output.
# 
module BeamConsole

  # here we chose to move all questions to the user to methods,
  # and name these methods with an exclamation mark at the end
  #
  def self.invoke
    beam = Beam.new( span? )
    begin
      beam.add_load_point( distance?, magnitude? )
    rescue ArgumentError => e
      puts "Invalid parameters for load point. Please retry." ; retry   
    end while more? 
    format_reactions( beam.reactions )
  end

  private #=================================================================== 

  # adapted from @nakilon's answer, tail-recursive variant
  def self.prompt( msg )
    output = gets.strip!
    return output unless output.empty? 
    prompt( msg )
  end

  def self.span?
    prompt( 'Please specify span of beam' ).to_f
  end

  def self.more?
    answer = prompt( 'Do you want to add points ? (y/n)' )
    answer = prompt( 'please answer by y or n' ) unless answer =~ /^[y|n]$/
    answer == 'y'
  end

  def self.distance?
    prompt( 'At what distance should the point be placed from left?' ).to_f
  end

  def self.magnitude?
    prompt( 'Magnitude should be?' ).to_f
  end

  def self.format_reactions( reactions )
    puts "Reaction on the left is  : %.4f" % reactions[:left]
    puts "Reaction on the right is : %.4f" % reactions[:right]
  end
end

This may seem overkill, but separating concerns is the core of OOP. With this approach, you'll end up with a Beam class devoid of any responsability concerning user interaction, and that's a good thing : you want your class to perform business logic before anything else.

This way, your code will be more readable (no logic riddled with prompts), so more maintainable, but also more reusable. Imagine that one day you want to allow users to perform calculations from a web page : you could not reuse your Beam class if it is tied to the console, but you could if you separated responsibilities.

|improve this answer|||||
\$\endgroup\$
2
\$\begingroup\$

I would write something like:

class Beam
    def prompt msg
        # a bit kung-fu, sorry
        puts msg while gets.strip!.empty?
        yield $_
    end
    def initialize 
        @span = prompt "Please specify span of beam", &:to_f
        @pointLoadNos = prompt "How many number of point loads do you want?", &:to_i
        @pointLoad = []
        @pointLoadDistFromLeft = []
        @pointLoadDistFromRight = []
        @magnitude = []
    end
    def setValues
        @pointLoadNos.times do |count|
            while 0 > @pointLoadDistFromRight[count] = @span - @pointLoadDistFromLeft[count] =
                  prompt("At what dist should point load #{count + 1} be placed from left?", &:to_f)
                puts "Dist of Point Load #{count + 1} From Left should be less than span length of beam"
            end
            @magnitude[count] = prompt "Magnitude of point load #{count + 1} should be?", &:to_f
        end
    end
    def calReactions
        @reaction = [0, 0]
        @magnitude.zip(@pointLoadDistFromLeft, @pointLoadDistFromRight) do |mag, left, right|
            @reaction[0] += (left * mag) / @span
            @reaction[1] += (right * mag) / @span
        end
        puts "Reaction at Left: #{@reaction[0]}"
        puts "Reaction at Right: #{@reaction[1]}"
    end
end

beam = Beam.new
beam.setValues
beam.calReactions

See this and this - when you pass a block, you don't have to catch it into argument, but can just use Ruby's keyword yield. Also instead of describing a block as a proc, you can use &:method notation.

|improve this answer|||||
\$\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.