Looking up ingredients and creating a shopping list

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) @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 w

'==========================================
'MIT License
'Copyright (c)    @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

StackExchange Code Review Q#117529, answer score: 7