C++ Caesar cipher

Question

sandorlev 0 Newbie Poster

16 Years Ago

Hello!

I've started programming C++ a week ago, so I cannot solve my problems easily. I've started a small project for learning classes, it's about encryption and stuff. I don't really get pointers, although I've read some about them. My problem is that when I use the cipher, it replaces all characters in the string with an 'a' or an 'A'. Thus, the output of test.cpp is the following

LeVEnte A nEVeM
AaAAaaa A aAAaA
Press any key to continue . . .

Also, I'd like to learn how to write beatiful code, so if I do something the ugly way, please tell me!

//character.h
#pragma once

#ifndef _SLENCRYPT_CHARACTER_H_
#define _SLENCRYPT_CHARACTER_H_

const char SPACE = ' ';

const char LOWER_ALPHABET[26] = {
'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i',
'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r',
's', 't', 'u', 'v', 'w', 'x', 'y', 'z'
};

const char UPPER_ALPHABET[26] = {
'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I',
'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R',
'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z'
};

#endif

================================================

//cipher.h
#pragma once

#ifndef _SLENCRYPT_CIPHER_H_
#define _SLENCRYPT_CIPHER_H_

#include <string>
#include "character.h"

class SLEncryptCipher
{
	public:
		std::string text;
		void convertToLower()
		{
			int length = this->text.length();
			for (int index = 0; index < length; index += 1)
			{
				//This is for trying all characters in the alphabet
				for (int count = 0; count < 26; count += 1)
				{
					if (this->text[index] == UPPER_ALPHABET[count])
						this->text[index] = LOWER_ALPHABET[count];
				}
			}
		}
		void convertToUpper()
		{
			int length = this->text.length();
			for (int index = 0; index < length; index += 1)
			{
				//This is for trying all characters in the alphabet
				for (int count = 0; count < 26; count += 1)
				{
					if (this->text[index] == LOWER_ALPHABET[count])
						this->text[index] = UPPER_ALPHABET[count];
				}
			}
		}
		void removeSpaces()
		{
			std::string::size_type index = 0;
			bool spacesLeft = true;
			while (spacesLeft)
			{
				index = this->text.find(SPACE);
				if (index != std::string::npos)
					this->text.erase(index, 1);
				else
					spacesLeft = false;
			}
		}
};

//The Caesar cipher
class Caesar : SLEncryptCipher
{
	public:
		std::string text;
		Caesar(std::string text)
		{
			this->text = text;
		}
		void encipher(const int shift)
		{
			int length = this->text.length();
			for (int index = 0; index < length; index += 1)
			{
				for (int count = 0; count < 26; count += 1)
				{
					if (this->text[index] == LOWER_ALPHABET[count])
					{
						this->text[index] = LOWER_ALPHABET[(count + shift) % 26];
					}
					else if (this->text[index] == UPPER_ALPHABET[count])
					{
						this->text[index] = UPPER_ALPHABET[(count + shift) % 26];
					}
				}
			}
		}
};

#endif

================================================

//test.cpp

//test.cpp
#include <iostream>
#include "cipher.h"

using namespace std;

int main()
{
	Caesar test("LeVEnte A nEVeM");
	cout << test.text << endl;
	test.encipher(1);
	cout << test.text << endl;
	system("pause");
	return 0;
}

c c++ encryption

2 Contributors
8 Replies
793 Views
6 Days Discussion Span
Latest Post 16 Years Ago Latest Post by vmanes

Dani AI

Generated 10 Months Ago

Short diagnosis and a practical fix.

The symptom (everything ending up as a/A) comes from the inner loop continuing to test a character after you replace it. As explained, once you assign a new letter inside the for (count = 0; count < 26; ++count) loop that freshly written letter can match a later LOWER_ALPHABET/UPPER_ALPHABET entry and be changed again, possibly all the way around to 'a'/'A'. Using break or a boolean flag (as suggested by ) stops that; both are correct fixes for the current approach.

A cleaner and safer approach is to avoid the alphabet arrays and nested search entirely. Convert each character by arithmetic on its ASCII code and normalize the shift so negative values work the same in all compilers. Example:

int s = (shift % 26 + 26) % 26;
for (size_t i = 0; i < text.size(); ++i) {
    unsigned char c = text[i];
    if (c >= 'a' && c <= 'z')
        text[i] = char((c - 'a' + s) % 26 + 'a');
    else if (c >= 'A' && c <= 'Z')
        text[i] = char((c - 'A' + s) % 26 + 'A');
}

Notes and style tips:

C++ % with a negative left operand can be negative (e.g. -3 % 26 == -3), so normalize as shown.
Do not redeclare text in Caesar (that hides the base member). Keep data members private/protected and expose clear methods.
Prefer std::isupper/std::islower from <cctype> for readability, and avoid system("pause") in portable code.

With these changes, LeVEnte A nEVeM shifted by +1 will produce MfWFouf B oFWfN.

All 8 Replies

vmanes 1,165 Posting Virtuoso

16 Years Ago

When your encipher loop has found the letter and converted to the enciphered character, it keeps on checking that character. With a shift of one, if finds it again, changes it to the next letter, till it gets all the way to z/Z, and wraps it around to a/A at the end of the loop.

A couple ways you can fix this. One would be a boolean var in addition to the loop counter, something like:

void encipher(const int shift)
{
   int length = this->text.length();

   bool found = false;

   for (int index = 0; index < length; index += 1)
   {
      found = false;

      for (int count = 0; !found && count < 26; count += 1)
      {
         if (this->text[index] == LOWER_ALPHABET[count])
         {
            this->text[index] = LOWER_ALPHABET[(count + shift) % 26];
            found = true;
         }
         else if (this->text[index] == UPPER_ALPHABET[count])
         {
            this->text[index] = UPPER_ALPHABET[(count + shift) % 26];
            found = true;
         }
      }
   }
}

Another would be to use break statements when the conversion is done

for (int count = 0;  count < 26; count += 1)
      {
         if (this->text[index] == LOWER_ALPHABET[count])
         {
            this->text[index] = LOWER_ALPHABET[(count + shift) % 26];
            break;
         }
         else if (this->text[index] == UPPER_ALPHABET[count])
         {
            this->text[index] = UPPER_ALPHABET[(count + shift) % 26];
            break;
         }
      }

The break gets you out of the inner loop, but the outer loop will execute its next iteration.

vmanes 1,165 Posting Virtuoso

16 Years Ago

Oh, and another thought. What happens when a negative value is used as the shift?

Reply to this topic

Be a part of the DaniWeb community

We're a friendly, industry-focused community of developers, IT pros, digital marketers, and technology enthusiasts meeting, networking, learning, and sharing knowledge.

sandorlev 0 Newbie Poster · Answer 1 · 2010-02-02T21:33:28+00:00

Oh, and another thought. What happens when a negative value is used as the shift?

it should decipher it, but it doesn't... i don't know why.

vmanes 1,165 Posting Virtuoso · Answer 2 · 2010-02-02T23:03:57+00:00

Let's say the shift is -5. What happens when you find a letter at index 2?

( 2 + (-5)) % 26 = ????

sandorlev 0 Newbie Poster · Answer 3 · 2010-02-03T18:42:05+00:00

Let's say the shift is -5. What happens when you find a letter at index 2?
( 2 + (-5)) % 26 = ????

i've tried in in the python interpreter and it says

>>> (2 + (-5) % 26)
23

which should be right, but i don't get the good results when i try it in c++

vmanes 1,165 Posting Virtuoso · Answer 4 · 2010-02-03T19:58:43+00:00

We're working in C++ here.
What does this do?

#include <iostream>
using namespace std;

int main( )
{
   cout << ( -3 % 26 );
   cout << endl;
   return 0;
}

sandorlev 0 Newbie Poster · Answer 5 · 2010-02-07T23:32:48+00:00

We're working in C++ here.
What does this do?

#include <iostream>
using namespace std;

int main( )
{
   cout << ( -3 % 26 );
   cout << endl;
   return 0;
}

it returns -3, so the solution can be (26 - (shift % 26)), right?

vmanes 1,165 Posting Virtuoso · Answer 6 · 2010-02-08T00:15:01+00:00

vmanes 1,165 Posting Virtuoso

16 Years Ago

How about:
( 26 + (count + shift ) ) % 26

C++ Caesar cipher

Dani AI

Recommended Answers Collapse Answers

All 8 Replies

Recommended Answers