Simplify long nested if statement workflow C#

Question

This program is for validating Excel row data. If a if statement is false, I need to append a message to a string and so forth for N number of if statements. This doesn't work because as you can see if the first if statement fails, I would not proceed onto the the other if statements. Also later on, I'll need to handle 100+ columns, so I'm looking for a way to do this.

Is there any other way I can rewrite this, so it is more readable and less repetitive? I know I can just do a giant if (.... && ....) for validating all cells and individual if statements to append messages, but I was wondering if there is a another way to do this. I would like to preserve the order of the errors, but that is not as important. The result would be something like "facilityID is invalid, daysOfTheWeek is invalid" I'm also returning a custom Pair datatype.

string facilityID, facilityDockDoorID, increment, hoursOfOperationId, updatedById, startTime, endTime, dockDoorServiceType, daysOfTheWeek;
            facilityID = row[0];
            facilityDockDoorID = row[1];
            increment = row[2];
            hoursOfOperationId = row[3];
            updatedById = row[4];
            startTime = row[5];
            endTime = row[6];
            dockDoorServiceType = row[7];
            daysOfTheWeek = row[8];

            string errorMessage = " is invalid";
            if (IsInt(facilityID))
            {
                if (IsInt(facilityDockDoorID))
                {
                    if (IsInt(increment))
                    {
                        if (IsInt(hoursOfOperationId))
                        {
                            if (IsInt(updatedById))
                            {
                                if (IsTime(startTime))
                                {
                                    if (IsTime(endTime))
                                    {
                                        if (IsValidDockDoorServiceType(dockDoorServiceType))
                                        {
                                            if (IsValidDayOfTheWeek(daysOfTheWeek))
                                            {
                                                isDataValid.First = true;
                                            }
                                            else
                                            {
                                                isDataValid.Second += "daysOfTheWeek" + errorMessage + ",";                                                
                                            }
                                        }
                                        else
                                        {
                                            isDataValid.Second += "dockDoorServiceType" + errorMessage + ",";

                                        }
                                    }
                                    else
                                    {
                                        isDataValid.Second += "endTime" + errorMessage + ",";
                                    }
                                }
                                else
                                {
                                    isDataValid.Second += "startTime" + errorMessage + ",";
                                }
                            }
                            else
                            {
                                isDataValid.Second += "updatedById" + errorMessage + ",";
                            }
                        }
                        else
                        {
                            isDataValid.Second += "hoursOfOperationId" + errorMessage + ",";
                        }
                    }
                    else
                    {
                        isDataValid.Second += "increment" + errorMessage + ",";
                    }
                }
                else
                {
                    isDataValid.Second += "facilityDockDoorID" + errorMessage + ",";
                }
            }
            else
            {
                isDataValid.Second = "facilityID" + errorMessage + ",";
            }
            return isDataValid;
        }

possible duplicate http://stackoverflow.com/questions/17804005/how-to-prevent-the-arrowhead-anti-pattern — nicholas, Jul 05 '16 at 19:01
@nicholas I looked at that question. However, I'm not intending to return from any of the if statements. Only intending to return after going through all the checks. The most important part is getting the message — ygongdev, Jul 05 '16 at 19:05
but your code sample as-posted will short circuit after the first failed check and return — nicholas, Jul 05 '16 at 19:06
I see where you're talking about. I think thats my mistake. I shall put the return outside, I accidentally put it inside when copying pasting my code over — ygongdev, Jul 05 '16 at 19:08
@nicholas I don't think he wants to keep the same behavior, as he says it isn't working the way he wants in the first paragraph. — Bradley Uffner, Jul 05 '16 at 19:09
jk nvm. My code is not correct btw. It is just an exmaple of what i tried to do. What I want to do is NOT for it to short circuit after first failed check. — ygongdev, Jul 05 '16 at 19:10

juharr · Accepted Answer · 2016-07-05T19:12:48.567

If you invert the if statements and return inside each one you can flatten your code out like this.

if (!IsInt(facilityID))
{
    isDataValid.Second = "facilityID" + errorMessage + ",";
    return isDataValid;
}
if (!IsInt(facilityDockDoorID))
{
    isDataValid.Second += "facilityDockDoorID" + errorMessage + ",";
    return isDataValid;
}
if (!IsInt(increment))
{
    isDataValid.Second += "increment" + errorMessage + ",";
    return isDataValid;
}
if (!IsInt(hoursOfOperationId))
{
    isDataValid.Second += "hoursOfOperationId" + errorMessage + ",";
    return isDataValid;
}
if (!IsInt(updatedById))
{
    isDataValid.Second += "updatedById" + errorMessage + ",";
    return isDataValid;
}
if (!IsTime(startTime))
{
    isDataValid.Second += "startTime" + errorMessage + ",";
    return isDataValid;
}
if (!IsTime(endTime))
{
    isDataValid.Second += "endTime" + errorMessage + ",";
    return isDataValid;
}
if (!IsValidDockDoorServiceType(dockDoorServiceType))
{
    isDataValid.Second += "dockDoorServiceType" + errorMessage + ",";
    return isDataValid;
}
if (IsValidDayOfTheWeek(daysOfTheWeek))
{
    isDataValid.First = true;
}
else
{
    isDataValid.Second += "daysOfTheWeek" + errorMessage + ",";
}
return isDataValid;

However based on the fact that you are concatenating to Second it's more likely that you actually want something like

if (!IsInt(facilityID))
    isDataValid.Second = "facilityID" + errorMessage + ",";
if (!IsInt(facilityDockDoorID))
    isDataValid.Second += "facilityDockDoorID" + errorMessage + ",";
if (!IsInt(increment))
    isDataValid.Second += "increment" + errorMessage + ",";
if (!IsInt(hoursOfOperationId))
    isDataValid.Second += "hoursOfOperationId" + errorMessage + ",";
if (!IsInt(updatedById))
    isDataValid.Second += "updatedById" + errorMessage + ",";
if (!IsTime(startTime))
    isDataValid.Second += "startTime" + errorMessage + ",";
if (!IsTime(endTime))
    isDataValid.Second += "endTime" + errorMessage + ",";
if (!IsValidDockDoorServiceType(dockDoorServiceType))
    isDataValid.Second += "dockDoorServiceType" + errorMessage + ",";
if (!IsValidDayOfTheWeek(daysOfTheWeek))
    isDataValid.Second += "daysOfTheWeek" + errorMessage + ",";
isDataValid.First = isDataValid.Second.Length == 0;
return isDataValid;

Notice that I'm comparing the length of Second to determine if any errors occurred.

I was thinking of this as my last resort. I was wondering if I could do something similar to a switch(), but for sequentially if statements. Because later on, this method would require 100+ if statements for 100+ columns — ygongdev, Jul 05 '16 at 19:13
If you are doing a lot of this kind of check, you might want to rig up some kind of general purpose "scheme" validator function that loops through columns and compares with an expected data type using a loop instead of a hard coded function. You could place the expected types in a comma separated list, like "int, int, date, date, string", split them up, and to a switch inside a loop. — Bradley Uffner, Jul 05 '16 at 19:14
@BradleyUffner, Yes I am going to do a lot of this kind of check, which is why I'm looking for a better suited solution. I'll try your suggestion. Meanwhile I'm going to accept this as the temporary solution because it does do what I want my current code to do. — ygongdev, Jul 05 '16 at 19:17

score 4 · Answer 2 · answered Jul 05 '16 at 19:01

4

You could invert the if statements. For instance:

if(!IsInt(facilityID))
{
    isDataValid.Second = "facilityID" + errorMessage + ",";
}
if(!IsInt(facilityDockDoorID))
{
    isDataValid.Second += "facilityDockDoorID" + errorMessage + ",";
}

answered Jul 05 '16 at 19:01

Christos

53,228
8
76
108

1

The OP would also need to add `return` statements to get the same behavior. – juharr Jul 05 '16 at 19:05
Wouldn't this mean that, I need to have 8 individual if statements for appending. and then I would also need a giant if ( ... && ... && ...) to check if all data are validated? I'm returning a custom Pair struct btw. Not just a boolean – ygongdev Jul 05 '16 at 19:07
@ygongdev nope. You could just check to see if your error message has any text in it, or set some other error flag that you check later. – Bradley Uffner Jul 05 '16 at 19:08

score 2 · Answer 3 · answered Jul 05 '16 at 19:24

You could build a collection of validator functions, for example:

List<Func<string[], StringBuilder, bool>> validators = new List<Func<string[], StringBuilder, bool>>();

validators.Add((row, logger) => 
{
    string facilityID = row[0];

    if(IsInt(facilityID))
    {
        logger.AppendLine("facilityID is invalid");

        return false;
    }

    return true;
});

validators.Add((row, logger) =>
{
    string increment = row[2];

    if (IsInt(increment))
    {
        logger.AppendLine("increment is invalid");

        return false;
    }

    return true;
});

. . .

You can then just loop through all like so:

StringBuilder log = new StringBuilder();

if(validators.Any(v => v(rows, log) == false))
{
    return false;
}
else
{
    return true;
}

isDataValid.Second = log.ToString();

This way you can break the validator logic up until into smaller chunks. This avoid nesting the if statements. Each validator is responsible for picking the data it wants to validate, returning a valid/invalid bool, and logging any messages to the common StringBuilder. As your list of validators grows you can just add a new function to the collection.

I like this implementation because you can take it one step further. For super clean code, ideally what you need is to define your schema in such a way that it can be changed or modified with minimal updating to this code. — m1m1k, Jul 05 '16 at 19:47

score 0 · Answer 4 · answered Jul 05 '16 at 19:51

It's best to break your schema apart from your code functionality. This allows you to change the schema in the future without having to change the rest of the code.

    public class SchemaItem
    {
        public int Index;
        public string ColumnName;
        public int ParsedValue;

        public SchemaItem(int index, string columnName)
        {
            Index = index;
            ColumnName = columnName;
        }
    }
    public static Dictionary<int, SchemaItem> Schema = new Dictionary<int, SchemaItem>
    {
        {0, new SchemaItem(0, "facilityID")},
        {1, new SchemaItem(1, "facilityDockDoorID")},
        {2, new SchemaItem(2, "increment")},
        {3, new SchemaItem(3, "hoursOfOperationId")},
        {4, new SchemaItem(4, "updatedById")},
        {5, new SchemaItem(5, "startTime")},
        {6, new SchemaItem(6, "endTime")},
        {7, new SchemaItem(7, "dockDoorServiceType")},
        {8, new SchemaItem(8, "daysOfTheWeek")},
    };
    private static bool ParseIntAndOrWriteFailMessage(string[] row, StringBuilder logger, int index)
    {
        string cellContents = row[index];
        string columnName = Schema[index].ColumnName;
        int intContents = -1;
        if (int.TryParse(cellContents, out intContents))
        {
            Schema[index].ParsedValue = intContents;
            return true;
        }
        logger.AppendFormat("{0} is invalid,", columnName);
        return false;
    }

Then finally you would do:

        StringBuilder logger = new StringBuilder();
        foreach (var column in Schema.Select(pair => pair))
        {
            ParseIntAndOrWriteFailMessage(row, logger, column.Key);
        }
        // Parsed values are now stored in the Schema Dictionary (which probably could use a better name)

Simplify long nested if statement workflow C#

4 Answers4