C# nested if/else in a for loop

Question

I have a nested if/else statement in a for loop to determine whether or a a value is valid with an array value. It returns all values just fine, however if the IF is correct, it still does the else an additional three times. I thought once it was equal one time, it would stop, but I suppose I am missing something here.

string sectionChoice;
int ticketQuantity;
double ticketPrice, totalCost;
string[] section = { "orchestra", "mezzanine", "balcony", "general" };
double[] price = { 125.25, 62.00, 35.75, 55.50 };
bool isValidSection = false;

sectionChoice = GetSection();
ticketQuantity = GetQuantity();

for (int x = 0; x < section.Length; ++x)
{
    if (sectionChoice == section[x])
    {
        isValidSection = true;
        ticketPrice = price[x];

        totalCost = CalcTicketCost(ticketPrice, ticketQuantity);
        Console.Write("\n\nTotal cost for the tickets are: {0:c2}", totalCost);
    }
    else
        Console.Write("\n\nInvalid entry, {0} does not exist", sectionChoice);
}

When it is valid, it returns something like this:

Your price is 32.50. Invalid entry, x does not exist Invalid entry, x does not exist Invalid entry, x does not exist

You should be using `decimal` for currency values ([read more](http://stackoverflow.com/q/693372/188246)). — David Sherret, Jun 18 '15 at 18:22

Servy · Answer 1 · 2015-06-18T18:26:20.067

4

What you're really trying to do is determine if section contains a particular value, and do something if it does. Just check that directly:

if (section.Contains(sectionChoice))

You also shouldn't be using parallel arrays. Rather than having two arrays, sections an prices, in which the object at the index of each both "combine" to equal a value, it appears that what you're actually modeling is a means to lookup the price of a particular section. This is best modeled with a Dictionary that can easily look up the value for a particular key. Here your key is the section, and the value is its price.

Dictionary<string, decimal> ticketsPrices = new Dictionary<string, decimal>()
{
    {"orchestra", 125.25m},
    //...
};
bool isValidSection = false;

string sectionChoice = GetSection();
int ticketQuantity = GetQuantity();

if (ticketsPrices.ContainsKey(sectionChoice))
{
    isValidSection = true;
    decimal ticketPrice = ticketsPrices[sectionChoice];

    decimal totalCost = CalcTicketCost(ticketPrice, ticketQuantity);
    Console.Write("\n\nTotal cost for the tickets are: {0:c2}", totalCost);
}
else
    Console.Write("\n\nInvalid entry, {0} does not exsist", sectionChoice);

edited Jun 18 '15 at 18:26

answered Jun 18 '15 at 18:14

Servy

202,030
26
332
449

This answer surpasses the others by identifying the intent and solving the problem a better way. – adv12 Jun 18 '15 at 18:16
What happens with `x`? You still need the index to get the price, but this is the right idea (or a dictionary could be used with Dictionary) – David Sherret Jun 18 '15 at 18:16
@DavidSherret, good point. Didn't see that initially. Could be solved by using `Array.IndexOf` and would still be cleaner than the other answers. – adv12 Jun 18 '15 at 18:17
1

@DavidSherret yeah, the design was even more flawed than I noticed at first glance. This is why you avoid parallel arrays; it creates a giant mess. – Servy Jun 18 '15 at 18:27
@adv12 The solution is to get rid of the parallel arrays, rather than trying to work around them. They'll only continue to act up down the road. – Servy Jun 18 '15 at 18:27

DotNetRussell · Accepted Answer · 2015-06-18T18:14:38.223

1

The keyword you are looking for is break;

break will stop the execution of the loop it is inside. If you are inside nested loops it will only work on the innermost.

The opposite of this is continue. Continue stops that iteration and moves onto the next.

Here is an MSDN article explaining it more in depth

for (int x = 0; x < section.Length; ++x)
{
   if (sectionChoice == section[x])
   {
      isValidSection = true;
      ticketPrice = price[x];

      totalCost = CalcTicketCost(ticketPrice, ticketQuantity);
      Console.Write("\n\nTotal cost for the tickets are: {0:c2}", totalCost);
      break;
   }
   else
     Console.Write("\n\nInvalid entry, {0} does not exsist", sectionChoice);
 }

edited Jun 18 '15 at 18:14

answered Jun 18 '15 at 18:11

DotNetRussell

9,716
10
56
111

1

Doh! Thank you kindly :) – Podo Jun 18 '15 at 18:13
@JeffreyDilley please don't forget to select this as an answer by clicking the checkmark if this worked for you. – DotNetRussell Jun 18 '15 at 18:18
1

Absolutely. It says I have to wait a few minutes, but I most certainly will. – Podo Jun 18 '15 at 18:21
1

You still might get the invalid entry message several times, if the selected section is not the first! – Olivier Jacot-Descombes Jun 18 '15 at 18:41
@OlivierJacot-Descombes the question was how to break a loop. Not to fix the logic behind it. Let him learn it through trial and error. – DotNetRussell Jun 18 '15 at 20:54

Olivier Jacot-Descombes · Answer 3 · 2015-06-18T18:43:33.500

Get the index of the chosen item like this:

int i = section.IndexOf(sectionChoice);
if (i >= 0) {
    ticketPrice = price[i];
} else {
    // Not found!
}

However your code would become more flexible with an item class

public class Section
{
    public string Name { get; set; }
    public decimal Price { get; set; }
}

With this class you can create a list of sections like this

List<Item> sections = new List<item> {
    new Section { Name = "orchestra", Price = 125.25 },
    new Section { Name = "mezzanine", Price = 62.00 },
    ...
};

Now you can find a section like this:

Section section = sections.FirstOrDefault(s => s.Name == sectionChoice);
if (section != null ) ...

Like this it is easier to add new properties to the sections. E.g. you could add a number of remaining seats to it, without having to create a new array.

score 0 · Answer 4 · edited Jun 18 '15 at 20:08

0

If you want iteration to stop when the if condition is met, you need to break out of the loop with a break statement:

for (int x = 0; x < section.Length; ++x)
{

    if (sectionChoice == section[x])
    {
        isValidSection = true;
        ticketPrice = price[x];

        totalCost = CalcTicketCost(ticketPrice, ticketQuantity);
        Console.Write("\n\nTotal cost for the tickets are: {0:c2}", totalCost);
        break;  // THIS IS THE IMPORTANT CHANGE
    }

}
if (!isValidSection) // TEST MOVED OUTSIDE OF LOOP
{
    Console.Write("\n\nInvalid entry, {0} does not exsist", sectionChoice);
}

edited Jun 18 '15 at 20:08

Olivier Jacot-Descombes

104,806
13
138
188

answered Jun 18 '15 at 18:12

adv12

8,443
2
24
48

You still might get the invalid entry message several times, if the selected section is not the first! – Olivier Jacot-Descombes Jun 18 '15 at 18:52

score 0 · Answer 5 · answered Jun 18 '15 at 18:23

0

You don't want to report that it's an invalid choice for each one that it doesn't match:

for (int x = 0; x < section.Length; ++x)
{
    if (sectionChoice == section[x])
    {
        isValidSection = true;
        ticketPrice = price[x];

        totalCost = CalcTicketCost(ticketPrice, ticketQuantity);
        Console.Write("\n\nTotal cost for the tickets are: {0:c2}", totalCost);
        break;
     }
}

if (!isValidSection)
    Console.Write("\n\nInvalid entry, {0} does not exist", sectionChoice);

answered Jun 18 '15 at 18:23

Mike

435
2
7

Could I request a reason for the down votes? This is only my third post. I'm not sure where it's lacking. – Mike Jun 18 '15 at 18:39
In fact, the invalid entry message must be placed **after** the loop, otherwise it still might be written to the console several times! – Olivier Jacot-Descombes Jun 18 '15 at 18:46

C# nested if/else in a for loop

5 Answers5