Encrypting a given string of text - Caesar cipher

Question

Edit: this is known as Caesar cipher. I was trying to make a program which had the main purpose of encrypting a given (short and lowercase) string. It would do so by shifting all of the letters n spaces to the right (encrypting) or to the left (decoding).

Here's what I've written so far(edited)

#include "stdafx.h"
#include <iostream>
#include <string>
using namespace std;

char abc[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'};

void code(int n) {
  string cadena;
  cout << "Introduzca la cadena a cifrar : " << '\n';
  cin >> cadena;

  for (int i(0); i < cadena.length; i++) {
    for (int f(0); f < strlen(abc); f++) {
      if (cadena[i] == abc[f]) {
        int w;
        w = f + n;
        if (w > strlen(abc)) {
          w -= strlen(abc);
        }
        cadena[i] = abc[w];
      }
    }
  }
  cout << cadena << '\n';
  system("pause");
}

void decode(int n) {
  string cadena;
  cout << "Introduzca la cadena a cifrar : " << '\n';
  cin >> cadena;

  for (int i(0); i < cadena.length; i++) {
    for (int f(0); f < strlen(abc); f++) {
      if (cadena[i] == abc[f]) {
        int w;
        w = f - n;
        if (w < 0) {
          w--;
          w = strlen(abc) - w;
        }
        cadena[i] = abc[w];
      }
    }
  }
  cout << cadena << '\n';
  system("pause");
}

int main() {
  int n;
  cout << "Introduzca el numero del cesar " << '\n';
  cin >> n;
  cout << "Desea usted cifrar o descifrar?" << '\n';
  cout << "Introduzca c para cifrar o d para descifrar" << '\n';
  char chos;
  cin >> chos;
  if (chos == 'c')
    code(n);
  else
    decode(n);

  return 0;
}  

Now the problem is I'm getting an awful string which I didn't even know how it was formed. And there's also an off by one mistake.

This is the result

Answer 1

Let's start by taking a quick walk through the code.

char abc[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' };

This is good. It doesn't handle upper case, but a great start

void code(int n)
{
    char cadena;

Defines a lone character. We'll see later that this isn't what you want.

    char * cadena_ptr = &cadena;

Under normal circumstances, this is a good idea, but with a single character, not so useful

    cout << "Introduzca la cadena a cifrar : " << '\n';
    cin >> cadena;

Here is where things start going wrong. The code just read in one and only one character because of how cadena was defined.

    for (int i(0); i <= sizeof(cadena); i++)

As mentioned before cadena is one character so i <= sizeof(cadena) is the same as i <= 1. this results in 2 iterations of the loop. One for i=0 (0<=1) and one for i=1 (1<=1). This means the loop will go outside of the bounds of cadenas one character. The results of this are undefined. Program may crash. Program may overwrite and corrupt data around cadena. Program may eat your neighbour's cat. All would be perfectly valid, but the first two are much more likely than the third.

    {
        for (int f(0); f <= 26; f++)

This also runs outside of bounds. This loop also might not be necessary. In 2017 it is very unlikely that you will ever see a character encoding that does not store all of the characters in an ascending contiguous block. That said, this brute force loop eliminates the possibility of running afoul of a legacy or bizzaro character encoding that scrambles the character order.

I'm going to stick with this loop because the question's not really about ASCII arithmetic.

        {
            if (*cadena_ptr == abc[f])
            {
                int w;
                w = f + n;
                if (w >= 26)
                {
                    w -= 26;
                }
                *cadena_ptr = abc[w];

This whole block looks good and should work once the indexing problems are resolved. There is one improvement, though. As soon as you have found one match, you can stop looking for more. Also a lot of use of the magic number 26 that should be replaced.

            }
        }

    }
    cout << cadena << '\n';
    system("pause");
}

Cleaning it up stage 1:

Fix the off by one indexing. That's pretty quick:

    for (int i(0); i < sizeof(cadena); i++)
    {
        for (int f(0); f < sizeof(abc); f++) // death to magic number!

Next reading in more than one character:

The right way! Use a std::string and a range-based for loop

void code(int n)
{
    std::string cadena;
    //char * cadena_ptr = &cadena; obsoleted by std::string

    cout << "Introduzca la cadena a cifrar : " << '\n';
    cin >> cadena;

    for (char & ch:cadena)// loop through all characters in cadena
    {
        for (int f(0); f < sizeof(abc) ; f++)
        {
            if (ch == abc[f])
            {
                int w;
                w = f + n;
                if (w >= sizeof(abc))
                {
                    w -= sizeof(abc);
                }
                ch = abc[w];
                break;// we found it! Stop looking.
            }
        }
    }
    cout << cadena << '\n';
    system("pause");
}

I'm not going to waste my time on the wrong way. It's wrong. Learn to use std::string.

If the project requirements say no std::string, don't do it the wrong way. Do something completely different! Read characters one by one, convert them, and print them. To cipher until we find a character that cannot be converted, we could do something like:

bool goodchar(char & cadena, int n)
{
    for (int f(0); f < sizeof(abc) ; f++)
    {
        if (cadena == abc[f])
        {
            int w;
            w = f + n;
            if (w >= sizeof(abc))
            {
                w -= sizeof(abc);
            }
            cadena = abc[w];
            return true;
        }
    }
    return false;
}

void code(int n)
{
    char cadena;

    cout << "Introduzca la cadena a cifrar : " << '\n';
    cin >> cadena; // get first character

    while (goodchar(cadena)) // keep looping until we find a character that's 
                             // not in the list       
    {
        cout << cadena << '\n';
        cin >> cadena; // get next character
    }
    cout << '\n';
    system("pause");
}