C# IF statement not breaking correctly

Question

Been banging my head against the wall and Google try to find the answer to my problem.

When the below IF statement executes is appears to be running completely through the IF and ELSE statements.

  if (IsPostBack)
         {
             Boolean bFileOK = false;

             if (fulReagentImg.HasFile)
             {
                 String sFileExtension = System.IO.Path.GetExtension(fulReagentImg.FileName).ToLower();
                 String sFileExtensionLabel = sFileExtension;
                 lblFileExtension.Text = sFileExtensionLabel;
                 String[] allowedExtensions = { ".gif", ".png", ".jpeg", ".jpg" };
                 for (int i = 0; i < allowedExtensions.Length; i++)
                 {
                     if (sFileExtension == allowedExtensions[i])
                     {
                         bFileOK = true;
                     }
                     else
                     {
                         lblException.Text = "Can only upload .gif, .png, .jpeg, .jpg";
                         lblException.CssClass = "red";
                     }

 }

Any ideas why it's not stopping with bFileOK = true?

Ever heard of "String.CompareTo()"? Or "String.Contains()"? I would *discourage* using "=="; I would definitely add a "break". http://stackoverflow.com/questions/9089716/c-what-is-the-difference-between-comparetostring-and-equalsstring — paulsm4, Sep 14 '12 at 20:12
I would likely use a `List` (not an array) and `if (allowedExtensions.Contains(sFileExtension)) { /* good */ } else { /* bad */ }`. Make sure to normalize the case first. — , Sep 14 '12 at 20:13
It really fascinates me to see how people think code should work and how it actually works. Makes me wonder about the different mental models people develop. — ChaosPandion, Sep 14 '12 at 20:15

SLaks · Answer 1 · 2012-09-14T20:13:04.743

5

Your loop is completely wrong.
If the user's extension isn't equal to all of the extensions in your list, it will show the error.

You should call the Contains() method, preferably of a HashSet<String>.

edited Sep 14 '12 at 20:13

answered Sep 14 '12 at 20:12

SLaks

868,454
176
1,908
1,964

1

Why do you feel `HashSet` is appropriate? – ChaosPandion Sep 14 '12 at 20:17
I would use a Contains-approach too. However, for such a small n .. it's splitting hairs at best here. – Sep 14 '12 at 20:18
3

@ChaosPandion: Because it's O(1). – SLaks Sep 14 '12 at 20:18
@SLaks - I know the theoretical reason to use this structure so what I really should of asked is why use it for such a small n? As an industry should we always break out a hash table for look ups? When can we fall back to a linear search? – ChaosPandion Sep 14 '12 at 20:25
@ChaosPandion 1) it's a good habit to have 2) You can't tell which at which n the HashSet really starts to outperform the Array without testing. I have only seen HashSet >= Array performances, never HashSet < Array performance when doing a lookup. The performance boost can be very large even for a small n, from [personal experience](http://stackoverflow.com/questions/5261858) I needed to filter invalid chars from a StreamReader before they where handed off to a XmlReader. I had **4** chars to test, but changing from Array to HashSet changed my run time from 35 sec to 4 seconds. – Scott Chamberlain Sep 14 '12 at 21:04
@ScottChamberlain - There are definitely multiple factors to consider. I pretty much always use it as a matter of course but I always like to think about why we do things. I think your example shows with absolute certainty that you don't want to do linear comparison in a tight loop. :) – ChaosPandion Sep 14 '12 at 21:21

score 1 · Accepted Answer · answered Sep 14 '12 at 20:13

Because your sFileExtension is a single extension, not all four of the allowed extensions. This means that even if the sFileExtension is ONE of the allowed extensions, it will still not be the other three, so no matter what, your else statement will get hit.

score 1 · Answer 3 · answered Sep 14 '12 at 20:15

i think you want that when bFileOK =true you should break ; then use break statement

if (IsPostBack)
    {
        Boolean bFileOK = false;

        if (fulReagentImg.HasFile)
        {
            String sFileExtension = System.IO.Path.GetExtension(fulReagentImg.FileName).ToLower();
            String sFileExtensionLabel = sFileExtension;
            lblFileExtension.Text = sFileExtensionLabel;
            String[] allowedExtensions = { ".gif", ".png", ".jpeg", ".jpg" };
            for (int i = 0; i < allowedExtensions.Length; i++)
            {
                if (sFileExtension == allowedExtensions[i])
                {
                    bFileOK = true;
                    break;
                }
                else
                {
                    lblException.Text = "Can only upload .gif, .png, .jpeg, .jpg";
                    lblException.CssClass = "red";
                }

}

score 0 · Answer 4 · answered Sep 14 '12 at 20:13

0

You shouldn't have that else inside the for loop, or you'll post the fail message at least allowedExtensions.Length-1 times.

Move that else out of the for loop, and have it follow:

if(bFileOK)
{
//Do Stuff
}

answered Sep 14 '12 at 20:13

Sconibulus

732
9
21

Jim Wooley · Answer 5 · 2012-09-14T20:15:17.420

Is it possible that the first item in the allowed extensions succeeds and the second one fails thus entering the first and second part of the criteria? Perhaps you meant this:

if (IsPostBack)
    {
        Boolean bFileOK = false;

        if (fulReagentImg.HasFile)
        {
            String sFileExtension = System.IO.Path.GetExtension(fulReagentImg.FileName).ToLower();
            String sFileExtensionLabel = sFileExtension;
            lblFileExtension.Text = sFileExtensionLabel;
            String[] allowedExtensions = { ".gif", ".png", ".jpeg", ".jpg" };
            for (int i = 0; i < allowedExtensions.Length; i++)
            {
                if (sFileExtension == allowedExtensions[i])
                {
                    bFileOK = true;
                }
            }
            if (!bFileOK)
            {

                    lblException.Text = "Can only upload .gif, .png, .jpeg, .jpg";
                    lblException.CssClass = "red";

            }
     }
}

score 0 · Answer 6 · answered Sep 14 '12 at 20:18

You need to add a break statement after setting bFileOk to true. As you are looping through every string in the array you're effectively only testing the last string I am the array.

I'd also investigate Array.Contains which will provide a cleaner implementation.

C# IF statement not breaking correctly

6 Answers6