6
\$\begingroup\$

Content of task:

Given a string of words and numbers. Extract the expression including:

  1. the operator: either addition or subtraction
  2. the two numbers that we are operating on

Return the result of the calculation.

Example:

"Panda has 48 apples and loses 4" returns 44

"Jerry has 34 apples and gains 6" returns 40

"loses" and "gains" are the only two words describing operators.

Should be a nice little kata for you :)

Note: No fruit debts nor bitten apples = The numbers are integers and no negatives

source: codewars.com

I want to optimise so ugly code:

def calculate(string)
  operator = '-' if string.split(' ').include? 'loses'
  operator = '+' if string.split(' ').include? 'gains'
  sum = 0
  count = 0
  n1 = 0
  n2 = 0
  string.split(' ').map do |s|
    if s.to_i != 0 && s.to_i.is_a?(Numeric)
      if count == 0
        n1 = s.to_i
        count += 1
      else
        n2 = s.to_i
      end
    end
  end
  n1.method(operator).(n2)
end

This code nice works(all tests was passed). But I don't want a large number of variables. Maybe have you some advice how to optimise this code? Thank you in advance.

| improve this question | | | | |
\$\endgroup\$
7
\$\begingroup\$

The problem is that you are manipulating text without using Regular Expressions.

Regular expressions are a tool made specifically for text manipulation (search, delete, replace ... ) and are extremely good at their job.

The over-complication you experience comes from the use loops, conditionals and arithmetic to manipulate strings.

Here is a solution using regular expressions:

def text_calculation(text)
  a, b = text.split(/[^0-9]/).reject(&:empty?).map(&:to_i)
  text.include?("gains") ? a + b : a - b
end

First line explanation

text.split(/[^0-9]/)

This splits the text at every character that is not a digit (^ negates and [0-9] means from 0 to 9.

The output is this list:

["", "", "", "", "", "", "", "", "", "", "48", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "4"]

Almost what we want, we just need to remove (reject) the empty strings (empty?):

.reject(&:empty?)

So now we have ["48", "4"] that we must convert to integers before we can do arithmetic on, map applies a function to each item of a list and to_i converts to integer so .map(&:to_i)

Second line explanation

We add if we find "gains" else subtract (condition ? if_true : if_false is the Ternary operator)

.scan

We need to find the parts of the string that match a given regex, given that the regex /\d+\ matches all the subsequent strings of digits.

We must "find all regex matches in ruby", searching Google for such a task lands us on StackOverflow reveals that .scan is exactly what we can use. (The second Stack Overflow example is even about searching digits in a string!)

def text_calculation(text)
  a, b = text.scan(/\d+/).map(&:to_i)
  text.include?("gains") ? a + b : a - b
end

This code is more direct than splitting and filtering.

| improve this answer | | | | |
\$\endgroup\$
  • \$\begingroup\$ Why are you using code that does the same thing as scan but takes more resources and is less clear? Also, why the ternary? There's no point in it and it just makes it more difficult to parse. \$\endgroup\$ – Fund Monica's Lawsuit Aug 29 '16 at 20:22
  • \$\begingroup\$ @QPaysTaxes 1 becuase it was a previous iteration and I found scan afterwards. 2 the ternary is exactly as clear the if expression, they are conceptually identical so it makes no difference which one to use. \$\endgroup\$ – Caridorc Aug 29 '16 at 22:16
  • 1
    \$\begingroup\$ Fair enough. I'd prefer the explicit if because it's easier for me to read, but if you read symbols easier, fine. \$\endgroup\$ – Fund Monica's Lawsuit Aug 29 '16 at 22:17

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.