CODEWITHSUNDEEP
c#.netvisual-studioperformanceif-statement
NeedHelpSometimes
NeedHelpSometimes

Reputation: 41

How can I get rid of these else if statements and make the code look better?

I have this c# code

progressBar1.Increment(1);
if (progressBar1.Value > 5 && progressBar1.Value < 10)
{
  label1.Text = "Some text...";
  timer1.Stop();
  timer1.Start();
  timer1.Interval = 1000;
}
else if (progressBar1.Value > 10 && progressBar1.Value < 15)
{
  label1.Text = "Some text 2...";
  timer1.Interval = 250;
}
else if (progressBar1.Value > 15 && progressBar1.Value < 30)
{
  label1.Text = "Some text 3...";
}
else if (progressBar1.Value > 30 && progressBar1.Value < progressBar1.Maximum)
{
  label1.Text = "Some text 4...";
  timer1.Interval = 100;
}
else if (progressBar1.Value == progressBar1.Maximum)
{
  timer1.Stop();
  const string message = "Some cool popup message";
  const string caption = "Test";
  MessageBox.Show(message, caption, MessageBoxButtons.OK);
  Close();
}

I'm sure this code is bad, I'm not really a programmer and I just tried to create some fun application. How can I get rid of these long else if statements and replace it with something better? I've been looking for answers & tips but none of them helped me.

Upvotes: 1

Views: 157

Answers (6)

D Stanley
D Stanley

Reputation: 152566

Your code is not bad (other than a few potential gaps by using < instead of <=< etc. It's perfectly clear what the intent is, and if it works it;'s perfectly acceptable.

Don't fall for the trap that less code is better code. Look at some of the suggested "improvements". Is it obvious what they do, or do you have to spend time thinking about what the actual logic is. I've seen way too many nested ternary expressions that make by brain hurt trying to decipher then (let alone trying to debug part of the expression), when a nested if would do exactly the same thing and by much easier to understand, debug, and maintain.

You have 5 potential states and very little overlap (effect-wise) between them. I would say that's a perfectly acceptable use of 5 if statements. If they were all "equality" cases instead of ranges, theand each were just setting the same variable, then perhaps something fancier like a Dictionary could be useful, but that's not the case here.

Upvotes: 1

Surt
Surt

Reputation: 16099

Now you only have a few cases so i barely makes a difference. If you had a lot of cases a table lookup could make it a lot easier.

Pseudo code:

struct Case {
  int Min, Max; // inclusive
  String txt; // if not empty
  int TimerValue; // set if greater than zero.
  Func ToCall; // null or lampda
};

Case cases { 
  { 6, 9, "Text1", 1000, { timer1.Stop(); timer1.Start(); } }, 
  { 11, 14, "Text2, 250, {}},
  etc.
}
ASSERT(is_sorted(cases)); // so we don't mess up the order.

auto res = BinarySearch(value, cases);
if (!res) continue;
Execute(res);

This can save some work when there are many cases.

Upvotes: 0

ShiningLea
ShiningLea

Reputation: 162

Since you're a beginner, and what I'm going to show is kind of complex, I highly advise you to check out the ternary conditionnal operator, since that's what I'm going to show you here.

Here is a piece of code :

        Console.WriteLine(text);
        text = (!(value > 10 && value < 15)) ? "Some text..." : "";

        Console.WriteLine(text);
        text = (!(value > 15 && value < 30)) ? "Some text 2..." : "";

        Console.WriteLine(text);
        text = (!(value > 30 && value < maximum)) ? "Some text 3..." : "";

        Console.WriteLine(text);
        text = (!(value == maximum)) ? "Some text 4..." : "";

        Console.WriteLine(text);
        text = "Some cool message";

        Console.WriteLine(text);

Now, don't be scared about this, I'll explain it.

So these one-liners are ternary conditions. They are basically regular if/else conditions, but as one line. They are very nice to make your code cleaner, but also to give it a "fancy" look at it.

You should note that I was not using a WinForms project, but rather a Console one, for easier tests.

As you can see, there isn't the first condition, which is value > 5 && value < 10. That's simply because we don't need it.

You might also be confused by the end. As you can see, the final message is not on a condition, it's after the value == maximum condition. Why? Well, that's just because how it works. If you don't understand a single bit of what I said, you should really read the article I sent above.

Anyways, here's the legend :

value is the ProgressBar's value, so here progressBar1.Value.

maximum is the ProgressBar's maximum value, so here progressBar1.Maximum.

Upvotes: 0

Matthew Watson
Matthew Watson

Reputation: 109597

In C#9 (which is currently only available in preview) you will be able to use relational pattern matching like so:

progressBar1.Increment(1);

switch (progressBar1.Value)
{
    case > 5 and < 10:
        label1.Text = "Some text...";
        timer1.Stop();
        timer1.Start();
        timer1.Interval = 1000;
        break;
        
    case > 10 and < 15:
        label1.Text     = "Some text 2...";
        timer1.Interval = 250;
        break;
        
    case > 15 and < 30:
        label1.Text = "Some text 3...";
        break;
        
    case > 30 and < progressBar1.Maximum:
        label1.Text     = "Some text 4...";
        timer1.Interval = 100;
        break;
        
    case progressBar1.Maximum:
        timer1.Stop();
        const string message = "Some cool popup message";
        const string caption = "Test";
        MessageBox.Show(message, caption, MessageBoxButtons.OK);
        Close();
        break;
}

This makes it much clearer how the ranges are subdivided. (C# 9 should be released around about November this year, as far as I know.)

Upvotes: 1

Athanasios Kataras
Athanasios Kataras

Reputation: 26372

Two ways I can think of:

Simplify the conditions and change the flow with continue I'm assuming here that you are in a for or while loop

progressBar1.Increment(1);
if (progressBar1.Value < 5) continue;
if (progressBar1.Value < 10)
{
  label1.Text = "Some text...";
  timer1.Stop();
  timer1.Start();
  timer1.Interval = 1000;
  continue;
}
if (progressBar1.Value < 15)
{
  label1.Text = "Some text 2...";
  timer1.Interval = 250;
  continue;
}
if (progressBar1.Value < 30)
{
  label1.Text = "Some text 3...";
  continue;
}
if (progressBar1.Value < progressBar1.Maximum)
{
  label1.Text = "Some text 4...";
  timer1.Interval = 100;
  continue;
}

  timer1.Stop();
  const string message = "Some cool popup message";
  const string caption = "Test";
  MessageBox.Show(message, caption, MessageBoxButtons.OK);
  Close();

If not in a loop, remove the continue and re-add the else statements and the maximum condition if. The code will only enter one of the if/else cases.

For further study, you could extract to a function, but I would need your whole example to refactor it.

Even further study could lead you to the state pattern, which would suit your case, but it's overkill.

Upvotes: 2

Saumye Navarathna
Saumye Navarathna

Reputation: 32

Here is some example you can look for.

switch (value)
{
     case var expression when value < 0:
         //some code
         break; 

     case var expression when (value >= 0 && value < 5):
         //some code
         break;

     default:
         //some code
         break;
}

Upvotes: -1

Related Questions