Рефакторинг метода со слишком большим количеством bool

Question

У меня есть этот метод в C #, и я хочу его рефакторинг.Просто слишком много булов и строк.Что будет лучшим рефакторингом.Создание нового класса кажется немного излишним, и сокращение просто в два кажется трудным.Любое понимание или указатель будет оценен.

метод рефакторинга

    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's 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's 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

Один из вариантов - это рефакторинг каждого из основных логических значений (canDeleteSires, deletingSearchGroup || wasSearchGroup) в методы с именами, которые описывают читаемую версию логики:

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

Затем вы инкапсулируете свой текущийБулева логика внутри этих методов: как вы передаете состояние (аргументы метода или члены класса) - дело вкуса.

Это удалит логическое значение из основного метода в меньшие методы, которые напрямую задают вопрос и отвечают на вопрос.Я видел этот подход, используемый в стиле «комментарии - зло».Честно говоря, я нахожу это немного излишним, если вы одинокий волк, но в команде это может быть намного легче читать.

Из личных предпочтений я бы также инвертировал ваше первое утверждение if, чтобы вернутьсярано, это уменьшит уровень отступа всего метода:

if (searchAllSireList)
{
    return result;
}

DataAccessDialog dlg = BeginWaitMessage();
bool isClose = false;
try
...

Но тогда вы можете быть наказаны толпой "множественные возвраты - зло".У меня складывается впечатление, что практика развития похожа на политику ...

Answer 2

Это небольшой рефактор для удаления отступов:

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's 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's 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

Может быть, вы можете попробовать удалить все комментарии. Переменные bool, которые вы используете, добавляют ценность для понимания кода, вы можете поместить несколько из них в строку для canDeleteSires, но я не думаю, что это добавит какое-либо значение.

С другой стороны, этот код находится в вашей форме, поэтому он может быть лучше в вашем контроллере, чтобы вы могли сохранять форму простой, а контроллер фактически контролировал поведение.

Answer 4

На самом деле, я бы лично оставил все как есть. Булевы значения, которые у вас есть повсюду, хотя и неэффективны, делают эту функцию удобочитаемой и понятной.

Вы можете сделать что-то вроде объединения всех bool-ов в одну строку (как показано ниже), однако это не так легко обслуживать, как написанное вами.

x = ((a & b) &! D) | е;