Adding or replacing a sheet in an Excel file

Question

I have the following function and want to simplify the code to make it more readable by eliminating redundant lines:

Function AddSheet(shtName As String, Optional readOnlyFlag As Boolean = True) As Boolean  'Add new worksheet to current workbook at the end

    If readOnlyFlag And IsSheet(shtName) Then
        AddSheet = False
        Exit Function
    ElseIf Not readOnlyFlag Then
        If IsSheet(shtName) Then DeleteSheet (shtName)
        With ThisWorkbook:
            .Sheets.Add(After:=.Sheets(.Sheets.Count)).Name = shtName
        End With
        AddSheet = True
    End If

End Function

Since the first condition (If readOnlyFlag And IsSheet(shtName)) will only assign a return value of False to the function (which should already be the default, as I understand) and then exits the function, would it be acceptable to leave out that aspect of the logic and just have the following:

    If Not readOnlyFlag Then
        If IsSheet(shtName) Then DeleteSheet (shtName)
        With ThisWorkbook:
            .Sheets.Add(After:=.Sheets(.Sheets.Count)).Name = shtName
        End With
        AddSheet = True
    End If

In other words, is it best practice to be explicit like the first code block, by stating that the function returns False if that occurs, or to avoid what might be considered redundancy, by using the simplified second version?

Answer 1

VBA does a lot of implicit things. And it can get confusing, and it can cause surprising bugs.

Your function is implicitly Public. Is it meant to be called from another module? Or is it only called from within the same module it's in? Being explicit about everything makes the intent of the code much easier to understand.

The parameters are implicitly passed ByRef, and yet, their values aren't reassigned in the body of the function (which is good!), so they could, and should, be passed ByVal. It's really a shame that ByRef is the implicit default in VB6/VBA.

The comment that's trailing after the function's signature, should be either immediately above, or immediately below the signature - avoid horizontal scrolling, and don't hide information from maintainers.

The comment could be clearer about why a Boolean return value is returned.

Private Function AddSheet(ByVal shtName As String, Optional ByVal readOnlyFlag As Boolean = True) As Boolean
'Appends a new worksheet to the active workbook. Returns True if sheet is successfully added.

It's not clear what readOnlyFlag does, even when reading the code. It probably could use a better name and/or a comment. It seems it determines whether the sheet should be replaced if it already exists?

The IsSheet function looks like it should be called IsExistingSheetName.

The instructions separator (colon) after With ThisWorkbook: serves no purpose and should be omitted; the Exit Function is redundant, too, since the code branches out of that block anyway.

There's an execution path where you're verifying whether the sheet name exists twice - I'd factor that check out of the If block.

As for the return value - I'd go even more explicit than that:

Dim result As Boolean

Dim isExisting As Boolean
isExisting = IsExistingSheetName(shtName)

If readOnlyFlag And isExisting Then
    result = False
ElseIf Not readOnlyFlag Then
    If isExisting Then DeleteSheet (shtName)
    With ThisWorkbook
        .Sheets.Add(After:=.Sheets(.Sheets.Count)).Name = shtName
    End With
    result = True
End If

AddSheet = result

Now, how helpful is that? How does the calling code access that new worksheet now? With Application.ActiveSheet? That's a dangerous assumption to make!

How about returning the Worksheet object instead? The function would return Nothing if no worksheet is added, and an object reference when it is.

Dim result As Worksheet

Dim isExisting As Boolean
isExisting = IsExistingSheetName(shtName)

If readOnlyFlag And isExisting Then
    'Set result = Nothing '' todo: remove this useless branch
ElseIf Not readOnlyFlag Then
    If isExisting Then DeleteSheet shtName
    With ThisWorkbook
        Set result = .Sheets.Add(After:=.Sheets(.Sheets.Count))
        If Not result Is Nothing Then result.Name = shtName
    End With
End If

Set AddSheet = result

Notice something? The code will blow up if .Add fails, because the function will return Nothing and then .Name cannot be set: you need to handle this.