CODEWITHSUNDEEP
c#design-patternsrefactoring
Manish Basantani
Manish Basantani

Reputation: 17499

Refactor my C# code : if-else statement and code repetition

Given:

private void UpdataFieldItems(List<FieldItemBase> existingFieldItems, List<FieldItemBase> selectedFieldItems)
    {
        List<FieldItemBase> newFieldItemsSelected;
        var fieldSelectionChanges = GetFieldSelectionChanges(out newFieldItemsSelected);//retuns a Flagged enum

        if (Coding.EnumHas(fieldSelectionChanges, FieldSelectionChanges.AddedFieldItem))
        {
            StartEditMode();
            SetColumnDescriptorsToAdd(newFieldItemsSelected);
            UpdateItems(selectedFieldItems);

            SetColumnsToShow();
            CustomizeAlignmentAndCellFormatters(_tfaTableGrid.TableGrid);

            if (_tfaTableGrid.TableGrid.ColumnDescriptors.Count() > 0)
            {
                SetAdditionalFirstGroupedColumn();
            }

            StopEditMode();
        }

        else if (Coding.EnumHas(fieldSelectionChanges, FieldSelectionChanges.Summary))
        {
            StartEditMode();

            UpdateItems(fieldItems);
            SetColumnsToShow();

            if (_tfaTableGrid.TableGrid.ColumnDescriptors.Count() > 0)
            {
                SetAdditionalFirstGroupedColumn();
            }

            StopEditMode();

        }

        else if (Coding.EnumHas(fieldSelectionChanges, FieldSelectionChanges.Order) ||
                 Coding.EnumHas(fieldSelectionChanges, FieldSelectionChanges.AddedCustomFieldItem) ||
                 Coding.EnumHas(fieldSelectionChanges, FieldSelectionChanges.RemovedItem))
        {
            UpdateItems(fieldItems);
            SetColumnsToShow();

        }
            Invalidate();
    }

//Coding.cs
public static bool EnumHas(FieldSelectionChanges selectionChanges, FieldSelectionChanges valueToCheck)
        {
            return (selectionChanges & valueToCheck) == valueToCheck;
        }

I am willing to refactor the above code. There are two things that i don't like about the code above:

1) Writing same method calls in different cases, its not being possible to pull out the common method calls from these cases.

2) The readability of this code is very bad. It would be very confusing to understand and debug, if later needed.

Can someone suggest a design pattern for this code? or some way to improve upon the above two concerns?

Thanks for your interest.

Upvotes: 1

Views: 1305

Answers (4)

MrDosu
MrDosu

Reputation: 3435

I would make the different conditions more expressive in terms of what it actually does to your application seeing you already use quite descriptive method names for the actions. Something like:

        private void UpdataFieldItems(List<FieldItemBase> existingFieldItems, List<FieldItemBase> selectedFieldItems)
        {
            List<FieldItemBase> newFieldItemsSelected;
            var fieldSelectionChanges = GetFieldSelectionChanges(out newFieldItemsSelected);//retuns a Flagged enum

            if (IsValidChange(fieldSelectionChanges))
            {
                List<FieldItemBase> targetfields = null;
                if (IsInEditMode(fieldSelectionChanges))
                    StartEditMode();

                if (IsItemAdded(fieldSelectionChanges))
                {
                    SetColumnDescriptorsToAdd(newFieldItemsSelected);
                    targetFields = selectedFieldItems;
                }
                else
                    targetFields = existingFieldItems;

                UpdateItems(targetFields);
                SetColumnsToShow();

                if (IsItemAdded(fieldSelectionChanges))
                    CustomizeAlignmentAndCellFormatters(_tfaTableGrid.TableGrid);

                if (IsInEditMode(fieldSelectionChanges))
                {
                    if (_tfaTableGrid.TableGrid.ColumnDescriptors.Count() > 0)
                        SetAdditionalFirstGroupedColumn();
                    StopEditMode();
                }
            }

            Invalidate();
        }

        private bool InEditMode(FlaggedEnum fieldSelectionChanges)
        {
            return Coding.EnumHas(fieldSelectionChanges, FieldSelectionChanges.Summary) || Coding.EnumHas(fieldSelectionChanges, FieldSelectionChanges.AddedFieldItem);
        }

        private bool IsItemAdded(FlaggedEnum fieldSelectionChanges)
        {
            Coding.EnumHas(Coding.EnumHas(fieldSelectionChanges, FieldSelectionChanges.AddedFieldItem);
        }

        private bool IsValidChange(FlaggedEnum fieldSelectionChanges)
        {
            return Coding.EnumHas(fieldSelectionChanges, FieldSelectionChanges.Order) ||
                   Coding.EnumHas(fieldSelectionChanges, FieldSelectionChanges.AddedCustomFieldItem) ||
                   Coding.EnumHas(fieldSelectionChanges, FieldSelectionChanges.RemovedItem) ||
                   Coding.EnumHas(fieldSelectionChanges, FieldSelectionChanges.Summary) ||
                   Coding.EnumHas(fieldSelectionChanges, FieldSelectionChanges.AddedFieldItem);
        }

        //Coding.cs
        public static bool EnumHas(FieldSelectionChanges selectionChanges, FieldSelectionChanges valueToCheck)
        {
            return (selectionChanges & valueToCheck) == valueToCheck;
        }

Upvotes: 1

gandjustas
gandjustas

Reputation: 1955

  1. Use extract method for body of each if statement
  2. Create Dictionary> to choose appropriate action for fieldSelectionChanges. This is Strategy pattern

Upvotes: 2

RPM1984
RPM1984

Reputation: 73102

Well the part that is repetitive/somewhat ugly is the IF statements.

Suggest holding the result of those IF conditions in a boolean variable, and leverage that.

This code isnt complete, but you get the idea.

        bool scenarioOne = Coding.EnumHas(fieldSelectionChanges, FieldSelectionChanges.AddedFieldItem);
        bool scenarioTwo = Coding.EnumHas(fieldSelectionChanges, FieldSelectionChanges.Summary);
        bool scenarioThree = Coding.EnumHas(fieldSelectionChanges, FieldSelectionChanges.Order) || Coding.EnumHas(fieldSelectionChanges, FieldSelectionChanges.AddedCustomFieldItem) || Coding.EnumHas(fieldSelectionChanges,FieldSelectionChanges.RemovedItem);

        if (scenarioOne || scenarioTwo)
            StartEditMode();

        if (scenarioOne) {
            SetColumnDescriptorsToAdd(newFieldItemsSelected);
            UpdateItems(selectedFieldItems);
        }
        else if (scenarioTwo || scenarioThree) {
            UpdateItems(fieldItems);
        }

        if (scenarioOne || scenarioTwo || scenarioThree)
            SetColumnsToShow();

Obviously, pick better names for the variable.

Or better yet, seperate out into seperate methods.

Upvotes: 1

Peter Lillevold
Peter Lillevold

Reputation: 33910

I would suggest extracting the three blocks into separate, well-named, methods. This will improve the readability if the UpdateFieldItems method. Since the three blocks are totally different, there is imo no way of consolidating these further.

Upvotes: 0

Related Questions