ruby
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25
# Simple method to calculate the square of a number def square(number) return number**2 end # Variable to be assigned the biggest square value biggest_square = 0 # Iterate over a list of base numbers and compare their squared values to biggest_square, and replace if bigger. [1,4,3,5].each do |base| # Compare values if square(base) > biggest_square # Assign new value biggest_square = square(base) end end # Alternative [1,4,3,5].each do |base| temp = square(base) if temp > biggest_square biggest_square = temp end end
Refactorings
No refactoring yet !
Amos King
September 21, 2008, September 21, 2008 15:11, permalink
Don't forget you Enumberable functions. Use max.
1 2 3 4 5 6 7 8 9 10 11
# Simple method to calculate the square of a number def square(number) return number**2 end # Variable to be assigned the biggest square value biggest_square = 0 [1,4,3,5].each do |base| biggest _square = [biggest_square, square(base)].max end
Tien Dung
September 21, 2008, September 21, 2008 15:43, permalink
I prefer using temp variable for PERFORMANCE. For me, the # Alternative code is already clean and clear.
If your concern is lines of code. Here is one line assignment using temp variable refactoring.
1 2 3 4 5 6 7 8 9
def square(number) return number**2 end biggest_square = 0 [1,4,3,5].each do |base| (temp = square(base)) > biggest_square && biggest_square = temp end
Tien Dung
September 21, 2008, September 21, 2008 15:58, permalink
Refactor Amos King .max function version
1 2 3 4 5
def square(number) return number**2 end biggest_square = [1,4,3,5].map{|base| square(base)}.max
ruphin.myopenid.com
September 21, 2008, September 21, 2008 15:58, permalink
Thanks, I was already looking into something like putting them both in an array and taking the highest value, but it seemed like too much hassle to me. That last one also looked good, I'll probably put it in some more lines for readability though. Thanks.
Tien Dung
September 21, 2008, September 21, 2008 16:19, permalink
Don't know who vote 1 star for my "one line assignment using temp variable refactoring". Please tell me why you think it's bad. As I said the original code is perfect for me. That refactoring is in case your concern is lines of code.
danielharan
September 21, 2008, September 21, 2008 16:23, permalink
Tien Dung's last function is crystal clear. If you have a function that's as predictable as square though, you can do:
1 2 3 4 5
def square(number) return number**2 end biggest_square = square([1,4,3,5].max)
ruphin.myopenid.com
September 21, 2008, September 21, 2008 19:10, permalink
Don't know who downvoted, I gave a 5, I hadn't thought of && myself.
danielharan
September 21, 2008, September 21, 2008 19:38, permalink
Tien - I voted 5 for your 2nd refactoring, 1 for the 1st.
The 1st is bad because 1 - it's still procedural and 2- it's hard to read. The '&&' operator is generally only used for boolean operations rather than chaining assignment or other operations with side-effects. Consider the three statements for which you find more readable:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16
biggest_square = 0 [1,4,3,5].each do |base| (temp = square(base)) > biggest_square && biggest_square = temp end # Using a loop w/ temp variable to avoid [].max sorting through the whole array # (yes, I would leave the above line as a comment # explaining *why* Enumerable methods weren't used) biggest_square, temp = 0 [1,4,3,5].each do |base| biggest_square = temp if (temp = square(base)) > biggest_square end # Procedural style, split for clarity, self-commenting squares = [1,4,3,5].collect {|e| square(e) } biggest_square = squares.max
Tien Dung
September 21, 2008, September 21, 2008 22:31, permalink
Thanks Daniel. I found the last one is more readable and self-commenting. Nice refactoring.
Maciej Piechotka
September 21, 2008, September 21, 2008 23:45, permalink
@danielharan:
Well - -100 < 10 but (-100)** > 10**2 as 10000 > 100.
@Tien Dung:
Sorry for voting 1 on the second refactoring. I meant 5 but clicked in wrong place. Do anybody knowns how to change it?
danielharan
September 21, 2008, September 21, 2008 23:53, permalink
Maciej, nice catch, that doesn't handle negatives.
I'll ask Marc-André to add a feature: the ability to change ratings!
Tien Dung
September 22, 2008, September 22, 2008 00:01, permalink
Hi Daniel, can you ask Marc-Andre for comment on rating also. For me the reason or thinking behind a refactoring or a rating is more important than the code or the rating itself.
Tien Dung
September 22, 2008, September 22, 2008 01:27, permalink
temp variable based version using inject function.
1 2 3 4 5
def square(number) return number**2 end biggest_square = [1,4,3,5].inject(0){ |max, base| (temp = square(base)) > max ? temp : max }
danielharan
September 22, 2008, September 22, 2008 16:34, permalink
I sent MA a link to your request :)
Inject is a good choice here.
1
biggest_square = [1,4,3,5].inject(0){ |memo, i| [memo, square(i)].max }
This is a tiny bit of code I wrote just to display a general case. What I want to do is replace the value in biggest_square if another value is bigger. the problem is that this way, it will calculate square(base) twice for every base, one to compare and one to assign. An alternative is to assign the value to a temporary variable, but that's not really clean either.
Is there a way to do both the comparison and assignment in one?