3
\$\begingroup\$

This method splits up two comma separated items, if they exist, and increments the amount if the are already in the list; otherwise add the item to the list

I want to know if this code can be optimized some more. Any suggestions?

The method below works 100% fine but I feel like it is not neatly done.

private void SeperateJoinedDisposables(IEnumerable<Models.Container> joinedDisposables)
{
    if (!joinedDisposables.Any())
        return;

    foreach (var item in joinedDisposables.ToList())
    {
        var amount = item.Amount;
        var container = item.Container;

        var disposable1 = container.Substring(0, container.IndexOf(","));
        var disposable2 = container.Substring(container.IndexOf(",") + 1);

        DisposablesModel.Containers.Remove(item);

        var disposableCointians1 = DisposablesModel.Containers.FirstOrDefault(x => x.Container.ToUpper().Contains(disposable1.ToUpper()));
        var disposableCointians2 = DisposablesModel.Containers.FirstOrDefault(x => x.Container.ToUpper().Contains(disposable2.ToUpper()));

        if (disposableCointians1 != null)
            disposableCointians1.Amount = (int.Parse(disposableCointians1.Amount) + int.Parse(amount)).ToString();
        else
            DisposablesModel.Containers.Add(new Models.Container { Amount = amount, Container = disposable1 });

        if (disposableCointians2 != null)
            disposableCointians2.Amount = (int.Parse(disposableCointians2.Amount) + int.Parse(amount)).ToString();
        else
            DisposablesModel.Containers.Add(new Models.Container { Amount = amount, Container = disposable2 });
    }
}

Here is my model class

    public class DisposablesModel
    {
        public string Barcode { get; set; }
        public string RushColor { get; set; }
        public string RushDescription { get; set; }
        public string Pollution { get; set; }
        public int NumberOfSamples { get; set; }
        public List<Container> Containers { get; set; }
        public bool IsSubcontracted { get; set; }
    }

    public class Container
    {
        public string Amount { get; set; }
        public string Container { get; set; }
    }

| improve this question | | | | |
\$\endgroup\$
  • \$\begingroup\$ is items.Container a typo? item is of type Models.Container, therefore it can't have a member of the same name. \$\endgroup\$ – tinstaafl Jan 30 at 23:27
  • 1
    \$\begingroup\$ It would be great question if you just provided a sample input and include Models.Container class as well. \$\endgroup\$ – iSR5 Jan 31 at 8:28
  • \$\begingroup\$ I've included the model class in the edited version. @iSR5 \$\endgroup\$ – Raldo Tromp Feb 7 at 11:57
1
\$\begingroup\$

Some quick remarks:

  • The class "Container" has a property named "Container"? That is... confusing, to say the least. Hence you having to name it item when iterating through it, which is unclear (and thus bad).

  • "Cointians" has two typos.

  • If you need to have a case insensitive Contains(), there are better ways to do it than use ToUpper(): https://stackoverflow.com/a/54686459/648075

  • Do not copy-paste logic when it is the same for two cases; instead move it to a method and call it.

  • Why is the property Amount a string instead of an int? Your code constantly needs to converts strings to ints and vice versa; why not use the proper types instead and avoid all that?

  • Is the Container of a Container always a comma-separated string of two items? Why then not make it a List<string> Names?


Most impotantly: due to the bad names, I have trouble figuring out what your code actually does.

That joinedDisposables is a list of Containers is unclear, and due to "Container" being both the name of a class as well as one of its properties the code becomes even more unclear.

I have the impression that once your Container class has a "proper" structure, much of this code can be replaced by simple LINQ queries; it's just that the bad names and the bad structures obfuscate so much it makes things hard to grasp.

To me it looks like public List<Container> Containers { get; set; } should be a Dictionary<string, int>, and that what you pass to SeperateJoinedDisposables should be a List<string> where each string is a comma separated list. And then your code would be something like this:

Dictionary<string, int> Containers = new Dictionary<string, int>();

private void Parse(List<string> commaSeparatedValues)
{
    foreach (var commaSeparatedValue in commaSeparatedValues)
    {
        foreach (var value in commaSeparatedValue.Split(','))
        {
            var normalisedValue = value.ToLower();

            if (!Containers.TryGetValue(normalisedValue, out int count))
            {
                count = 0;
            }

            count++;
            Containers[normalisedValue] = count;
        }
    }
}

To me this code expresses far more clearly the intent of what you're trying to do -- if I correctly understand the purpose of your code, of course.

But then I don't know where IEnumerable<Models.Container> joinedDisposables comes from and how it is constructed and why it is constructed that way, so I can only assume a "better" solution based on limited knowledge.

| improve this answer | | | | |
\$\endgroup\$
  • \$\begingroup\$ IEnumerable<Models.Container> joinedDisposables comes from an Api call and can sometimes contain elements that are comma separated and if so it would never be more than 2 elements that is split by a comma. \$\endgroup\$ – Raldo Tromp Feb 10 at 9:42

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.