How to improve string comparison and concatenation C#

Question

I have the following code:

private static string MontarDescricaoOrgaoUoUe(RequisicaoFisica requisicao)
        {
            return !string.IsNullOrEmpty(MontarNomeOrgao(requisicao)) ? MontarNomeOrgao(requisicao)
                    : !string.IsNullOrEmpty(MontarNomeUo(requisicao)) ? MontarNomeUo(requisicao)
                        : !string.IsNullOrEmpty(MontarNomeUe(requisicao)) ? MontarNomeUe(requisicao) : String.Empty;
        }


        private static string MontarNomeOrgao(RequisicaoFisica re)
        {
            var retorno = re.CodOrgaoFpe != null && re.CodUoFpe == null && re.CodUeFpe == null && !string.IsNullOrEmpty(re.NomeOrgaoFpe)
                ? string.Concat(re.CodOrgaoFpe, " - ", re.NomeOrgaoFpe)
                : string.Empty;

            return retorno;
        }

        private static string MontarNomeUo(RequisicaoFisica re)
        {
            var retorno = re.CodOrgaoFpe != null && re.CodUoFpe != null && re.CodUeFpe == null && !string.IsNullOrEmpty(re.NomeUoFpe)
                ? string.Concat(re.CodOrgaoFpe, ".", re.CodUoFpe, " - ", re.NomeUoFpe)
                : string.Empty;

            return retorno;
        }

        private static string MontarNomeUe(RequisicaoFisica re)
        {
            var retorno = re.CodOrgaoFpe != null && re.CodUoFpe != null && re.CodUeFpe != null && !string.IsNullOrEmpty(re.NomeUeFpe)
                ? string.Concat(re.CodOrgaoFpe, ".", re.CodUoFpe, ".", re.CodUeFpe, " - ", re.NomeUeFpe)
                : string.Empty;

            return retorno;
        }

I use it to assemble a "Description", using in all 6 properties of my object. Is there any way to improve this code in performance and readability? Which?

Answer 1

In performance I can not see anything that can help much, but this kind of thing can only be sure doing tests in real conditions. Unless you’re having real problems, I wouldn’t worry too much about this. I always try to do the fastest thing when it’s quite obvious, but not if it’s unreadable, if the gain is marginal and unnecessary. Surely there are some things in there that could give marginal gains but I doubt something that makes up for.

In readability there are several things that could be improved, but nothing very important. Some may be more like.

I use no variable when it will only be used once unless this contributes to readability, in this case I think returning the direct expression is better.

On the other hand, in the first method has some method calls more than once. Probably - but certainly not - I would call and hold in variable and use it. This would probably improve performance, marginally.

This method is also somewhat difficult to read because it has nested conditional ternary operators. You can understand but could at least organize better. Even not needing so much, the others are more organized and each operating is more obvious.

Some people would recommend you use if in place of the ternary operator. Even more that you use variable. With it, it even makes some sense even, because there is side effect. Looking at the code I think a good one.

In the first method maybe I used IsEmpty() since I know that can not return null.

Maybe I’d rethink all this design. But I don’t know if you don’t know all the details of the case, and that’s not the point of the question.

I think it’s silly to use string.Empty. Some people like it, some people find it more readable, I don’t, I prefer it "". It has no advantage to use it. Nor disadvantage.

I’d probably use the operator + to concatenate the texts. The compiler will transform them into Concat and I find it more readable with operators. There are those who disagree. I think this is the most important part of the question for the AP. But deep down, it makes no difference.

If you think you need to use StringBuilder, in this case is not necessary. Please note that Concat will probably make use of a StringBuilder if necessary. The compiler is intelligent to find the best solution in this particular case. The subject has already been dealt with in another question.

Some people will prefer the use of string.Format(). In fact there is case to think about it. Example:

string.Format("{0}.{1}.{2}-{3}", re.CodOrgaoFpe, re.CodUoFpe, re.CodUeFpe, re.NomeUeFpe)

Or in C# 6:

$"{re.CodOrgaoFpe}.{re.CodUoFpe}.{re.CodUeFpe}-{re.NomeUeFpe}"

Another thing I would probably think to do is to encapsulate these complex conditions in specific methods that return a bool. So give name to this condition, does not look like a magical thing, and hint more DRY, more canonical, facilitating maintenance. In what class put these methods is an art :)

There are other possible improvements if you can use C# 6. Checking null can be dispensed with. At least it has better syntax.

An example of what I would probably do:

private static string MontarDescricaoOrgaoUoUe(RequisicaoFisica requisicao) {
    var texto = "";
    if (!string.IsNullOrEmpty(MontarNomeOrgao(requisicao)) texto = MontarNomeOrgao(requisicao);
    else if (!string.IsNullOrEmpty(MontarNomeUo(requisicao)) texto = MontarNomeUo(requisicao);
    else if (!string.IsNullOrEmpty(MontarNomeUe(requisicao)) texto = MontarNomeUe(requisicao);
    return texto;
}

private static string MontarNomeOrgao(RequisicaoFisica re) => re.CodOrgaoFpe != null && re.CodUoFpe == null && re.CodUeFpe == null && !string.IsNullOrEmpty(re.NomeOrgaoFpe) ?
        re.CodOrgaoFpe + " - " + re.NomeOrgaoFpe :
        "";

I put in the Github for future reference.

For his previous question about ties It seems to me that you like to do everything in as few lines as possible. Don’t try this. It can worsen the readability, the understanding of all involved and even worsen the performance.