CSV to list, map to database

Question

I have a code block that imports CSV to list and map to a database. And there are few validations and I've to return error messages based on those validations. Right now all those logic is handled by multiple if else conditions. The code is working but I don't think it's the right approach. Is there any way I can replace these conditions with something clean?

if (fileCSV == null && fileCSV.ContentLength == 0)
{
 importModel.Error = "Error1";
 return importModel;
}
else
{
 List<ImportModel> mappings = _importService.ImportCSVToList<ImportModel>(fileCSV);
 if (mappings.Count > 0)
 {
 IEnumerable<ImportModel> duplicates = mappings.GroupBy(x => x.ProductSku).SelectMany(g => g.Skip(1)).ToList();
 if (duplicates.Any())
 {
 importModel.Error = "Error2";
 return importModel;
 }
 else
 {
 var products = _productService.GetProducts(productSkuList).ToList();
 if (!importModel.InvalidSkuList.Any())
 {
 bool isImported = _productService.Import(mappings);
 if (!isImported)
 {
 importModel.Error = "Error3";
 }
 }
 else
 {
 return importModel;
 }
 }
 }
 else
 {
 importModel.Error = "Error4";
 return importModel;
 }
}

Answer 1

Generally speaking whenever you are facing the problem like the above one what you can do is to perform an assessment against your current flow control and/or try to replace some part of your logic to reduce code complexity. The former one tries to logically reduce the complexity while the latter one mechanically.

Assessment

• Iterate through the different branches and try to describe each of them with simple words
• Try to visualize the nestedness by using indented lines
• Try to simplfy on it
  • By combining multiple branches into a single one
  • By removing unnecessary branches
  • By splitting up the whole logic into smaller chucks

Before

When entity is present

When entity's id is even

When entity's parent is X
When entity's parent is Y

Otherwise

Otherwise

Otherwise

After

When entity is not present or entity's id is odd

When entity's parent is one of [X, Y]

Otherwise

Replacement

In C# you have a couple of options. Just to name a few:

Ternary conditional operator

If you two branches with simple logic both returns with something then replace it with ternary conditional operator

From

if(condition)
{
 return X();
}
else
{
 return Y();
}

To

return condition ? X() or Y();

Null coalescing operator

A special case of the previous one is when you want to return X is it not null otherwise Y as a fallback value

From

var x = X();
if (x != null)
{
 return x;
}
else
{
 return Y();
}

To

return X() ?? Y();

Early exit

If you use the if-else structure to perform early exiting in the if branch then simply get rid of the else block

From

if (parameter is null)
{
 return -1;
}
else
{
 //The core logic
}

To

if (parameter is null)
{
 return -1;
}
//The core logic

Switch statement/expression

If you have a couple of else if blocks to handle different cases then prefer switch instead

From

if (x == "A")
{
 return A();
}
else if(x == "B")
{
 return B();
}
...
else
{
 return Fallback();
}

To

switch (x)
{
 case "A": return A();
 case "B": return B();
 ...
 default: return Fallback();
}

Or

return x switch
{
 "A" => A(),
 "B" => B(),
 ...
 _: => Fallback()
};

Applying these to your code

//I assume you wanted to check OR not AND in your original code
if (fileCSV?.ContentLength == 0)
{
 importModel.Error = "Error1";
 return importModel;
}
 
List<ImportModel> mappings = _importService.ImportCSVToList<ImportModel>(fileCSV);
if (mappings.Count == 0)
{
 importModel.Error = "Error4";
 return importModel;
}
IEnumerable<ImportModel> duplicates = mappings.GroupBy(x => x.ProductSku).SelectMany(g => g.Skip(1)).ToList();
if (duplicates.Any())
{
 importModel.Error = "Error2";
 return importModel;
}
 
//It seems like the products is unused, so this statement is unnecessary
//var products = _productService.GetProducts(productSkuList).ToList();
if (!importModel.InvalidSkuList.Any())
{
 importModel.Error = _productService.Import(mappings) ? importModel.Error : "Error3";
}
return importModel;

As I noted in the code I assumed that you wanted to write fileCSV == null || fileCSV.ContentLength == 0 in your outer most if statement, because with AND it does not make any sense.

• radarbob: Each alternative structure expresses different meaning and are not arbitrarily interchangeable. Inline early returns checks say all prerequisite conditions must be present before proceeding. if-else implies a mutually exclusive binary choice. A switch implies "only only one of these is correct." Code should, ideally, capture the wording and intent of business rules, context, algorithm, process, etc.
• Peter Csala: @radarbob Shifting from one alternative to another comes with a price / trade off. My intent was to show a toolbox for refactoring, not to convince one option is better than other. Sorry if it was not clear. :(
• radarbob: I am not advocating one nuance over another for this particular OP, but additional perspetive. I rather like your answer that it illustrates the various structures supporting your premise.

Answer 2

Since your validations might be reused, I think moving the validations into a separate method would improve your code. something like :

private bool TryValidateCSV(FileCSV file, out string errorMessage)
{
 errorMessage = null;
 
 if(fileCSV?.ContentLength == 0)
 {
 errorMessage = "Error1";
 return false;
 }
 List<ImportModel> mappings = _importService.ImportCSVToList<ImportModel>(fileCSV);
 if (mappings?.Count == 0)
 {
 errorMessage = "Error4";
 return false;
 }
 // no need for ToList(), just check the IEnumerable
 var hasDuplicates = mappings.GroupBy(x => x.ProductSku).SelectMany(g => g.Skip(1)).Any();
 if (hasDuplicates)
 {
 errorMessage = "Error2";
 return false;
 }
 
 if (!importModel.InvalidSkuList.Any() || !_productService.Import(mappings))
 {
 errorMessage = "Error3";
 return false;
 }
 
 return true;
}

the FileCSV it's just a placeholder that I assumed from the variable name fileCSV.

Now, you can do this :

if(!TryValidateCSV(fileCSV, out string errorMessage))
{
 importModel.Error = errorMessage;
}
return importModel;

You can then reuse the ValidateCSV method whenever you need to validate the file.

the other note that I see, is that you mapped the file to its model, and then used the returned value to validate. This might work fine, however, it is not the proper approach.

You need to separate the validation from the actual file processing (or modeling), to minimize the memory allocation, and improve the process performance.

So, if you can implement a method in _importService that would only check if the mapping has elements or not without the mapping process, then it would increase your code performance, and decrease your memory allocation.

same thing applies to _productService.Import.

You should only validate the integrity of the file, then process upon the validation results.

Answer 3

Not knowing the rest of your code or the exact contents it is difficult to offer better advice, but having multiple ifs isn't necessarily a bad thing as long as the code is readable, its not repetitive (each if does distinctly different things), and the code overall does what is expected. But as you seem to always want to return you can add that last, if no Error then normal importModel will be returned.

if (fileCSV == null && fileCSV.ContentLength == 0)
{
 importModel.Error = "Error1";
}
else
{
 List<ImportModel> mappings = _importService.ImportCSVToList<ImportModel>(fileCSV);
 if (mappings.Count > 0)
 {
 IEnumerable<ImportModel> duplicates = mappings.GroupBy(x => x.ProductSku).SelectMany(g => g.Skip(1)).ToList();
 if (duplicates.Any())
 {
 importModel.Error = "Error2";
 }
 else
 {
 if (!importModel.InvalidSkuList.Any())
 {
 bool isImported = _productService.Import(mappings);
 if (!isImported)
 {
 importModel.Error = "Error3";
 }
 // should there be a separate Error here or only if !isImported
 }
 }
 }
 else
 {
 importModel.Error = "Error4";
 }
}
return importModel;

Answer 4

Following is a fast prototyping with the sole purpose of depicting an alternative solution.

The if statements could be encapsulated in a class that enables preceding methods' execution with various tests, so called guard clauses.

try
{
 List<ImportModel> mappings = Ensure.Clause( () => { return ( fileCSV == null || fileCSV.ContentLength == 0 ); }, () => { importModel.Error = "Error1"; } )
 .Run( () => { return _importService.ImportCSVToList<ImportModel>(fileCSV); } );
 IEnumerable<ImportModel> duplicates = Ensure.Clause( () => { return mappings.Count > 0; }, () => { importModel.Error = "Error4"; } ) 
 .Run( () => { return mappings.GroupBy( x => xProductSku).SelectMany( g => g.Skip(1).ToList()); } );
 bool isImported = Ensure.Clause( () => { return duplicates.Any(); }, () => { importModel.Error = "Error2"; } )
 .AndClause( () => { return importModel.InvalidSkuList.Any(); }, "" )
 .Run( () => { return _productService.Import(mappings); } );
 
 if ( isImported ) 
 {
 importModel.Error = "Error3";
 }
} 
catch( ArgumentException e) 
{
 importModel.Error = e.Message;
}
return importModel;

A possible implementation for leveraging paring method calls with sanity tests.

using System;
using System.Collections.Generic;
class Ensure
{
 private List<Tuple<Func<bool>, string>> tuples;
 private Ensure(Func<bool> filter, string message)
 {
 tuples = new List<Tuple<Func<bool>, string>>
 {
 Tuple.Create<Func<bool>, string>(filter, message)
 };
 }
 public static Ensure Clause(Func<bool> filter, string message)
 {
 return new Ensure(filter, message);
 }
 public Ensure AndClause(Func<bool> filter, string message)
 {
 tuples.Add( Tuple.Create<Func<bool>, string>(filter, message) );
 return this;
 }
 public T Run<T>(Func<T> toRun)
 {
 foreach ( Tuple<Func<bool>, string> filter in tuples )
 {
 if ( filter.Item1() == false )
 {
 throw new ArgumentException( filter.Item2 );
 }
 }
 return toRun();
 }
}

• Billal BEGUERADJ: the aim is to review the existing solution
• pacmaninbw: Welcome to the Code Review Community. While this might be a good answer on stackoverflow, it is not a good answer on Code Review. A good answer on Code Review contains at least one insightful observation about the code. Alternate code only solutions are considered poor answers and may be down voted or deleted by the community. Please read How do I write a good answer.