# Finding the person with the biggest loss

Posted on

Problem

For a course I following and my code is approved I had to make some code that finds the person with the biggest loss where the input is a string[]

The string arrray looks like this :

``````{ "Tom, 1, 3, -1", "Gillie, 2, 3, 1", "Thor, 1000, 1001, 1002" };
``````

I solved it this way but it’s not a clean way. I must say we were not allowed to use classes. that will be the next chapter of the course.

My solution looks now like this :

`````` public static string FindPersonWithBiggestLoss(string[] peopleAndBalances)
{
if (IsInValidInput(peopleAndBalances))
{
return InValidOutput;
}

var highestLossEver = decimal.MinValue;
var personWithHighestLoss = new StringBuilder();

for (int i = 0; i <= peopleAndBalances.Length - 1; i++)
{
var currentPersonData = peopleAndBalances[i].Split(',');
var allAmounts = currentPersonData[1..];

// calculateLoss

if (allAmounts.Length <= 1)
{
return InValidOutput;
}

for (int j = 0; j < allAmounts.Length - 1; j++)
{
try
{
var amount1 = decimal.Parse(allAmounts[j], NumberStyles.Currency | NumberStyles.Number);
var amount2 = decimal.Parse(allAmounts[j + 1], NumberStyles.Currency | NumberStyles.Number);
var lossForCurrentPerson = amount1 - amount2;

//check if loss is greater then the current highest loss
if (lossForCurrentPerson > highestLossEver)
{
highestLossEver = lossForCurrentPerson;
personWithHighestLoss.Clear();
personWithHighestLoss.Append(currentPersonData);
}
}
catch (FormatException ex)
{
Console.WriteLine(\$"there is not a valid character in the database. Reason{ex}");
break;
}
}
}

if (personWithHighestLoss.ToString().Contains(','))
{
ReplaceCommaWithAnd(personWithHighestLoss);
}

return \$"{personWithHighestLoss} lost the most money. -¤{highestLossEver}.";
}
``````

it works but also I need a lot of nested control statements to make it work.
Is there a cleaner way to make this work ?

Solution

A few things I noticed:

Using the range option of the array index(`var allAmounts = currentPersonData[1..];`) doesn’t really help here. All that’s needed is to use the appropriate indexes of the original array.

It’s quite inefficient to use a `try` block to test if a string is a valid number. Using the `tryParse` method is much more efficient.

A hard coded currency symbol means that your output might not format properly on someone else’s machine. I would suggest using the format overload of the `ToString` method with the currency format.

I appears you’re using a `StringBuilder` to change the name of whichever person has the highest loss. If I’m not mistaken this will copy the string you already have. I would suggest just use the string array itself. This will be assigned by reference and won’t require recopying the string.
This:

``````if (personWithHighestLoss.Contains(','))
{
ReplaceCommaWithAnd(personWithHighestLoss);
}
``````

doesn’t really make much sense. The `Split` method already removes the commas. If you’re worried about extra commas use the `StringSplitOptions.RemoveEmptyEntries` option of the `Split` method.

Here’s what the modified code could look like:

``````public static string FindPersonWithBiggestLoss(string[] peopleAndBalances)
{
if (IsInValidInput(peopleAndBalances))
{
return InValidInput;
}

var highestLossEver = decimal.MinValue;
string[] personWithHighestLoss = new string[] { "" };

for (int i = 0; i <= peopleAndBalances.Length - 1; i++)
{
var currentPersonData = peopleAndBalances[i].Split(',',StringSplitOptions.RemoveEmptyEntries);

// calculateLoss

if (currentPersonData.Length < 4)
{
return InValidInput;
}

for (int j = 1; j < currentPersonData.Length - 1; j++)
{
var amount1 = 0.0M;
var amount2 = 0.0M;
if (!(decimal.TryParse(currentPersonData[j], NumberStyles.Currency | NumberStyles.Number, null, out amount1) &&
decimal.TryParse(currentPersonData[j + 1], NumberStyles.Currency | NumberStyles.Number, null, out amount2)))
{
Console.WriteLine(\$"there is not a valid character in the database");
}

var lossForCurrentPerson = amount1 - amount2;

//check if loss is greater then the current highest loss
if (lossForCurrentPerson > highestLossEver)
{
highestLossEver = lossForCurrentPerson;
personWithHighestLoss = currentPersonData;
}
}

}
return \$"{personWithHighestLoss} lost the most money. -{highestLossEver.ToString("C")}.";
}
``````

I wrote this code with a simpler version.
At first I created a class to store parsed information.

``````public class PersonWithMinLoss
{
public string Name { get; set; }
public decimal MaxLoss { get; set; }

public PersonWithMinLoss(string currentPersonData)
{
var parts = currentPersonData.Split(',');
this.Amounts = parts[1..]
.Select(_ => decimal.Parse(_, NumberStyles.Currency | NumberStyles.Number)).ToList();
this.Name = parts;
this.MaxLoss = GetMaxLoss();
}

private decimal GetMaxLoss()
{
decimal maxLoss = int.MinValue;

for (int i = 0; i < Amounts.Count - 1; i++)
{
var loss = Amounts[i] - Amounts[i + 1];
if (loss > maxLoss) maxLoss = loss;
}

return maxLoss;
}
}
``````

After that we just have to do several `Linq` queries. For this case

``````public static string FindPersonWithBiggestLoss(string[] peopleAndBalances)
{
var personsWithMaxLoss = peopleAndBalances.Select(_ => new PersonWithMinLoss(_));
var maxValue = personsWithMaxLoss.Max(_ => _.MaxLoss);
var personWithMaxLoss = personsWithMaxLoss.Where(x => (x.MaxLoss - maxValue) < 0.0001m).First();

return \$"{personWithMaxLoss.Name} lost the most money. -¤{personWithMaxLoss.MaxLoss}.";
}
``````

I stored amounts in `List<decimal> Amounts` so that we can use it after other operations(Not needed for this specific case).
And in `var personWithMaxLoss = personsWithMaxLoss.Where(x => (x.MaxLoss - maxValue) < 0.0001m).First();`, we can use `.ToList()` instead of `.First()` to find all the persons with max loss.

In this way, complexity is `O(n*m)` where n is number of person and m is the number of values each person has.

Alternatively we also can use aggregate funtion to find person of max loss.

``````var personsWithMaxLoss = peopleAndBalances.Select(_ => new PersonWithMinLoss(_));
var personWithMaxLoss = personsWithMaxLoss.Aggregate((cur, next) => (cur == null || cur.MaxLoss < next.MaxLoss) ? next : cur);
``````

A few things can be noted.

1. Find the largest value by using `Max()` which is a `LINQ` extension.
2. Don’t initialize integers variables with `MinValue` or `MaxValue` such as `decimal.MinValue`. Use a default value of `0` or `-1` (which are the common used ones` for better code management).
3. `IsInValidInput` is not presented in your post, but I assume it’s a method where you have validate the input based on your business needs, I just hoped that would be present as well to make the picture clearer.
4. Using `Try/Catch` block inside this method is unnecessary. The `Try/Catch` should be outside this scope.
5. Parsing string, then returning a string is a bad practice. You always need to parse once, and work through the parsed typed, then use `ToString()` whenever needed, this would create a string while keeping the actual source untouched. Using the current solution means that you’re parsing the values in every step in the application. If you see multiple parsing to the same data, try to unify it to make it parsed once, and let the application uses the actual types, then convert it to string whenever needed (like, sending it to database or over web request ..etc).
6. Use `decimal.TryParse` instead of `decimal.Parse` as you don’t know what values are passed to the method, so validate these values before parse them. Also, use `string.IsNullOrEmpty` since you’re working with `string`. It’s minor changes, but it would avoid invalid inputs. Even if it’s already validated in `IsInValidInput`, as long as the method is exposed, you should always consider the implement the method validation process.
7. Make use of generic classes such as `IEnumerable`. instead of accepting `string[]` you could converted to `IEnumerable<string>` which would extend your acceptable collections types, and would work with any collection that implements `IEnumerable<string>`. This would make your method flixable and adaptable to other classes out-of-the-box.
8. using `StringBuilder` is a good choice in your case, but you’re misused it when you added a string at the return statement. What you need to do is to keep using the `StringBuilder` save the results inside it, and then call `ToString()` on the return statement.

for the current work, where you get a string’s array and output one string containing the person information who has the biggest loss. You can simplify it to :

``````public static string FindPersonWithBiggestLoss(string[] peopleAndBalances)
{
if (IsInValidInput(peopleAndBalances)) { return InValidOutput; }

decimal max = 0;

var personWithHighestLoss = new StringBuilder();

for (int x = 0; x < peopleAndBalances.Length; ++x)
{
// convert string to array
var data = peopleAndBalances[x].Split(',');

// since index 0 would hold the person name, then we can start with index 1 to get balances
for (int i = 1; i < data.Length; i++)
{
if (decimal.TryParse(data[i], out decimal amount) && amount > max)
{
personWithHighestLoss.Clear();
personWithHighestLoss
.Append(data)
.Append(" lost the most money. -¤")
.Append(amount)
.Append('.');

max = amount;
}
}
}

return personWithHighestLoss.ToString();
}
``````

if you want a better version which would be more practical in actual work environment, then you would need to do something like this :

``````// Make use of `IEnumerable` and `KeyValuePair<string, decimal>`
public static IEnumerable<KeyValuePair<string, decimal>> FindPersonWithBiggestLoss(IEnumerable<string> peopleAndBalances)
{
if (IsInValidInput(peopleAndBalances)) { return InValidOutput;  }

foreach (var person in peopleAndBalances)
{
decimal max = 0;

// check the string array
if (!string.IsNullOrEmpty(person))
{
// convert string to array
var data = person.Split(',');

// since index 0 would hold the person name, then we can start with index 1 to get balances
for (int i = 1; i < data.Length; i++)
{
// check the string and validate if it's an integer
// if not valid, just ignore and get the next value.
if (decimal.TryParse(data[i], out decimal amount) && amount > max)
{
max = amount;
}
}

yield return new KeyValuePair<string, decimal>(data, max);
}
}
}
``````

this can be simplified using `Linq` to this :

``````public static IEnumerable<KeyValuePair<string, decimal>> FindPersonWithBiggestLoss(IEnumerable<string> peopleAndBalances)
{
if (IsInValidInput(peopleAndBalances)) { return InValidOutput;  }

foreach (var person in peopleAndBalances)
{
var data = person.Split(',');

yield return new KeyValuePair<string, decimal>(
data, // you can use Array.Find(data, x => !decimal.TryParse(x, out _)) as well.
data.Where(x => decimal.TryParse(x, out _)).Max(x => decimal.Parse(x))
);
}
}
``````

I would rather use the LINQ Query Syntax for simplicity and readability

``````var input = new List<string> { "Tom, 1, 3, -1", "Gillie, 2, 3, 1", "Thor, 1000, 1001, 1002" };

var personsWithLoss =
from pl in input
let splitted = pl.Split(",")
let person = splitted
let maxLoss = splitted.Skip(1).OrderByDescending(x=>x).First()
select new {person,maxLoss};

var personWithMaxLoss = personsWithLoss.OrderBy(x=>x.maxLoss).First();
Console.WriteLine(\$"{personWithMaxLoss.person} - {personWithMaxLoss.maxLoss}");
``````

Posted in C#Tagged