Caesar Cipher | Optimization and coding tips wanted

[CrossG]

Hello I have recently begun programming in C++ and would love for someone to help me learn the process of optimizing my code from a beginner level to a more proficient level required in the professional world today. Any tips and or recommendations are wanted/needed to help me broaden my coding skills

  // Caesar Cipher 3.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include <iostream>
#include <string>

using namespace std;

class mainClass {
    public:
		void setShift(int z) {
			shiftVal = z;
		}
		string getMessage() {
			for (int i = 0; i < message.length(); i++) {
				message[i] += shiftVal;
			}
			cout << "Encrypted Message: " + message << endl;
			for (int i = 0; i < message.length(); i++) {
				message[i] -= shiftVal;
			}
			cout << "Decrypted Message: " + message << endl;
			return message;
		}

		void setMessage(string x) {
			message = x;
		}
    private:
		string message;
		int shiftVal;
};

int main()
{
	mainClass mo;
	int shift;
	cout << "Shift by: (1-10) " << endl;
	cin >> shift;
	mo.setShift(shift);
	cout << "Enter Message: " << endl;
	string messageVar;
	cin >> messageVar;
	mo.setMessage(messageVar);
	mo.getMessage();
    return 0;
}

[jonnin]

this isnt really a good candidate for optimization. Its hard to beat a tight loop that does something simple like +=


tweaking it for professionalism, you can add more comments as to what you are doing, and consider range based for loops. Naming conventions .. variable named mo is probably right out in most workplaces, use words that mean something and shorties like int x should be only used for loop variables or other very simple purposes with short lifespans. Most people are favoring std::cout instead of cout, and not having the using namespace std statement anymore.

style depends on your employer. Some places may like the mismatched book braces, others may like them to align in columns. Some will favor this or that variable name styles. Some will want specific comment styles and those can be very important as some commonly used tools can extract them to build a type of documentation from the comments, if the format is correct.

[icy1]

Firstly you want to prioritize the logic before formatting. All you're doing is adding an offset to the int value of the character, which is not how Caesar Cipher works.

If I enter 'zzzzz' , where a 'z' is equal to 122, with offset 10, I should not be seeing a bunch of 132, but rather a "wrap-around" back to the beginning of the alphabet. Since 'z' is the last letter of the alphabet, I should be seeing a bunch of 10's, aka 'j', and so the output should be 'jjjjj'. http://practicalcryptography.com/ciphers/caesar-cipher/

Convert everything to either lowercase or uppercase and figure out how to incorporate the wrap-around for a working caesar shift algo.

For formatting, for now I'd just comment about consistent spacing everywhere. Things are a bit simpler when copy-pasting code if you change your editor options to "insert spaces instead of tabs" , and set it to "4" , for example. From my experience a lot of C++/C# developers use 4 spaces, but of course can vary.

Edit: on naming -- getMessage is not quite getting a message. It's currently doing some encryption, printing that out, doing some decryption, and printing that out. There should be separate methods for this. "getSomething" and "setSomething" are more Java style naming. C++ it's more common to just have "Something" as the getter, returning a read-only reference.

const string& Message() { return message; }
void SetMessage(const string& x) { message = x; }

void Encrypt()
{
// ...
}

void Decrypt()
{
//...
}

Marking things as read-only lets the user of the class know up front that certain parts will not be changed. Methods should only have one function and not try to do too much.

[dhayden]

Use '\n' instead of endl unless you really mean to flush the stream. The performance difference can be astounding.

To write better code, keep this in mind: the purpose of your classes is not to "do the work." The purpose is to "make doing the work easy." If you think like this, you'll write more general code.

Suppose I come to you and say that I want to use your class to encrypt and decrypt whole files. In other words, I want to read a file, encrypt the whole file and it back out again. Can I do that? The answer is no. In fact, your class is super specific. It encrypts the string, prints it to cout with a message, then decrypts it and prints it again. Yuck. I can't use it for anything else.

Now consider this program. All I've done is rearranged the logic a little to make the class more generic. It now encrypts and decrypts strings and leave the I/O to the main program.

// Caesar Cipher 3.cpp : Defines the entry point for the console application.
//

// #include "stdafx.h"
#include <iostream>
#include <string>

using namespace std;

class mainClass {
  public:
    void setShift(int z)
    {
	shiftVal = z;
    };
    string encrypt()
    {
	string result(message);
	for (size_t i = 0; i < message.length(); i++) {
	    result[i] += shiftVal;
	}
	return result;
    }
    string decrypt()
    {
	string result(message);
	for (size_t i = 0; i < message.length(); i++) {
	    result[i] -= shiftVal;
	}
	return result;
    }
    void setMessage(string x)
    {
	message = x;
    }
  private:
    string message;
    int shiftVal;
};

int
main()
{
    mainClass mo;
    int shift;
    cout << "Shift by: (1-10) " << endl;
    cin >> shift;
    mo.setShift(shift);
    cout << "Enter Message: " << endl;
    string messageVar;
    cin >> messageVar;
    mo.setMessage(messageVar);

    messageVar = mo.encrypt();
    cout << "Encrypted Message: " << messageVar << '\n';
    mo.setMessage(messageVar);
    cout << "Decrypted Message: " << mo.decrypt() << '\n';
    return 0;
}