ansaurus

Question

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

Answer 1

A:

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.

Peter Lillevold 2010-09-22 07:28:43

i was thinking that, but there is some common code. still, always better to put out into methods (if possible), i agree.

RPM1984 2010-09-22 07:32:24

Answer 2

+1 A:

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.

RPM1984 2010-09-22 07:30:38

+1: I wouldn't create four methods for the if blocks based on the posted code since it fairly clean and readable. If you start to add more statements to the if blocks then create four methods with descriptive names. Cleaning the if statement is a huge improvement.

AMissico 2010-09-22 07:40:28

Actually, IMO it is now become harder to "decode" what is actually done for each scenario. The if-statements are improved, yes. But the net execution for the various permutations of the flags? no.

Peter Lillevold 2010-09-22 09:35:56

@Peter Lillevold - we'll have to agree to disagree. I was under the impression this question was about refactoring for readbility/code reuse, in which i think my above answer helps. If we're talking about "execution" and performance of flags, well that's a seperate issue which was not highlighted by the OP to be of importance.

RPM1984 2010-09-22 10:01:59

@RPM1984 - no, I'm all with you on the readability/code reuse aspect, not thinking about actual performance. My point is that in your example the reader would still have to read *all* the if-blocks in order to understand what happens when the various flags are set. The "extract method" way avoids this problem since you isolate the logic for each case in each if-block.

Peter Lillevold 2010-09-22 12:09:21

Ok - i've got ya. =)

RPM1984 2010-09-22 12:16:02

I have accepted @MrDosu's answer. Yours is in the same lines but i found it more close to my preference. +1 for you. Thanks for the answer.

Amby 2010-09-24 05:36:15

No probs, thanks for the upvote.

RPM1984 2010-09-24 05:37:39

Answer 3

+1 A:

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

gandjustas 2010-09-22 07:32:27

Yeah use separate methods. Would make your life a LOT easier.

Jaco Pretorius 2010-09-22 07:49:06

Answer 4

+1 A:

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;
        }

MrDosu 2010-09-22 07:51:13

ansaurus

tags:

views:

answers:

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

related questions