0
\$\begingroup\$

I have a class that stores some data and two methods that set this data from strings and convert it back to a string.

I call AddDetail(string detail) when reading multiple lines of a text file, so it gets called many times.

I don't like multiple if and else if statements and I use loops whenever possible.

Is it possible to use loops in this case or somehow optimize these methods?

using System;
using System.Text;

public class Details
{
    public DateTime? Date { get; set; }
    public string? Address { get; set; }
    public string? Phone { get; set; }
    public Uri? Email { get; set; }
    public Uri? Website { get; set; }
    public TimeOnly? OpenFrom { get; set; }
    public TimeOnly? OpenTill { get; set; }

    public void AddDetail(string detail)
    {
        if (detail.StartsWith("- Date: "))
        {
            if (DateTime.TryParse(detail.Replace("- Date: ", string.Empty), out DateTime date))
            {
                Date = date;
            }
        }
        else if (detail.StartsWith("- Address: "))
        {
            Address = detail.Replace("- Address: ", string.Empty);
        }
        else if (detail.StartsWith("- Phone: "))
        {
            Phone = detail.Replace("- Phone: ", string.Empty);
        }
        else if (detail.StartsWith("- Email: "))
        {
            if (Uri.TryCreate(detail.Replace("- Email: ", string.Empty), UriKind.Absolute, out Uri? uri))
            {
                Email = uri;
            }
        }
        else if (detail.StartsWith("- Website: "))
        {
            if (Uri.TryCreate(detail.Replace("- Website: ", string.Empty), UriKind.Absolute, out Uri? uri))
            {
                Website = uri;
            }
        }
        else if (detail.StartsWith("- Open: "))
        {
            string open = detail.Replace("- Open: ", string.Empty);

            string[] fromTill = open.Split('-');

            if (fromTill.Length == 2)
            {
                if (TimeOnly.TryParse(fromTill[0].Trim(), out TimeOnly from))
                {
                    OpenFrom = from;
                }

                if (TimeOnly.TryParse(fromTill[1].Trim(), out TimeOnly till))
                {
                    OpenTill = till;
                }
            }
        }
    }

    public override string ToString()
    {
        StringBuilder stringBuilder = new();

        if (Date is not null)
            stringBuilder.AppendLine("- Date: " + Date);

        if (Address is not null)
            stringBuilder.AppendLine("- Address: " + Address);

        if (Phone is not null)
            stringBuilder.AppendLine("- Phone: " + Phone);

        if (Email is not null)
            stringBuilder.AppendLine("- Email: " + Email);

        if (Website is not null)
            stringBuilder.AppendLine("- Website: " + Website);

        if (OpenFrom is not null && OpenTill is not null)
            stringBuilder.AppendLine($"- Open: {OpenFrom} - {OpenTill}");

        return stringBuilder.ToString();
    }
}
```
\$\endgroup\$
0

1 Answer 1

2
\$\begingroup\$

Usually big chain of if condition is a code smell and polymorphism is usually the solution. I suggest using it in combination with immutability - then you don't "parse", but create the object from the string. If not, I would add builder pattern and use polymorphism (explained below) only for the builder, that will in the end create/modify your class as it exists now.

Create interface of your class, possibly with abstract class too and then implementation for each possibility, for example DateDetails, etc. Your parse method will become constructor rather than modifier, your properties all become read-only if possible. Then you need a factory, which decides based on your detail string, what constructor to call and what instance to create (this will be basically iterating over a loop).

I should note, that your whole design is weird, since you are in each case filling only 1-2 fields of the whole class, so it is always almost empty.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you for your suggestion. I call AddDetail(string detail) when reading multiple lines of a text file, so it gets called many times. \$\endgroup\$
    – user266115
    Oct 22, 2022 at 6:45
  • 1
    \$\begingroup\$ That's useful to know. Could be useful to add sample of this file for more context and to be able to make it run on sample data. \$\endgroup\$
    – K.H.
    Oct 22, 2022 at 9:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.