Simple Caesar Cipher Function

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


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)

• 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. – Raystafarian Jan 29 '16 at 16:05
• 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. – Kaz Jan 29 '16 at 16:22

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.)

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

• 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. – Raystafarian Jan 31 '16 at 10:25
• 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. – Malachi Jan 31 '16 at 18:32
• 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. – Raystafarian Feb 1 '16 at 8:57
• 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. – Malachi Feb 1 '16 at 15:07
• Lol. @Malachi a VB6 Integer is a Short. A VB6 Long is an (.Net) Integer. – RubberDuck Feb 2 '16 at 10:02