3
\$\begingroup\$

It only does expressions with 2 operands yet, but I'm wondering if there are any ways I can improve this:

# infix.rb: parse infix-operated math expressions

class String
  def is_number?
    true if Float(self) rescue false
  end
end

class InfixParser
  @operations = [ '/', '*', '+', '-' ]

  testString = '(24 + 6) / 10 * 3'
  shouldEqual = 9

  def parseExpression(expr) #TODO: implement parsing of entire expressions

  end

  def self.parseChunk(chunk)
    chunk = chunk.gsub ' ', ''

    if not (chunk.include?('/') or chunk.include?('*') \
        or chunk.include?('+') or chunk.include?('-'))
        puts "error: no operations in chunk: #{chunk}"
        exit
    end

    firstNumber = ''
    secondNumber = ''

    i = 0
    currentChar = ''
    while not @operations.include? currentChar
      currentChar = chunk[i]
      if not currentChar.is_number? and not @operations.include? currentChar \
         and currentChar != '.'
        puts "error: non-numerical digit in chunk: #{currentChar}"
        exit
      end
      firstNumber += currentChar
      i += 1
    end
    firstNumber = firstNumber[0 .. -2]

    for c in i-1 .. chunk.length-1
      if not chunk[c].is_number? and not @operations.include? currentChar \
         and currentChar != '.'
        puts "error: non-numerical digit in chunk: #{currentChar}"
        exit
      end
      secondNumber += chunk[c]
    end
    secondNumber = secondNumber[1 .. secondNumber.length - 1]

    if chunk[i - 1] == '/'
      return Float(firstNumber) / Float(secondNumber)
    elsif chunk[i - 1] == '*'
      return Float(firstNumber) * Float(secondNumber)
    elsif chunk[i - 1] == '+'
      return Float(firstNumber) + Float(secondNumber)
    elsif chunk[i - 1] == '-'
      return Float(firstNumber) - Float(secondNumber)
    else
      puts "error: invalid operator in chunk: #{chunk}"
    end

  end

end
\$\endgroup\$
2
\$\begingroup\$

You've indicated that you prefer not to use regular expressions. In that case, you're still working too hard. In that case, you should look for a library function that does the job, such as the standard scanf library.

require 'scanf'

def eval_binary_expr(expr)
  l_operand, op, r_operand = expr.scanf('%f %c %f')
  if l_operand.nil?
    raise ArgumentError, "Missing or invalid left operand"
  end

  begin
    case op
      when '/' then l_operand / r_operand
      when '*' then l_operand * r_operand
      when '+' then l_operand + r_operand
      when '-' then l_operand - r_operand
      else raise ArgumentError, "Missing or invalid operator"
    end
  rescue TypeError
    raise ArgumentError, "Missing or invalid right operand"
  end
end

This implementation is better than the original, in that it can handle negative operands and explicitly positive operands. It also examines the string from left to right, which makes it easier to understand its behaviour.

\$\endgroup\$
  • 1
    \$\begingroup\$ thank you. I'm new to ruby, so learning more about things in the standard library is helpful to me. \$\endgroup\$ – joshhartigan Nov 3 '14 at 18:39
2
\$\begingroup\$

Your parseChunk function is not really parsing an expression — it is also evaluating it. Its naming style is also inconsistent with is_number?. I think that eval_binary_expr would be a more appropriate name.

Augmenting the String class with an is_number? method is something to avoid doing. That kind of patching increases the chances of breakage when your code coexists with other code in the same program.

In functions that are meant to be used by other code, calling exit is not an appropriate way to handle errors. You should raise exceptions instead, so that the caller can handle the error in whatever way it chooses.

You appear to be working much too hard to parse the binary expression. I think that this would accomplish more or less the same thing:

def eval_binary_expr(expr)
  l_operand, op, r_operand = expr.partition(%r{[/*+-]})

  if op.empty?
    raise ArgumentError, "Invalid operation or no operation in expression: #{expr}"
  end
  l_operand, r_operand = Float(l_operand), Float(r_operand)

  case op
    when '/' then l_operand / r_operand
    when '*' then l_operand * r_operand
    when '+' then l_operand + r_operand
    when '-' then l_operand - r_operand
  end
end

The solution above makes no attempt to handle negative numbers correctly. As with your original solution, it fails when the first operand is negative.

\$\endgroup\$
  • \$\begingroup\$ This is all helpful , except I don't want to use regexes - is my method too long even still? \$\endgroup\$ – joshhartigan Nov 3 '14 at 17:55
  • 1
    \$\begingroup\$ I've posted a second answer to address that. \$\endgroup\$ – 200_success Nov 3 '14 at 18:38

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.