6
\$\begingroup\$

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
\$\endgroup\$
5
\$\begingroup\$

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)

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

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

\$\endgroup\$
4
\$\begingroup\$

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.