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.
