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)

• Raystafarian: The only way I could figure out to combine those two would be using cases because depending on positive or negative the values they are compared to change. And cases didn't seem much better. Thanks, I didn't actually know there was a camelCase and PascalCase or the difference.
• Kaz: IMO, just combine the functions and do all the comparisons/cases in one. It's going to end up with roughly the same amount of code, but now you're going to have a universal (shift string) function which can handle any shift paramater, rather than 2 competing ones where you have to know beforehand if you're shifting up or down. You could even just wrap your +- shift functions under a main "ShiftString" function which will itself determine positive or negative then call the appropriate function.

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

• Raystafarian: That is a good way to avoid the boolean I didn't think of, thanks! Re: long vs integer, via the SO Answer to Why Use Integer there's reference in the office dev center that integers are silently converted to long in VBA, which was surprising to me when zak mentioned it in an answer and I went looking for the documentation. Very valid point otherwise.
• Malachi: That is interesting, although that page doesn't mention Short integers, I wonder if they too are converted to Long integers? it seems that there is a slight performance benefit to using a long over an integer because of the silent conversion of the integer to a long because the long won't need to do the extra conversion.
• Raystafarian: I've never used short but I'm pretty sure it's not a data type for VBA ref. There's single and double, but they are floating point.
• Malachi: I tried to substitute short, int16, system.int16 or Long and none of them worked in the VBA Macro, @Raystafarian. so Long is the most appropriate.
• RubberDuck: Lol. @Malachi a VB6 Integer is a Short. A VB6 Long is an (.Net) Integer.