Finding the person with the biggest loss

By 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[0]);
                        }
                    }
                    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[0].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[0]} 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; }
    private readonly List<decimal> Amounts;

    public PersonWithMinLoss(string currentPersonData)
    {
        var parts = currentPersonData.Split(',');
        this.Amounts = parts[1..]
            .Select(_ => decimal.Parse(_, NumberStyles.Currency | NumberStyles.Number)).ToList();
        this.Name = parts[0];
        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[0])
                    .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[0], 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[0], // 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[0]
    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}");

Leave a Reply

Your email address will not be published. Required fields are marked *