StackOverflow2013

Note that there are some explanatory texts on larger screens.

plurals

PO
text
Body
copied!<p>Here's one way you could fix your code:</p> <pre><code>a = [435,276,434] def product(a) result = [] # Create an empty array that will become [60, 85, 48] for e in a final = 1 # Convert the integer e to a string (e.g., "435") str = e.to_s # Iterate over each char of the string (e.g., "4", "3" and "5") str.each_char do |c| # Convert the character 'c' to an integer (digit) then multiply final by that integer final *= c.to_i end # Append the value of final to the result array result << final # e.g., when result = [60], result << 85 => [60, 85] end result # => [60, 85, 48] end product(a) # => [60, 85, 48] </code></pre> <p>Now let's see how we can improve it. Firstly, we can chain operations and avoid the use of the temporary variable <code>str</code>. Also, you'll find that for loops, <code>each</code> is generally preferable to <code>for</code> (especially because you can use a block with <code>each</code>), so I'll change that too. While I'm at it, since the <code>each_char</code> loop contains only one statement, I'll write the block with brackets rather than <code>do/end</code>. We now have:</p> <pre><code>def product(a) result = [] # Create an empty array that will become [60, 85, 48] a.each do |e| final = 1 e.to_s.each_char {|c| final *= c.to_i} result << final end result end </code></pre> <p>When I look at this, I'm thinking I want to convert each element of the array <code>a</code> to something else (the product of its digits). That suggests the use of the <code>Array</code> method <code>map!</code> (or its synonym, <code>collect!</code>), rather than <code>each</code>. Since <code>a</code> is the argument of the method <code>product</code>, if I use <code>a.map!</code>, that will change the values of <code>a</code> in the method that calls <code>product</code>. That may or may not be OK, but since I'm returning an array of the computed values, it's probably not OK, so I'll apply <code>map!</code> to a copy of <code>a</code>. (It's called a "shallow" copy, which is not important here, but can be in other situations.) We now have this:</p> <pre><code>def product(a) result = a.dup result.map! do |e| final = 1 e.to_s.each_char {|c| final *= c.to_i} final end result end </code></pre> <p>We don't need the last <code>result</code>, because <code>map!</code> returns <code>result</code> (as well as changing <code>result</code>). Hey, that also means we can use just <code>map</code> (makes no difference). Also, we can chain <code>a.dup</code> and <code>map</code> to get rid of <code>result</code>:</p> <pre><code>def product(a) a.dup.map do |e| final = 1 e.to_s.each_char {|c| final *= c.to_i} final end end </code></pre> <p>Man, we're cookin' with gas! Next, whenever you see a block that computes the product or sum of something, think <code>inject</code> (or its synomym, <code>reduce</code>): </p> <pre><code>def product(a) a.dup.map do |e| e.to_s.each_char.inject(1) {|final, c| final * c.to_i} end end </code></pre> <p>Suppose <code>a</code> < 0. What to do? (@Leon's answer twigged me to that possibility.) A negative receiver makes no sense to <code>each</code>, so let's raise an exception if that happens:</p> <pre><code>def product(a) raise RuntimeError, "Receiver must be non-negative" if self < 0 a.dup.map do |e| e.to_s.each_char.inject(1) {|final, c| final * c.to_i} end end </code></pre> <p>You may want to stop here, but you could replace <code>map</code> with another `inject:</p> <pre><code>def product(a) raise RuntimeError, "Receiver must be non-negative" if self < 0 a.inject([]) {|result, e| result.concat.e.to_s.each_char.inject(1) {|final, c| final * c.to_i}} end </code></pre> <p>Here the argument for inject is an empty array called <code>result</code>. Notice that, since we haven't changed <code>a</code>, we no longer needed <code>dup</code>. (I see @Kingston arrived at a similar answer.) If you prefer, you could write this as:</p> <pre><code>def product(a) raise RuntimeError, "Receiver must be non-negative" if self < 0 a.inject([]) {|result, e| result << e.to_s.each_char.inject(1) {|final, c| final * c.to_i}; result} end </code></pre> <p>but notice the need for that pesky <code>; result</code> an the end.</p> <p>You might think that the end result of all these "improvements" is too much of a mind-bender, or that the author is just showing off. That's what I thought when I was new to Ruby. With experience, however, you will find it is very natural and reads like a sentence. It also makes debugging easier: working left-to-right, you can test each link of the chain to make sure it's working.</p>

Querying!

Guidance

An individual column

Larger individual text columns get their own page to allow for proper reading.

SQuiL has stopped working due to an internal error.

If you are curious you may find further information in the browser console, which is accessible through the devtools (F12).

Reload