Refactoring a method with too many bools in it

Question

I have this method in C # and I want to reorganize it. Too many bolts and lines. What will be the best refactoring. Creating a new class seems a little redundant, and cutting just in two seems complicated. Any insight or pointer would be appreciated.

refactoring method

private DialogResult CheckForSireRestrictionInSubGroup(bool deletingGroup,string currentId) { DialogResult result = DialogResult.No; if (!searchAllSireList) { DataAccessDialog dlg = BeginWaitMessage(); bool isClose = false; try { ArrayList deletedSire = new ArrayList(); ISireGroupBE sireGroupBE = sireController.FindSireGroupSearch(); if (sireGroupBE != null) { //if the current group is in fact the seach group before saving bool currentGroupIsSeachGroup = sireGroupBE.TheSireGroup.id == currentId; //if we have setting this group as search group bool selectedAsSearchGroup = this.chkBoxSelectedSireGroup.Checked; //if the group we currently are in is not longer the seach group(chk box was unchecked) bool wasSearchGroup = currentGroupIsSeachGroup && !selectedAsSearchGroup; //if the group is becoming the search group bool becomesSearchGroup = !currentGroupIsSeachGroup && selectedAsSearchGroup; //if the group being deleted is in fact the search group bool deletingSearchGroup = deletingGroup && currentGroupIsSeachGroup; //if the user checked the checkbox but he deleting it, not a so common case, but //we shouldn't even consider to delete sire in this case bool deletingTemporarySearchGroup = deletingGroup && !currentGroupIsSeachGroup; //if we are not deleting a temporary search group and it either //becoming one (without deleting it) or we already are the search group bool canDeleteSires = !deletingTemporarySearchGroup && (becomesSearchGroup || currentGroupIsSeachGroup); //we only delete sires if we are in search group if (canDeleteSires) { if (deletingSearchGroup || wasSearchGroup) { // If we deleted all sires deletedSire = new ArrayList(); deletedSire.AddRange( sireGroupBE.SireList); } else { //if we delete a few sire from the change of search group deletedSire = GetDeleteSire(sireGroupBE.SireList); } } EndWaitMessage(dlg); isClose = true; result = ShowSubGroupAffected(deletedSire); } } finally { if (!isClose) { EndWaitMessage(dlg); } } } return result; } 

Answer 1

One option is to reorganize each of the core logic elements ( canDeleteSires , deletingSearchGroup || wasSearchGroup ) into methods with names that describe the readable version of the logic:

 if (WeAreInSearchGroup()) { if (WeAreDeletingAllSires()) { deletedSire = new ArrayList(); deletedSire.AddRange( sireGroupBE.SireList); } else { deletedSire = GetDeleteSire(sireGroupBE.SireList); } } 

Then you encapsulate your current logical logic inside these methods, how you pass the state (method arguments or class members) is a matter of taste.

This will remove the boolean elements from the main method into smaller methods that ask and answer the question directly. I saw this approach used in the style of "comments - evil." Honestly, I find this a bit crowded if you are a lone wolf, but on a team it is much easier to read.

From personal preferences, I would also invert your first if statement to return earlier, this will reduce the indentation level of the whole method:

 if (searchAllSireList) { return result; } DataAccessDialog dlg = BeginWaitMessage(); bool isClose = false; try ... 

But then you can be punished by a mob of "multiple returns of evil." I have developed the practice of developing an impression, as in politics ...

Answer 2

This is a little refactoring to remove some indentation:

 private DialogResult CheckForSireRestrictionInSubGroup(bool deletingGroup,string currentId) { if (searchAllSireList) return DialogResult.No; DataAccessDialog dlg = BeginWaitMessage(); bool isClose = false; try { ISireGroupBE sireGroupBE = sireController.FindSireGroupSearch(); if (sireGroupBE == null) return DialogResult.No; //if the current group is in fact the seach group before saving bool currentGroupIsSeachGroup = sireGroupBE.TheSireGroup.id == currentId; //if we have setting this group as search group bool selectedAsSearchGroup = this.chkBoxSelectedSireGroup.Checked; //if the group we currently are in is not longer the seach group(chk box was unchecked) bool wasSearchGroup = currentGroupIsSeachGroup && !selectedAsSearchGroup; //if the group is becoming the search group bool becomesSearchGroup = !currentGroupIsSeachGroup && selectedAsSearchGroup; //if the group being deleted is in fact the search group bool deletingSearchGroup = deletingGroup && currentGroupIsSeachGroup; //if the user checked the checkbox but he deleting it, not a so common case, but //we shouldn't even consider to delete sire in this case bool deletingTemporarySearchGroup = deletingGroup && !currentGroupIsSeachGroup; //if we are not deleting a temporary search group and it either //becoming one (without deleting it) or we already are the search group bool canDeleteSires = !deletingTemporarySearchGroup && (becomesSearchGroup || currentGroupIsSeachGroup); ArrayList deletedSire = new ArrayList(); //we only delete sires if we are in search group if (canDeleteSires) { if (deletingSearchGroup || wasSearchGroup) { // If we deleted all sires deletedSire.AddRange(sireGroupBE.SireList); } else { //if we delete a few sire from the change of search group deletedSire = GetDeleteSire(sireGroupBE.SireList); } } EndWaitMessage(dlg); isClose = true; return ShowSubGroupAffected(deletedSire); } finally { if (!isClose) { EndWaitMessage(dlg); } } return DialogResult.No; } 

Answer 3

Actually, I personally would leave it as it is. The booleans that you are everywhere, although ineffective, make this function clear and comprehensible.

You can do something like combining all bools into one line (for example, below), however this is not as convenient as what you wrote.

x = ((a, b) and! d) | e;

Answer 4

Perhaps you can try to delete all comments. The bool variables that you have add a value for understanding the code, you can add a few of them inline for canDeleteSires, but I do not think this will add any value.

This code, on the other hand, is in your form, so it might be better in your controller, so you can keep the form simple, and the controller actually controls the behavior.