Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have this working code. I think this must be easier with a while loop.

if not c.e1.nil?
  extrapreis = number_to_currency Extra.find(c.e1).aufschlag, :unit=>"EUR "
  array.push Extra.find(c.e1).name + " (" + extrapreis.to_s + ")"
  extpreis = extpreis + Extra.find(c.e1).aufschlag
end
if not c.e2.nil?
  extrapreis = number_to_currency Extra.find(c.e2).aufschlag, :unit=>"EUR "
  array.push Extra.find(c.e2).name + " (" + extrapreis.to_s + ")"
  extpreis = extpreis + Extra.find(c.e2).aufschlag
end
if not c.e3.nil?
  extrapreis = number_to_currency Extra.find(c.e3).aufschlag, :unit=>"EUR "
  array.push Extra.find(c.e3).name + " (" + extrapreis.to_s + ")"
  extpreis = extpreis + Extra.find(c.e3).aufschlag
end
if not c.e4.nil?
  extrapreis = number_to_currency Extra.find(c.e4).aufschlag, :unit=>"EUR "
  array.push Extra.find(c.e4).name + " (" + extrapreis.to_s + ")"
  extpreis = extpreis + Extra.find(c.e4).aufschlag
end
if not c.e5.nil?
  extrapreis = number_to_currency Extra.find(c.e5).aufschlag, :unit=>"EUR "
  array.push Extra.find(c.e5).name + " (" + extrapreis.to_s + ")"
  extpreis = extpreis + Extra.find(c.e5).aufschlag          
end

It is always the same code, except for the variable e (1, 2, 3, 4, 5). How can I put this in a while loop and put the counter together with e?

share|improve this question

migrated from stackoverflow.com Jan 7 '13 at 15:31

This question came from our site for professional and enthusiast programmers.

3 Answers 3

up vote 1 down vote accepted
(1..5).each do |i|
  e = c.send("e#{i}")
  next if e.nil?
  found = Extra.find(e)
  array.push("#{found.name} (#{number_to_currency(found.aufschlag, unit: "EUR ")})")
  extpreis += found.aufschlag
end

If you can be certain that c.e1 etc. never becomes false, then:

(1..5).each do |i|
  next unless e = c.send("e#{i}")
  found = Extra.find(e)
  array.push("#{found.name} (#{number_to_currency(found.aufschlag, unit: "EUR ")})")
  extpreis += found.aufschlag
end
share|improve this answer
    
Thank you very much. So I learn a little more about rails ;) –  user1953483 Jan 6 '13 at 21:40
    
It is not Rails. It's all plain Ruby. Actually, I barely know anything about Rails. –  sawa Jan 6 '13 at 21:42

I'd start with this untested code:

[c.e1, c.e2, c.e3, c.e4, c.e5].each do |e|
  if e
    extrapreis = number_to_currency(Extra.find(e).aufschlag, unit: 'EUR ')
    array << Extra.find(e).name + ' (' + extrapreis.to_s + ')'
    extpreis += Extra.find(e).aufschlag
  end
end
share|improve this answer
    
Maybe unless instead of if !? –  Andy H Jan 6 '13 at 21:17
    
I don't like unless, especially at the start of a conditional block. It's one of those programmer-choice, often argued, syntax things. –  the Tin Man Jan 6 '13 at 21:20
    
Fair enough - in fact this article would advocate a simple if e instead :) –  Andy H Jan 6 '13 at 21:24
    
That's true, if e is how I'd normally write it if I was doing a refactor or review. :-) I'll adjust it. Thanks. –  the Tin Man Jan 6 '13 at 21:36
1  
You'd probably like Ruby Best Practices –  the Tin Man Jan 6 '13 at 21:39

Notes on your code:

  • Use of parentheses: Ruby allows you to drop them, but the community seems to have reached a consensus it may harm readability, restrict its usage to DSL-style code.

  • extrapreis: this may be "private" code (well, it's not anymore since you publicly asked about it), but it's good practice to use the commonly accepted language in the programming world (for script names, variable names, comments, everything).

  • array.push, x = x + y, ...: you may use this but being aware about functional programming and why inplace updates are considered (by some) bad practice. More on this issue.

  • if not c.e1.nil? -> if c.e1.

  • Extra.find(c.e1).aufschlag. In each block you call this twice, why? use a variable to store the value.


For a novice programmer a functional approach may be a little difficult to grasp at first, but it introduces important concepts that will (hopefully) prove useful later on:

strings, values = [c.e1, c.e2, c.e3, c.e4].compact.map do |e|
  value = Extra.find(e).aufschlag
  extra_price = number_to_currency(value, :unit => "EUR")
  name = Extra.find(e).name 
  ["#{name} (#{extra_price})", value]
end.transpose
extra_price_total = values.inject(0, :+)
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.