Make a cascaded Else inside a for

Question

See below my for. Within that for there are some IF's. I don’t want another for to scroll through the same list. There is a if which validates whether IcObrigatorio == 1 and below it validates whether vlstDados[i].DsPathDocumento is neither empty nor null. What I want if possible in the same for to avoid having to go through the same list again, to do this validation backwards, that is, if IcObrigatorio == 0 and vlstDados[i].DsPathDocumento equal to null or void, then I am a variável booleana. Guys I’m getting hit on this lobe because of how I put one else cascateado, if that’s the case. Below mine for.

for (int i = 0; i < vlstDados.Count; i++)
                {
                    vstrNmPessoa = vlstDados[i].NmPessoa;

                    if (vlstDados[i].IcObrigatorio == 1)
                    {
                        if (string.IsNullOrEmpty(vlstDados[i].DsPathDocumento))
                        {
                            if (vlstDados[i].CdTipoDocumentoSubstitui == 0)
                            {
                                vbolErro = true;
                            }
                            else
                            {
                                for (int v = 0; v < vlstDados.Count; v++)
                                {
                                    if (vlstDados[i].CdTipoDocumentoSubstitui == vlstDados[v].CdTipoDocumento)
                                    {
                                        if (string.IsNullOrEmpty(vlstDados[v].DsPathDocumento))
                                        {
                                            vbolErro = true;
                                        }
                                    }
                                }
                            }
                        }
                    }
                }

I ended up doing it the way below. I didn’t like it, because I had to repeat the same command above Else. My intention would be to see if there’s some sort of cascading Else, like:

else 
else 

and I don’t know if that would be better either. So I did

for (int i = 0; i < vlstDados.Count; i++)
                {
                    vstrNmPessoa = vlstDados[i].NmPessoa;

                    if (vlstDados[i].IcObrigatorio == 1)
                    {
                        if (string.IsNullOrEmpty(vlstDados[i].DsPathDocumento))
                        {
                            if (vlstDados[i].CdTipoDocumentoSubstitui == 0)
                            {
                                vbolErro = true;
                            }
                            else
                            {
                                for (int v = 0; v < vlstDados.Count; v++)
                                {
                                    if (vlstDados[i].CdTipoDocumentoSubstitui == vlstDados[v].CdTipoDocumento)
                                    {
                                        if (string.IsNullOrEmpty(vlstDados[v].DsPathDocumento))
                                        {
                                            vbolErro = true;
                                        }
                                    }
                                }
                            }
                        }
                    }
                    else
                        if (vlstDados[i].IcObrigatorio == 0 && string.IsNullOrEmpty(vlstDados[i].DsPathDocumento))
                        {
                            vbolErroDocTorObrigatorio = true;
                        }
                }

Answer 1

So you didn’t like the code you had to use to solve the problem.

Here is a step-by-step for you to like your code and from there manage to implement a better logic:

Do not use names that are not significant to the business

Try to give names closer to the business rule than to the technical features of the code. So:

Do not identify the type of artifact in your name

Are you wearing a "v" prefix. Seems to indicate "variable". This is unnecessary. Never do it again and as a result, in addition to not feeling any lack of this practice you will still have a more expressive code.

Do not identify the data type in the artifact name

You are using "this" to indicate that it is a list. Do not do this. Just putting the name in the plural makes it clear enough that it is a list: "datum" contains 1 data and "dice" contains N data.

You are also using "bol" to specify a boolean. Do not do this. Instead, give a name that makes it clear that the variable contains true whether it asserts a truth or false otherwise. For example, instead drive use stuck or deuce or pathDocument.

Eventually until simply error already serve, since the code that will read the variable would be like this if (erro) and that makes perfect sense; unlike if (vbolErro).

Is it possible that you are so used to the "vbol" in the name of the variable you only read "error" instead of reading "see error pin". And this proves how unnecessary this prefix is, in addition to hindering the reading of others not as accustomed as you.

The same goes for the "str" indicating that it is a string. This type of information in a high level language like the one you use, in addition to not helping, disturbs.

Entering the data type in the name is an archaic practice.

Do not use generic terms like artifact names

Avoid calling your artifacts "data" and "info", for example, because in a system almost everything is given or information. Instead, choose meaningful business names like "document"; or, in the case of a list, "documents".

Do not use chained or nested IF

What are IFs nested:

if (condicao)
{
    if (outraCondicao)
    {
        ...
    }
}

Each IF in the code is an extra possible path. Each alternate path is an extra complexity. And complexity is a necessary but undesirable evil, it should be reduced to a minimum. So the less IF, the better.

A beautiful code block possesses no if - he tells a story in a fluid way.

So that:

• Zero ifs is good.

• A if is a necessary evil.

• A if within another ("chained" or "nested") is a necessary evil with the potential to be eliminated.

• Two or more ifnested is an evil that can and needs to be eliminated right now.

So, always doubt your logic every time you need to cuddle ifs.

Output, prefer repetition instead of complexity

Code repetition or duplication is not the first thing to be deleted during the Refactoring (process of improving your code without changing the behavior).

First write a simple code that does what needs to be done, even with duplicates. Then eliminate duplications but without the burden of adding complexity. Use ifnested s increases complexity.

Completion:

The best way you can improve your code from where you are:

• Rename your variables as described here.

• Bring the repetition back. Make as many loops as you need, i.e., scroll through the list once for each problem to be solved.

Maybe then you’ll be satisfied. Expressive code can be more important than gaining a billionth of a second by scrolling through a list less often, for example. If you’re not satisfied:

• Eliminate code duplication without using ifs nested. Use if nested does not simplify anything.

If you are not satisfied, post a new question with the code you have at hand. It will be clear enough that someone can help you with your logic (although I’m not sure that this is code review or some other kind of off-topic question for Sopt).

Update: Ah, yes, of course! Take a look at LINQ, which can be an expressive and little code way to solve this type of problem.

Answer 2

From what I understand what you need is this, within each block you put the rest of your code.

for (int i = 0; i < vlstDados.Count; i++)
{
    vstrNmPessoa = vlstDados[i].NmPessoa;

    if (vlstDados[i].IcObrigatorio == 1 && string.IsNullOrEmpty(vlstDados[i].DsPathDocumento))
    {
        /* ... */
    }
    else if (vlstDados[i].IcObrigatorio == 0 && !string.IsNullOrEmpty(vlstDados[i].DsPathDocumento))
    {
        /* ... */
    }
}

Answer 3

I tried to rewrite your is so that it becomes more legible, I could check if the logic is correct?

for (int i = 0; i < vlstDados.Count; i++)
{
    var isObrigatorio = vlstDados[i].IcObrigatorio == 1;
    var hasPathDocumento = !string.IsNullOrEmpty(vlstDados[i].DsPathDocumento);
    var isTipoDocSubstui = !vlstDados[i].CdTipoDocumentoSubstitui == 0; 

    if (!isObrigatorio) {
        vbolErroDocTorObrigatorio = !hasPathDocumento;
        continue;
    }

    if (!hasPathDocumento) {
        continue;
    }

    if (!isTipoDocSubstui) {
        vbolErro = true;
        continue;
    }

    for (int v = 0; v < vlstDados.Count; v++)
    {
        var documentoFound = vlstDados[i].CdTipoDocumentoSubstitui == vlstDados[v].CdTipoDocumento;
        hasPathDocumento = !string.IsNullOrEmpty(vlstDados[v].DsPathDocumento);
        if (documentoFound && !hasPathDocumento)
        {
            vbolErro = true;
            break;
        }
    }
}