Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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
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
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
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
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          

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 Jan 7 '13 at 15:31

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

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

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("#{} (#{number_to_currency(found.aufschlag, unit: "EUR ")})")
  extpreis += found.aufschlag
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
share|improve this answer
Maybe unless instead of if !? – Andrew Haines 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 :) – Andrew Haines 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
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] do |e|
  value = Extra.find(e).aufschlag
  extra_price = number_to_currency(value, :unit => "EUR")
  name = Extra.find(e).name 
  ["#{name} (#{extra_price})", value]
extra_price_total = values.inject(0, :+)
share|improve this answer

Your Answer


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