Извлекать общую и дисплей продуктов питания


Вот это первая итерация проекта на CodeReview

Примечание: вход meal данные проверены - он не может быть то, что не существует на лист поиск. Листы всех зовут, как и именованные диапазоны.

Качестве (в формате Excel) пользователь выбирает из пищи, а затем генерирует список покупок PopulateShoppingList().

Это выбор, смотрит их на соответствующий лист, собирает ингредиенты и обеспечивает нет повторяющихся ингредиентов.

Все это работает, но я чувствую, что делаю for each rng in rng не самая лучшая вещь. Мне тоже не нравится, как я использовал метки в GetIngredientsплюс стрелки-код там тоже есть.

Я также создаю массив ингредиентов в PopulateShoppingList() но я никогда не использовать его там, я только его сдать ByRef С другими процедурами, что имеет смысл в моей голове, но я не уверен, как это выглядит с точки зрения кода. Честно, это было около 9 месяцев с момента написания какого-либо кода.

Option Explicit

Public Sub PopulateShoppingList()

    Dim BreakfastArea As Range
    Set BreakfastArea = wsPlan.Range("BreakfastArea")

    Dim SnackAreaAM As Range
    Set SnackAreaAM = wsPlan.Range("SnacksAreaAM")

    Dim LunchArea As Range
    Set LunchArea = wsPlan.Range("LunchArea")

    Dim SnackAreaPM As Range
    Set SnackAreaPM = wsPlan.Range("SnacksAreaPM")

    Dim DinnerArea As Range
    Set DinnerArea = wsPlan.Range("DinnerArea")

    Dim ListArea As Range
    Set ListArea = wsPlan.Range("ListArea")
    ListArea.ClearContents

    Dim mealSelection As Range
    Dim mealName As String
    Dim IngredientList As Variant
    ReDim IngredientList(1, 0)

    For Each mealSelection In BreakfastArea
        If Not mealSelection = vbNullString Then
            mealName = mealSelection.Value
            GetIngredients wsBreakfast, mealName, IngredientList
        End If
    Next

    For Each mealSelection In LunchArea
        If Not mealSelection = vbNullString Then
            mealName = mealSelection.Value
            GetIngredients wsLunch, mealName, IngredientList
        End If
    Next

    For Each mealSelection In DinnerArea
        If Not mealSelection = vbNullString Then
            mealName = mealSelection.Value
            GetIngredients wsDinner, mealName, IngredientList
        End If
    Next

    For Each mealSelection In SnackAreaAM
        If Not mealSelection = vbNullString Then
            mealName = mealSelection.Value
            GetIngredients wsSnacks, mealName, IngredientList
        End If
    Next

    For Each mealSelection In SnackAreaPM
        If Not mealSelection = vbNullString Then
            mealName = mealSelection.Value
            GetIngredients wsSnacks, mealName, IngredientList
        End If
    Next

    If IsEmpty(IngredientList(0, 0)) Then Exit Sub
    WriteShoppingList IngredientList

End Sub
Private Sub WriteShoppingList(ByVal IngredientList As Variant)
    Const LIST_FIRST_ROW As Long = 14
    Const LIST_LAST_ROW As Long = 29
    Const LIST_FIRST_COLUMN As Long = 2
    Const LIST_LAST_COLUMN As Long = 8
    Dim arrayIndex As Long
    Dim listItem As String

    arrayIndex = 0
    Dim rowIndex As Long
    rowIndex = LIST_FIRST_ROW
    Dim columnIndex As Long
    columnIndex = LIST_FIRST_COLUMN
    With wsPlan
        For arrayIndex = LBound(IngredientList, 2) To UBound(IngredientList, 2)
            listItem = IngredientList(1, arrayIndex) & " " & IngredientList(0, arrayIndex)
            If rowIndex > LIST_LAST_ROW Then
                columnIndex = columnIndex + 1
                rowIndex = LIST_FIRST_ROW
                If columnIndex > LIST_LAST_COLUMN Then Exit Sub
            End If

            .Cells(rowIndex, columnIndex) = listItem
            rowIndex = rowIndex + 1
        Next
    End With
End Sub

Private Sub GetIngredients(ByVal targetSheet As Worksheet, ByVal mealName As String, ByRef IngredientList As Variant)
    Dim rowIndex As Long
    Dim mealIndex As Long
    Dim arrayIndex As Long
    Dim sheetLastRow As Long
    Dim mealLastRow As Long
    With targetSheet
        sheetLastRow = .Cells(.Rows.Count, 2).End(xlUp).Row
        For rowIndex = 2 To sheetLastRow
            If targetSheet.Cells(rowIndex, 1) = mealName Then
                mealLastRow = .Columns(1).Find(what:="*", after:=.Cells(rowIndex, 1), LookIn:=xlValues).Row
                For mealIndex = rowIndex To mealLastRow - 1
                    If IsEmpty(IngredientList(0, 0)) Then GoTo Immediate
                       For arrayIndex = LBound(IngredientList, 2) To UBound(IngredientList, 2)
                           If IngredientList(0, arrayIndex) = .Cells(mealIndex, 2) Then
                               IngredientList(1, arrayIndex) = IngredientList(1, arrayIndex) + .Cells(mealIndex, 3)
                               GoTo NewIngredient
                           End If
                        Next arrayIndex
                    ReDim Preserve IngredientList(1, UBound(IngredientList, 2) + 1)
Immediate:
                    IngredientList(0, UBound(IngredientList, 2)) = .Cells(mealIndex, 2)
                    IngredientList(1, UBound(IngredientList, 2)) = .Cells(mealIndex, 3)
NewIngredient:
                Next mealIndex
                Exit Sub
            End If
        Next rowIndex
    End With
End Sub


416
3
задан 5 февраля 2018 в 02:02 Источник Поделиться
Комментарии
2 ответа

Повторные циклы прыгнул на меня.

For Each mealSelection In SOME_RANGE
If Not mealSelection = vbNullString Then
mealName = mealSelection.Value
GetIngredients SOME_SHEET, mealName, IngredientList
End If
Next

Потянув ее в свою собственную процедуру, мы знаем, что с 2 параметрами и доброго имени, мы значительно улучшилась PopulateShoppingList.


Осторожно здесь:


             If IsEmpty(IngredientList(0, 0)) Then GoTo Immediate
For arrayIndex = LBound(IngredientList, 2) To UBound(IngredientList, 2)

Запустив его через Rubberduck / умный Индентора будет такой:

               If IsEmpty(IngredientList(0, 0)) Then GoTo Immediate
For arrayIndex = LBound(IngredientList, 2) To UBound(IngredientList, 2)

И следовательно:

               Next arrayIndex
ReDim Preserve IngredientList(1, UBound(IngredientList, 2) + 1)


Это:


 ReDim Preserve IngredientList(1, UBound(IngredientList, 2) + 1)

В петлю, значит, вы копируете весь массив более, для каждого подряд! Поскольку вы заранее знаете, сколько строк вам понадобится, это будет гораздо более эффективным, чтобы изменить его один раз.

Каждый цикл имеет свою цель - если вложенность их делает вещи трудно следовать, экстракт каждый цикл в свою собственную процедуру, передав необходимые параметры по:, что позволит повысить уровень абстракции и способствовать сокращению действительно жестокие гнездования =)

4
ответ дан 5 февраля 2018 в 03:02 Источник Поделиться

Для добавления в список.

Я обратил внимание на следующий фрагмент:

    If IsEmpty(IngredientList(0, 0)) Then Exit Sub
WriteShoppingList IngredientList
End Sub

Это может быть легко заменен:

    If Not IsEmpty(IngredientList(0, 0)) Then WriteShoppingList(IngredientList)
End Sub

Менее безусловные переходы (BreakС ExitС Gotoы) вы используете, тем надежнее ваш код может быть. Иногда это так же просто, как глядя на код потока и определения, если TRUE или FALSE это лучший способ, чтобы ветка.

Похожие рефакторинг может удалить ваши другие GOTOы (Immediate и NewIngredient). Может показаться тривиальным, но это даст вам больше контроля над вашим кодом, и сильнее, думая о движении, условия и причины, почему вы хотите вырваться из петли. Это также упростит уход не только за себя, но и для других.

У вас есть:

    With wsPlan
For arrayIndex = LBound(IngredientList, 2) To UBound(IngredientList, 2)
[...]
.Cells(rowIndex, columnIndex) = listItem
rowIndex = rowIndex + 1
Next
End With

Но только использовать "." когда-то (.Cells(rowIndex, columnIndex) = listItem). В этом случае вы можете легко использовать WS.Cells(rowIndex, columnIndex) = listItem и удалить один слой вложения.

3
ответ дан 5 февраля 2018 в 05:02 Источник Поделиться