0
\$\begingroup\$

First I have two conditions on the number of list elements,then in one of these conditions I have two others conditons to treat it :

My code:

List<StringBuilder> result = ReadResult_WS_Import(NameFile); 
 if ((result != null) && (result.Any()))
                {
                    var is_No_Errors_Found_Exist = result[0].ToString().Contains(Properties.Resources.ARGS_RESULT_Is_No_Errors_Found_Exist);
                    var is_Import_Failed_Exist = result[0].ToString().Contains(Properties.Resources.ARGS_RESULT_Is_Import_Failed_Exist);
                    var is_The_Import_Has_Failed_Exist = result[0].ToString().Contains(Properties.Resources.ARGS_RESULT_Is_The_Import_has_Failed_Exist);

                    string[] result_LOG = result[0].ToString().Split(new string[] { "\r\n" }, StringSplitOptions.RemoveEmptyEntries);
                    if ((result.Count == 2) && (!is_The_Import_Has_Failed_Exist))
                    {
                        result_REJECT = result[1].ToString().Split(new string[] { "\r\n" }, StringSplitOptions.RemoveEmptyEntries);
                        StatusResult = Properties.Resources.ARGS_RESULT_OK_REJECT;
                    }            
                    else if (is_No_Errors_Found_Exist)
                    {
                        StatusResult = Properties.Resources.ARGS_RESULT_OK;
                    }
                    else if (is_Import_Failed_Exist)
                    {
                        StatusResult = Properties.Resources.ARGS_RESULT_KO;
                    }

                    if ((is_The_Import_Has_Failed_Exist) && (result.Count == 2))
                    {
                        result_REJECT = result[1].ToString().Split(new string[] { "\r\n" }, StringSplitOptions.RemoveEmptyEntries);
                        StatusResult = Properties.Resources.ARGS_RESULT_KO_REJECT;
                    }               

                    returnObject = new ReturnObject
                    {
                        CountLines = CountLines,
                        Status = StatusResult,
                        LOG = result_LOG,
                        REJECT = result_REJECT
                    };
                }

It works well, I don't have any problems, but I want to know if there is other code organization ? or any feedback ?

\$\endgroup\$
2
  • \$\begingroup\$ What's the purpose of your code? You completely forgot to describe that in natural language. Plus, your code doesn't compile because most names cannot be resolved. This makes the question off-topic here. \$\endgroup\$ Jan 27 '20 at 20:49
  • \$\begingroup\$ Is the current formatting your code's actual formatting, or a result of copying it into your browser? \$\endgroup\$ Jan 29 '20 at 10:03
6
\$\begingroup\$

Naming and underscores

Underscores help to separate long sentences. But variable names should not be long sentences. You should drop the underscores, stick with camelCasing (as per convention), and shorten the names.

bool errorsFound;
bool importFailed;

The underscores in your constants are not as much of an issue. I have generally moved away from the SCREAMCASING for constants and instead just PascalCase them (without underscores); but that's not a globally agreed upon standard.

As an aside, unit and integration test methods often do have long sentences for names, and underscores are actually useful there. So you're definitely allowed to use underscores where appropriate. Local variables with long names are hardly ever appropriate.


Simplifying the logic

if ((result.Count == 2) && (!is_The_Import_Has_Failed_Exist))
{
    result_REJECT = result[1].ToString().Split(new string[] { "\r\n" }, StringSplitOptions.RemoveEmptyEntries);
    StatusResult = Properties.Resources.ARGS_RESULT_OK_REJECT;
}      

if ((is_The_Import_Has_Failed_Exist) && (result.Count == 2))
{
    result_REJECT = result[1].ToString().Split(new string[] { "\r\n" }, StringSplitOptions.RemoveEmptyEntries);
    StatusResult = Properties.Resources.ARGS_RESULT_KO_REJECT;
}   

These two evaluations share some commonalities, and their bodies are identical (assuming OK/KO is a typo). This can be merged into a single evaluation:

if(result.Count() == 2 && (!is_The_Import_Has_Failed_Exist || is_The_Import_Has_Failed_Exist) )
{        
    result_REJECT = result[1].ToString().Split(new string[] { "\r\n" }, StringSplitOptions.RemoveEmptyEntries);
    StatusResult = Properties.Resources.ARGS_RESULT_OK_REJECT;
}

Clear naming

var is_Import_Failed_Exist = ...;
var is_The_Import_Has_Failed_Exist = ...;

For the life of me, I cannot figure out what the difference between them is. The naming of these variables (and the constants after which they are named) is not good, since a reader cant figure out what they represent.

returnObject = new ReturnObject { ... }

ReturnObject is such a vague name that it fails to express anything meaningful. I'll happily admit that I've struggled coming up with names for return DTOs too, but it needs to have some sort of descriptive name, e.g. ImportStatusDto or ImportStatusResult.

Properties.Resources.ARGS_RESULT_OK_REJECT

"OK" and "reject" seemingly contradict one another. Is this a good outcome, or a bad one? It's unclear and needs better naming to make this clear.

else if (is_No_Errors_Found_Exist)
{
    StatusResult = Properties.Resources.ARGS_RESULT_OK;
}
else if (is_Import_Failed_Exist)
{
    StatusResult = Properties.Resources.ARGS_RESULT_OK; // I assume "KO" was a typo
}

As a developer reading your code, it's very confusing why the failure of an import is considered a positive ("OK") outcome. This seemingly contradicts the evaluation above where the existence of errors is clearly not OK.

This may be a bug in your code. In that case, you probably would've spotted the bug easier if your variable names had been easier to read (see the above tips).

If this isn't a bug, then I'm still inclined to conclude that your naming introduces confusion where none should have existed.

CountLines 

This sounds like a command or instruction ("Hey, you! Count those lines!"), which is how methods should be named.

Variable names (except for booleans) should be nouns or noun phrases. LineCount is the better name here.

For completeness' sake, boolean names should generally adhere to yes/no questions: isAlive, hasFood, ... or in other cases phrasings that strongly convey a binary result: userWasDeleted, importFailed, ...
In all of these cases, if I were to put a question mark behind the name, it would be bad English but you would understand the question and would easily identify what the meaning of "yes" and "no" is.


Avoid negative variable names

is_No_Errors_Found_Exist

While you can read it now, this can quickly get out of hand. How would you check that errors were found?

if(!is_No_Errors_Found_Exist)

That's a double negative, and really hard to parse. It's much better to stick with "positive" naming and then negate the boolean value

bool errorsFound = ...;

if(!errorsFound) { ... }

if(errorsFound) { ... }

In this example, there's no need to use a double negative, which improves readability.

Your internal voice should read this as "not errors found", which it intuitive and clearly expresses the intention of the evaluation.

Note that when I say "positive", I mean "not negated" instead of "good". A boolean called isFailed is a "positive" naming. Not because failure is good (it clearly isn't), but because it's not negated.
importDidNotFail would be the "negative" naming (because of the "not"), which you should try to avoid to enhance overall readability.


In conclusion

This is more of a review on your question than the code, but there is a common thread here.

First I have two conditions on the number of list elements,then in one of these conditions I have two others conditons to treat it

Try to read this from the perspective of a StackExchange user who knows nothing about you or your code. Does this explain what your code is trying to achieve?

I think you're explaining things the way they already make sense to you, while not considering how others will (or won't) understand what you write.

I mention this because code readability should be written from the perspective of those who don't know the code. Whenever you name something (a variable, a method, ...) you need to ask yourself a simple question:

Would a developer who is new to the project understand what this method does/what this variable expresses?

This is by far the biggest issue in your code. Your naming has made it very hard for anyone else to read your code and understand its purpose.

And it's not just about other people. If you start working on another project, and in a few months' time you have to return to this project, you're going to be scratching your head too, because you won't remember all the things you currently know and will have to build up that working knowledge again.

The other minor issues/bugs I've found are actually fairly common in codebases where readability is hampered, because low readability leads to a high likelyhood of making unintentional mistakes.

I suspect that if you work on improving your code readability, you'll automatically start seeing how to improve parts of the code more and more.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you so much for all the explanations and advices, it is very detailed \$\endgroup\$
    – IngTun2018
    Jan 28 '20 at 9:35
2
\$\begingroup\$

You use several times the result[0].ToString(). Store it if you need it several times.

The 3 variables in the beginning result in, those variables are computed, also when their value is not used. If you do not use these variables, but put the "Contains" evaluation directly in place, they would be only evaluated, when needed.

The last 'If' has no else before. That means it might be possible, a formerly set StatusResult will be overriden.

StatusResult is not always set, I'm not sure there is always at least one path taken.

You could create the ResultObject in the beginning, and store the member values directly, instead of first in local variables, and then copy the local variables. That's minor, cause it's fast, but more lines make the code less readable.

You really distinguish "OK" and "KO" ? You play tricks on everyone's brain. As more similar words are, as more easy they are mixed up by people. This comment is different from the other answer. I do not try to understand the meaning.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks a lot for your reply and your advices \$\endgroup\$
    – IngTun2018
    Jan 28 '20 at 9:36
1
\$\begingroup\$

Some quick remarks:

  • A List<StringBuilder> seems a very bad return value, IMHO. Is there a reason for such an unusual type?

  • result is a List<StringBuilder>, so it should be results.

  • What is returned in the case of if ((result == null) || (!result.Any()))? I'm guessing a null, so return that as fast as possible, that way the rest of the code doesn't need to be indented as much.

  • Instead of "\r\n", use Environment.NewLine.

  • This code is almost 40 lines long and yet it seems to be only a part of a longer section of code. This worries me, because it suggests that the method it is part of is much longer and even more convoluted. I'd urge you to split your code into smaller methods which each do one particular task.


But my main objection is List<StringBuilder>, and that you use this list to do a Contains() on its contents (and then even assume some kind of order in the returned result when a condition about result[0] causes you to parse result[1]). Considering that you're searching for specific predefined phrases, suggests that the return of ReadResult_WS_Import should really be a custom class with meaningful properties.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you so much for these good advices and tips \$\endgroup\$
    – IngTun2018
    Jan 29 '20 at 10:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.