2

Veja abaixo o meu for. Dentro desse for existe alguns IF's. Não quero um outro for para percorrer a mesma lista. Há um if que valida se IcObrigatorio == 1 e abaixo dele valida se vlstDados[i].DsPathDocumento não é vazio e nem nulo. O que eu quero se possível no mesmo for para não ter que percorrer outra vez a mesma lista, fazer essa validação ao contrário, ou seja, caso IcObrigatorio == 0 e vlstDados[i].DsPathDocumento igual a nulo ou vazio, aí eu seto uma variável booleana. Pessoal estou apanhando nessa lógiga devido a como eu coloco um else cascateado, se é o caso. Abaixo meu 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;
                                        }
                                    }
                                }
                            }
                        }
                    }
                }

Acabei fazendo da forma abaixo. Não gostei, pois tive que repeti o mesmo comando acima do else. A intensão minha seria ver se há uma espécie de else em cascata, tipo:

else 
else 

e não sei se isso seria o melhor também. Assim fiz

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;
                        }
                }
1
  • .IcObrigatorio pode assumir valores diferentes de 0 e 1? 11/02/2015 às 19:37

3 Respostas 3

3

Então você não gostou do código que teve que usar para resolver o problema.

Eis um passo-a-passo para você gostar do seu código e a partir daí conseguir implementar uma lógica melhor:

Não use nomes que não sejam significativos ao negócio

Procure dar nomes mais próximos à regra de negócio do que às características técnicas do código. Assim:

Não identifique o tipo do artefato no seu nome

Você está usando um "v" como prefixo. Parece ser para indicar "variável". Isso é desnecessário. Nunca mais faça isso e, como resultado, além de não sentir nenhuma falta desta prática você ainda vai ter um código mais expressivo.

Não identifique o tipo do dado no nome do artefato

Você está usando "lst" para indicar que é uma lista. Não faça isso. Apenas colocar o nome no plural deixa claro o bastante que se trata de uma lista: "dado" contém 1 dado e "dados" contém N dados.

Você também está usando "bol" para indicar um booleano. Não faça isso. Ao invés, dê um nome que deixe claro que a variável contém true se ela afirma uma verdade ou false caso contrário. Por exemplo, em vez vbolErro use estaErrado ou deuErro ou pathDocumentoNaoInformado.

Eventualmente até simplesmente erro já serve, já que o código que vai ler a variável ficaria assim if (erro) e isso faz todo sentido; ao contrário de if (vbolErro).

É possível que você esteja tão acostumado com o "vbol" no nome da variável que você só leia "erro" em vez de ler "vê ból erro". E isto prova o quanto este prefixo é desnecessário, além de atrapalhar a leitura de outros não tão acostumados quanto você.

O mesmo vale para o "str" indicando que é uma string. Este tipo de informação em uma linguagem de alto nível como a que você usa, além de não ajudar, atrapalha.

Indicar no nome o tipo de dado é uma prática arcaica.

Não use termos genéricos como nomes de artefatos

Evite chamar seus artefatos de "dado" e "info", por exemplo, pois em um sistema quase tudo é dado ou informação. Ao invés, escolha nomes significativos para o negócio como "documento"; ou, no caso de uma lista, "documentos".

Não use IF encadeado ou aninhado

O que são IFs aninhados:

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

Cada IF no código é um caminho possível à mais. Cada caminho alternativo, é uma complexidade à mais. E complexidade é um mal necessário mas indesejável, deve ser reduzida ao mínimo. Assim, quanto menos IF, melhor.

Um bloco de código bonito não possui nenhum if - ele conta uma história de maneira fluída.

De modo que:

  • Zero ifs é bom.

  • Um if é um mal necessário.

  • Um if dentro de outro ("encadeado" ou "aninhado") é um mal necessário com potencial para ser eliminado.

  • Dois ou mais ifs aninhados é um mal que pode e precisa ser eliminado agora mesmo.

Então, sempre duvide da sua lógica cada vez que você precisar aninhar ifs.

De saída, prefira a repetição em vez da complexidade

A repetição ou duplicação de código não é a primeira coisa a ser eliminada durante o refactoring (processo de melhoria do seu código sem mudar o comportamento).

Primeiro escreva um código simples que faça o que precisa ser feito, mesmo que com duplicações. Depois elimine as duplicações mas sem o ônus de adicionar complexidade. Usar ifs aninhados aumenta a complexidade.

Conclusão:

A melhor maneira de você melhorar o seu código a partir de onde você está é:

  • Renomeie suas variáveis conforme o que descrevi aqui.

  • Traga a repetição de volta. Faça quantos loops forem necessários, ou seja, percorra a lista uma vez para cada problema a ser resolvido.

Talvez aí você já esteja satisfeito. Um código expressivo pode ser mais importante do que ganhar um bilionésimo de segundo por percorrer uma lista menos vezes, por exemplo. Se não estiver satisfeito:

  • Elimine a duplicação de código sem usar ifs aninhados. Usar if aninhado não simplifica nada.

Se não estiver satisfeito, poste uma nova pergunta com o código que você tiver em mãos. Ele estará claro o bastante para que alguém possa ajudá-lo com a sua lógica (embora eu não tenha certeza de que isso não seja code review ou algum outro tipo de pergunta off-topic para o SOpt).

Update: Ah, sim, claro! Dê uma estudada em LINQ, que pode ser uma maneira expressiva e de pouco código para soluçao deste tipo de problema.

1
  • Usei lambda e resolveu minha questão de código limpo.
    – pnet
    12/02/2015 às 10:33
1

Pelo que entendi o que você precisa é isso, dentro de cada bloco você coloca o restante de seu código.

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))
    {
        /* ... */
    }
}
3
  • Isso foi o que eu fiz, mas tive que repetir os camandos.
    – pnet
    11/02/2015 às 12:44
  • @pnet De fato não entendi o que você quer então. 11/02/2015 às 12:46
  • Acho que a pergunta foi mal elaborada, só isso. Desculpas à comunidade. Não vou remover que pode ajudar outras pessoas.
    – pnet
    11/02/2015 às 13:11
1

tentei reescrever o seu for para que o mesmo fique mais legivel, poderia conferir se a logica está correta?

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;
        }
    }
}
1
  • Eu utilizei uma lambda e ficou mais claro. A questão toda é referente a código limpo, exigência patronal, por isso o post para saber se há como um else cascateado para não ficar repetindo as mesmas coisas.
    – pnet
    12/02/2015 às 10:32

Você deve fazer log-in para responder a esta pergunta.

Esta não é a resposta que você está procurando? Pesquise outras perguntas com a tag .