Looking up ingredients and creating a shopping list

Question

This is my first attempt at refactoring - FindIngredients and I'm wondering where I can improve. The github link in the code directs to the project that still has the old code.

The user has data validated selections to make in cells B2 through H12 on Sheets("Plan"). Each type of meal has certain selections from the ingredient tabs Sheets("Breakfast", "Lunch", "Dinner", "Snacks"). Each food item is listed in column A of those sheets with ingredients listed on the same row in subsequent columns.

For visual -

Once the user submits the meal plan, this creates a shopping list.

This is all in one module, but I broke the two subs apart so they can be read alone.

Option Explicit
'==========================================
'MIT License
'Copyright (c) <2016> <Raymond Wise> <https://github.com/RaymondWise/Excel-Weekly-Meal-Plan-Shopping-List-Creator> @raymondwise
'==========================================

Public Counter As Long

Public Sub populate_shoppinglist()

'The sheets and ranges need variables as Worksheet Type and Range Type cannot be constants
Dim PlanSheet As Worksheet
Set PlanSheet = Sheets("Plan")

Dim ShoppingSheet As Worksheet
Set ShoppingSheet = Sheets("Shopping")

Dim BreakfastSheet As Worksheet
Set BreakfastSheet = Sheets("Breakfast")

Dim LunchSheet As Worksheet
Set LunchSheet = Sheets("Lunch")

Dim DinnerSheet As Worksheet
Set DinnerSheet = Sheets("Dinner")

Dim SnacksSheet As Worksheet
Set SnacksSheet = Sheets("Snacks")

Dim BreakfastArea As Range
Set BreakfastArea = PlanSheet.Range("B2:H4")

Dim SnackAreaAM As Range
Set SnackAreaAM = PlanSheet.Range("B5:H5")

Dim LunchArea As Range
Set LunchArea = PlanSheet.Range("B6:H8")

Dim SnackAreaPM As Range
Set SnackAreaPM = PlanSheet.Range("B9:H9")

Dim DinnerArea As Range
Set DinnerArea = PlanSheet.Range("B10:H12")

Dim ListArea As Range
Set ListArea = PlanSheet.Range("B14:H29")
ListArea.ClearContents

Dim Ingredients() As String
Dim ListLastRow As Long
Dim ArrayItem As Long
Dim ListColumn As Long
Dim ListRow As Long

'The counter keeps track of the current row on the ShoppingSheet as we compile the list from the other sheets
Counter = 1

Call FindIngredients(BreakfastSheet, BreakfastArea, Counter)
Call FindIngredients(SnacksSheet, SnackAreaAM, Counter)
Call FindIngredients(LunchSheet, LunchArea, Counter)
Call FindIngredients(SnacksSheet, SnackAreaPM, Counter)
Call FindIngredients(DinnerSheet, DinnerArea, Counter)

'Many food items have the same ingredients
ShoppingSheet.Range("A:A").RemoveDuplicates Columns:=1, Header:=xlNo

'Essentially checking for no selections on PlanSheet but checking ListSheet is easier because of the data validation
    If IsEmpty(ShoppingSheet.Range("A1")) Then
        MsgBox ("No selections")
    Exit Sub
    End If

ListLastRow = ShoppingSheet.Cells(Rows.Count, 1).End(xlUp).Row
ReDim Ingredients(1 To ListLastRow)

    For ArrayItem = 1 To ListLastRow
        Ingredients(ArrayItem) = ShoppingSheet.Cells(ArrayItem, 1)
    Next

'Reusing an already declared variable
Counter = 1
ListColumn = 2

Populate:
    On Error GoTo Finish
    For ListRow = 14 To 29
        PlanSheet.Cells(ListRow, ListColumn) = Ingredients(Counter)
        Counter = Counter + 1
    Next

    If Counter - 1 < ListLastRow Then
        ListColumn = ListColumn + 1
        GoTo Populate
    End If

Finish:
    ShoppingSheet.Range("A:A").Clear

End Sub

---

Public Sub FindIngredients(ByVal IngredientSheet As Worksheet, ByVal FoodRange As Range, ByRef Counter As Long)
'This subroutine takes all of the selections in an area and finds the ingredients for each selection
Dim ListSheet As Worksheet
Set ListSheet = ThisWorkbook.Sheets("Shopping")

Dim FoodSelection As Range
Dim Ingredient As Range

Dim ColumnNumber As Long
Dim RowNumber As Long
Dim ColumnCounter As Long

 For Each FoodSelection In FoodRange

        If FoodSelection.Value <> "" Then
            Set Ingredient = IngredientSheet.Range("A:A").Find(FoodSelection.Value, LookIn:=xlValues, lookat:=xlWhole)
            If Not Ingredient Is Nothing Then
                RowNumber = Ingredient.Row
                ColumnNumber = Ingredient.End(xlToRight).Column
                    For ColumnCounter = 2 To ColumnNumber
                        ListSheet.Cells(Counter, 1) = IngredientSheet.Cells(RowNumber, ColumnCounter)
                        Counter = Counter + 1
                    Next ColumnCounter
            End If
        End If
    Next FoodSelection
End Sub

Edit: As of Office 2016, this no longer works on OSX because the RemoveDuplicates method just doesn't work. The github code has been updated to fix the issue.

Answer 1

'==========================================
'MIT License
'Copyright (c) <2016> <Raymond Wise> <https://github.com/RaymondWise/Excel-Weekly-Meal-Plan-Shopping-List-Creator> @raymondwise
'==========================================

Meh. Include a license.md file in your GitHub repository, and remove these comments - they just clutter up your code files IMO. Also as @Zak correctly pointed out, by posting on Stack Exchange you've just sub-licensed it as CC-BY-SA. Since you own the code and all rights to it, it's not a problem, but it's good to know nonetheless.

---

'The sheets and ranges need variables as Worksheet Type and Range Type cannot be constants

But they are!

Dim PlanSheet As Worksheet
Set PlanSheet = Sheets("Plan")

Dim ShoppingSheet As Worksheet
Set ShoppingSheet = Sheets("Shopping")

Dim BreakfastSheet As Worksheet
Set BreakfastSheet = Sheets("Breakfast")

Dim LunchSheet As Worksheet
Set LunchSheet = Sheets("Lunch")

Dim DinnerSheet As Worksheet
Set DinnerSheet = Sheets("Dinner")

Dim SnacksSheet As Worksheet
Set SnacksSheet = Sheets("Snacks")

Worksheets have a CodeName property.

Give each of these sheets a proper (Name), and then you don't need any of these global variables, VBA creates a global identifier for you already, that points to these exact object references you're creating.

---

Call FindIngredients(BreakfastSheet, BreakfastArea, Counter)
Call FindIngredients(SnacksSheet, SnackAreaAM, Counter)
Call FindIngredients(LunchSheet, LunchArea, Counter)
Call FindIngredients(SnacksSheet, SnackAreaPM, Counter)
Call FindIngredients(DinnerSheet, DinnerArea, Counter)

Call is obsolete. This code is completely equivalent:

FindIngredients BreakfastSheet, BreakfastArea, Counter
FindIngredients SnacksSheet, SnackAreaAM, Counter
FindIngredients LunchSheet, LunchArea, Counter
FindIngredients SnacksSheet, SnackAreaPM, Counter
FindIngredients DinnerSheet, DinnerArea, Counter

Counter must be passed by reference here, because FindIngredients will alter the value and return it to the caller. This code would break it:

FindIngredients BreakfastSheet, BreakfastArea, (Counter)
FindIngredients SnacksSheet, SnackAreaAM, (Counter)
FindIngredients LunchSheet, LunchArea, (Counter)
FindIngredients SnacksSheet, SnackAreaPM, (Counter)
FindIngredients DinnerSheet, DinnerArea, (Counter)

I would make FindIngredients a Function instead, pass Counter by value, and document that it's returning the counter value back to the caller - this would turn the call sites into this:

Counter = FindIngredients(BreakfastSheet, BreakfastArea, Counter)
Counter = FindIngredients(SnacksSheet, SnackAreaAM, Counter)
Counter = FindIngredients(LunchSheet, LunchArea, Counter)
Counter = FindIngredients(SnacksSheet, SnackAreaPM, Counter)
Counter = FindIngredients(DinnerSheet, DinnerArea, Counter)

That said, Counter isn't a very useful name - it even needs a comment to explain what it's used for:

'The counter keeps track of the current row on the ShoppingSheet as we compile the list from the other sheets
Counter = 1

Why is it declared at module scope? Its meaning is only ever useful inside the populate_shoppinglist procedure.. it should be declared in that scope.

Anyway, how about a more meaningful name for it?

Dim currentRow As Long
currentRow = 1

I'd deem the explanatory comment superfluous with a name like this.

---

The hard-coded ranges aren't ideal:

Dim BreakfastArea As Range
Set BreakfastArea = PlanSheet.Range("B2:H4")

Dim SnackAreaAM As Range
Set SnackAreaAM = PlanSheet.Range("B5:H5")

Dim LunchArea As Range
Set LunchArea = PlanSheet.Range("B6:H8")

Dim SnackAreaPM As Range
Set SnackAreaPM = PlanSheet.Range("B9:H9")

Dim DinnerArea As Range
Set DinnerArea = PlanSheet.Range("B10:H12")

Dim ListArea As Range
Set ListArea = PlanSheet.Range("B14:H29")

If the VBA code doesn't need to know what the actual cell addresses are, you can add an abstraction layer here in Excel, and introduce named ranges instead, so these "spreadsheet concerns" remain on the spreadsheet, not in your code:

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

Dim SnackAreaAM As Range
Set SnackAreaAM = PlanSheet.Range("SnakAreaAM")

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

Dim SnackAreaPM As Range
Set SnackAreaPM = PlanSheet.Range("SnakAreaPM")

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

Dim ListArea As Range
Set ListArea = PlanSheet.Range("ListArea")

---

The indentation is inconsistent, you have Dim, Redim and assignment statements left without indentation. Try to stick to code blocks:

Sub DoSomething()
....
....Dim Foo As Long
....On Error GoTo ErrHandler
....
....If Foo = 0 Then
........Foo = 42
....End If
....
....Debug.Assert Foo = 42
....
ErrHandler: 'VBE left-aligns line labels, cannot indent those
....Debug.Print Foo
End Sub