Simple Caesar Cipher Function

Question

For no particular reason, I wanted to create a function that would take a string and "encrypt" it via Caesar cipher. This function takes a string and shifts the letters left or right (in the alphabet) depending on the input. Shift right(2) for instance would be -

ABCDEFGHIJKLMNOPQRSTUVWXYZ
CDEFGHIJKLMNOPQRSTUVWXYZAB

The UDF:

Option Explicit

Public Function CaesarCipher(ByVal TextToEncrypt As String, ByVal CaesarShift As Long) As String

    'Positive means shift to right e.g. "A" Shift 1 returns "B"
    Dim IsPositive As Boolean
    IsPositive = True
    If CaesarShift < 0 Then IsPositive = False
    CaesarShift = Abs(CaesarShift)

    Dim OutputText As String
    TextToEncrypt = UCase(TextToEncrypt)

    If CaesarShift > 26 Then
        CaesarShift = CaesarShift Mod 26
    End If

    If IsPositive Then
          OutputText = ShiftRight(TextToEncrypt, CaesarShift)
    Else: OutputText = ShiftLeft(TextToEncrypt, CaesarShift)
    End If

    CaesarCipher = OutputText
End Function

The shifting functions:

Private Function ShiftRight(ByVal ShiftString As String, ByVal ShiftQuantity As Long) As String

    Dim TextLength As Long
    TextLength = Len(ShiftString)

    Dim CipherText As String
    Dim CharacterCode As Long
    Dim AsciiIndex As Long
    Dim AsciiIdentifier() As Long
    ReDim AsciiIdentifier(1 To TextLength)

    For AsciiIndex = 1 To TextLength
        CharacterCode = Asc(Mid(ShiftString, AsciiIndex, 1))
        If CharacterCode + ShiftQuantity > 90 Then
            CharacterCode = CharacterCode - 26 + ShiftQuantity
        ElseIf CharacterCode = 32 Then GoTo Spaces
        Else:  CharacterCode = CharacterCode + ShiftQuantity
        End If
Spaces:
        AsciiIdentifier(AsciiIndex) = CharacterCode
    Next

        For AsciiIndex = 1 To TextLength
            CipherText = CipherText & Chr(AsciiIdentifier(AsciiIndex))
        Next
    ShiftRight = CipherText
End Function

---

Private Function ShiftLeft(ByVal ShiftString As String, ByVal ShiftQuantity As Long) As String

    Dim TextLength As Long
    TextLength = Len(ShiftString)

    Dim CipherText As String
    Dim CharacterCode As Long
    Dim AsciiIndex As Long
    Dim AsciiIdentifier() As Long
    ReDim AsciiIdentifier(1 To TextLength)

    For AsciiIndex = 1 To TextLength
        CharacterCode = Asc(Mid(ShiftString, AsciiIndex, 1))
        If CharacterCode = 32 Then GoTo Spaces
        If CharacterCode - ShiftQuantity < 65 Then
            CharacterCode = CharacterCode + 26 - ShiftQuantity
        Else: CharacterCode = CharacterCode - ShiftQuantity
        End If
Spaces:
        AsciiIdentifier(AsciiIndex) = CharacterCode
    Next

        For AsciiIndex = 1 To TextLength
            CipherText = CipherText & Chr(AsciiIdentifier(AsciiIndex))
        Next
    ShiftLeft = CipherText
End Function

Answer 1

Just some things that jump out at me:

Standard VBA Naming conventions have camelCase for local variables, and PascalCase only for sub/function names and Module/Global Variables. This allows you to tell at a glance if the variable you're looking at is local to your procedure, or coming from somewhere else.

---

I would probably use EncryptUsingCaesarCypher as your function name. It's more descriptive and closer to what it actually does.

`Text = CaesarCypher(text)

versus

`Text = EncryptUsingCaesarCypher(text)

Other than that, your naming is pretty solid.

---

Why separate functions for ShiftLeft and ShiftRight? The code in both is heavily-repeated and could be easily combined into a Shift(ByVal shiftValue as Long) function that handles positive and negative. This also lets you cut out all that messing around with isPositve and Abs(shift)

Answer 2

Minor note. This line:

If CaesarShift < 0 Then IsPositive = False

Would probably be more clear as an assignment.

IsPositive = ( CaesarShift >= 0 )

Albeit a matter a preference, I prefer the latter because it puts the variable that's being written to completely on the left of the statement. Doing it this way also reduces the total number of operations.

(Not that it's a performance bottleneck anyway, but it's important to know how to spot these things when you eventually do have a bottleneck you need to take care of.)

Answer 3

in your CaesarCipher function you create an extra boolean that you don't really need

Public Function CaesarCipher(ByVal TextToEncrypt As String, ByVal CaesarShift As Long) As String

    'Positive means shift to right e.g. "A" Shift 1 returns "B"
    Dim IsPositive As Boolean
    IsPositive = True
    If CaesarShift < 0 Then IsPositive = False
    CaesarShift = Abs(CaesarShift)

    Dim OutputText As String
    TextToEncrypt = UCase(TextToEncrypt)

    If CaesarShift > 26 Then
        CaesarShift = CaesarShift Mod 26
    End If

    If IsPositive Then
          OutputText = ShiftRight(TextToEncrypt, CaesarShift)
    Else: OutputText = ShiftLeft(TextToEncrypt, CaesarShift)
    End If

    CaesarCipher = OutputText
End Function

I suggest you change the last if/then statement to check whether the the shift is zero, positive or negative, if it is zero you don't have to call either shift and you save processing, if it is positive you don't need to call the absolute function at all, so you shave a lot of data processing if the shift is positive.

only if the shift is negative do you have to call the absolute function. here is what it looks like after we change the if/else statement and remove the boolean.

Public Function CaesarCipher(ByVal TextToEncrypt As String, ByVal CaesarShift As Long) As String

    Dim OutputText As String
    TextToEncrypt = UCase(TextToEncrypt)

    If CaesarShift > 26 Then
        CaesarShift = CaesarShift Mod 26
    End If

    If CaesarShift = 0 Then
        OutputText = TextToEncrypt
    ElseIf CaesarShift > 0 Then
        OutputText = ShiftRight(TextToEncrypt, CaesarShift)
    Else
        CaesarShift = Abs(CaesarShift)
        OutputText = ShiftLeft(TextToEncrypt, CaesarShift)
    End If

    CaesarCipher = OutputText
End Function