CODEWITHSUNDEEP
c#refactoring
Funky
Funky

Reputation: 13612

How can I refactor a set of ugly if statements?

I have this set of If statements which work fine but just looks a little ugly. Is there any way I can tart it up instead of using big ole' ugly if statements? The problem is that it uses string.contains which makes it a bit difficult to implement a dictionary, which I tried and failed :(

Here it is:

foreach (var Item in Stuff)
            {
                var loweredQuoteItem = quoteItem.Name.ToLower();

                if (loweredQuoteItem.Contains("one"))
                    Item.InsurerNameShort = "Item One";

                if (loweredQuoteItem.Contains("two"))
                    Item.InsurerNameShort = "Item Two";

                if (loweredQuoteItem.Contains("chaucer"))
                    Item.InsurerNameShort = "Chaucer";

                if (loweredQuoteItem.Contains("three"))
                    Item.InsurerNameShort = "Item Three";

                if (loweredQuoteItem.Contains("four"))
                    Item.InsurerNameShort = "Item Four";

                if (loweredQuoteItem.Contains("five"))
                    Item.InsurerNameShort = "Item Five";

               }

Upvotes: 2

Views: 143

Answers (2)

Michael
Michael

Reputation: 1473

To simplify your method you can use inner Action. In this way your method will be looking like this

        Action<string,string, Action> method = (source, search, action) => {if (source.Contains(search)) action(); };
        method(loweredQuoteItem, "one", () => Item.InsurerNameShort = "Item One");
        method(loweredQuoteItem, "two", () => Item.InsurerNameShort = "Item Two");
        method(loweredQuoteItem, "chaucer", () => Item.InsurerNameShort = "Item Chaucer");

If without lambda and your logic is really so simple then you can move your IF statement inside different method:

    public void SetValueIfContains(string source, string search, string value, MyClass item)
    {
        if (source.Contains(search))
        {
            item.InsurerNameShort = value;
        }
    }

    public void YourFunction()
    {
        var loweredQuoteItem = quovteItem.Name.ToLower();
        SetValueIfContains(loweredQuoteItem, "one", "Item One", Item);
        SetValueIfContains(loweredQuoteItem, "two", "Item Two", Item);
        SetValueIfContains(loweredQuoteItem, "Chaucer", "Item chaucer", Item);
    }

If logic inside IF statement will be complex your can define ISrategy interface and implement strategies for each case. It is more best practice.

Upvotes: 2

Simon Forsberg
Simon Forsberg

Reputation: 13331

The problem is that it uses string.contains which makes it a bit difficult to implement a dictionary, which I tried and failed :(

Then I guess you implemented it incorrectly. Here's what you should do:

  • Construct a Dictionary.
  • Put "Item One" as value for key "one"
  • Put "Item Two" as value for key "two"
  • Put "Item Two" as value for key "chaucer"
  • etc...
  • Inside your current loop, once you have the loweredQuoteItem, loop over your Dictionary.
    • If dictionary contains innerLoopKey, set Item.InsurerNameShort to innerLoopValue (and optionally break)

Make sure to construct this Dictionary outside of your foreach (var Item in Stuff) loop, for better performance.

Upvotes: 3

Related Questions