Vigenere cipher breaker

Question

I am a relative newbie to C++. I recently finished a command line program which cracks Vigenere encoded messages from a file by testing each word in the dictionary.

I have never had a tutor/teacher for C++, so I wanted to make sure that I don't develop any bad habits.

Main.cpp:

#include <iostream>
#include <fstream>
#include <vector>
#include <string>
#include "Alphanumeric.h" //turns characters into numerical equivalents
#include "Vigenere.h" //Vigenere Functions
#include <list>

#ifdef __APPLE__
    #define DICTPATH "/usr/share/dict/words"
#endif


int main(int argc, char* argv[])
{

std::cout << "*************VIGENERE BREAKER*************" << std::endl;

std::ifstream input(argv[1]);
std::ifstream dictionary(DICTPATH);

std::string process = argv[2];
std::string key = argv[3];

std::string plain;
std::string str;

while (std::getline(input, str))
{
    plain += str;
}

if (process == "-e")
{
    std::cout << "*************ENCODE*************" << std::endl;
    std::string result = Vigenere::EncryptVigenere(plain, key);
    std::cout << result << std::endl;
}
if (process =="-d")
{
    std::cout << "*************DECODE*************" << std::endl;
    std::string result = Vigenere::DecryptVigenere(plain, key);
    std::cout << result << std::endl;
}

if (process == "-dicf")
{
    const float englishfreq[] = {8.2,1.5,2.8,4.3,12.7,2.2,2,6.1,7,0.2,0.8,4,2.4,6.7,7.5,1.9,0.1,6,6.3,9.1,2.7,1,2.4,0.15,1.9,0.074};
    const int threshold = atoi(argv[3]);

    std::cout << "*************DICTIONARY_ATTACK*************" << std::endl;
    std::list<std::string> results;
    while (std::getline(dictionary, str))
    {
        float total = 0;
        float frequency[26] = {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0};

        std::cout << str << std::endl;
        std::string result = Vigenere::DecryptVigenere(plain, str);


        for (char c : result)
        {
            frequency[Alphanumeric::chartoint(c)]++;
        }

        int size = result.size();
        int index = 0;
        for (float freq : frequency)
        {
            freq = (freq / size)*100;
            float difference = freq - englishfreq[index];
            if (difference < 0)
            {
                difference *= -1;
            }
            total += difference;
            index++;
        }

        if (total <= threshold) {
            std::cout << "RESULT FOUND" << std::endl;
            results.push_back("Using " + str);
            results.push_back(result);
        }
    }
    std::cout << "*************RESULTS*************" << std::endl;
    for (std::string s : results)
    {
        std::cout << s << std::endl;
    }
    std::cout << "dictionary attack complete!!!" << std::endl;
}
if (process == "-dic")
    {

        std::cout << "*************DICTIONARY_ATTACK*************" << std::endl;
        std::list<std::string> results;
        std::string keyword = argv[3];
        while (std::getline(dictionary, str))
        {
            std::cout << "key:";
            std::cout << str << std::endl;
            std::string result = Vigenere::DecryptVigenere(plain, str);

            if (result.find(keyword) != std::string::npos) {
                std::cout << "RESULT FOUND" << std::endl;
                results.push_back("Using " + str);
                results.push_back(result);
            }
        }
        std::cout << "*************RESULTS*************" << std::endl;
        for (std::string s : results)
        {
            std::cout << s << std::endl;
        }
        std::cout << "dictionary attack complete!!!" << std::endl;
    }
return 0;
}

Vigenere.h:

#ifndef VIGENERE_H_
#define VIGENERE_H_

namespace Vigenere
{

std::string DecryptVigenere(std::string ciphertext, std::string key);
std::string EncryptVigenere(std::string plaintext, std::string key);

}
#endif /* VIGENERE_H_ */

Vigenere.cpp:

#include <string>
#include "Vigenere.h"
#include "Alphanumeric.h"

std::string Vigenere::DecryptVigenere(std::string ciphertext, std::string key)
    {
    std::string answer;
    for (int i = 0; i < ciphertext.length(); i++)
    {
        int numeric = Alphanumeric::chartoint(ciphertext[i]);
        int keyval = Alphanumeric::chartoint(key[i%key.length()]);
        numeric -= keyval;
        if (numeric < 0)
        {
            numeric += 26;
        }
        numeric %= 26;
        answer += Alphanumeric::inttochar(numeric);
    }
    return answer;
}

std::string Vigenere::EncryptVigenere(std::string plaintext, std::string key)
{
    std::string answer;

    for (int i = 0; i < plaintext.length(); i++)
    {
        int numeric = Alphanumeric::chartoint(plaintext[i]);
        int keyval = Alphanumeric::chartoint(key[i % key.length()]);
        numeric += keyval;
        numeric %= 26;
        answer += Alphanumeric::inttochar(numeric);
    }
    return answer;
}

Answer 1

std::string DecryptVigenere(std::string ciphertext, std::string key);
std::string EncryptVigenere(std::string plaintext, std::string key);

For input parameters, pass non-primitive types by reference-to-const to avoid unnecessary allocations. See Const-Correctness and Parameter Passing.

std::string DecryptVigenere(std::string const& ciphertext,
                            std::string const& key);
std::string EncryptVigenere(std::string const& plaintext,
                            std::string const& key);

---

#include <string>
#include "Vigenere.h"
#include "Alphanumeric.h"

Organize your headers by robustness to catch latent usage errors as soon as possible.

#include "Vigenere.h"      // Interface
#include "Alphanumeric.h"  // Project-Level header
#include <string>          // Language-Level header

---

std::string EncryptVigenere(std::string const& plaintext,
                            std::string const& key);
{
    std::string answer;

When you know the amount of space required for a container, use the reserve member function to pre-allocate space. In your two transformation functions, the length of the answer will always be the same length as the first input argument.

std::string EncryptVigenere(std::string const& plaintext,
                            std::string const& key);
{
    std::string answer;
    answer.reserve(plaintext.size());

---

        int keyval = Alphanumeric::chartoint(key[i%key.length()]);

Make sure to test your preconditions. key must be non-empty as you cannot divide/modulo-by-zero (undefined behavior according to the C++ Standard §5.6/4).

Be consistent with your formatting (operator spacing, function naming).

std::string encrypt_vigenere(std::string const& plaintext,
                            std::string const& key);
{
    if (key.empty()) {
        throw std::invalid_argument("Invalid key length of Zero.");
    }
    // ...
        int keyval = Alphanumeric::char_to_int(key[i % key.length()]);

---

    std::cout << "*************VIGENERE BREAKER*************" << std::endl;

Be aware of what std::endl actually does.

Inserts a newline character into the output sequence os and flushes it as if by calling os.put(os.widen('\n')) followed by os.flush().

If you did not intend to force the buffer to flush, consider just outputting '\n'.

    std::cout << "*************VIGENERE BREAKER*************\n";

---

    std::ifstream input(argv[1]);
    std::string process = argv[2];
    std::string key = argv[3];

Check to make sure that these arguments actually exists. Index access out of bounds is undefined behavior.

const qualify your immutable objects.

    if (argc != 4) {
        // TODO: Print a helpful message instead.
        std::cerr << "The syntax of the command is incorrect.\n";
        return EXIT_FAILURE;
    }

    std::ifstream input(argv[1]);
    const std::string process = argv[2];
    const std::string key = argv[3];

---

#ifdef __APPLE__
    #define DICTPATH "/usr/share/dict/words"
#endif

// ...
    std::ifstream dictionary(DICTPATH);

What if __APPLE__ isn't defined? Would be nice if there was some fallback dictionary to use.

Don't use macros to define variables. If you need compile-time evaluation of variables, functions, or objects, use constexpr. Source control remains a legitimate use-case for macros.

#ifdef __APPLE__
constexpr auto dictionary_path = "/usr/share/dict/words";
#else
constexpr auto dictionary_path = "english.txt"
#endif

// ...
    std::ifstream dictionary(DICTPATH);

---

    if (process == "-e") { /* ... */ }
    if (process == "-d") { /* ... */ }
    if (process == "-dicf") { /* ... */ }
    if (process == "-dic") { /* ... */ }

The blocks that process can branch to are exclusive from each other. To enforce that exclusivity, you can use else if to make it clear to the reader that this is happening.

What should happen if the process is invalid?

    if (process == "-e") { /* ... */ }
    else if (process == "-d") { /* ... */ }
    else if (process == "-dicf") { /* ... */ }
    else if (process == "-dic") { /* ... */ }
    else { /* handle invalid process */ }

---

            frequency[Alphanumeric::chartoint(c)]++;

This seems a little fishy to me. I would expect an Alphanumeric class or function collection to return a mapping over the set of characters and numbers. 26 characters just doesn't seem right.

---

        if (total <= threshold) {
            std::cout << "RESULT FOUND" << std::endl;
            results.push_back("Using " + str);
            results.push_back(result);
        }

Since the results are not being saved for anything, just print them as you find them.

Answer 2

Code design

It is perfectly fine if you're really new to C++, but I think that one of the worst thing you can do with C++ is to use it like C. If you don't use the object oriented paradigm, you are more likely to write C code with STL than actual C++.

If you need ideas for a second version of this exercise, you could try to design it in a way that take the code out of the main function to organise it in classes. This would also allow you to easily store and use your magic number "26" that you copy and pasted to several places (which is a very bad practise as it harms maintainability).

Error checking

It may be a good idea to check for command line parameters errors and give a feedback for it. You should check the length of argv[] to avoid any problem but you should also give a feedback to the user if his parameters do not meet what you expect.

Containers

You are using std::list to store the results but you seem to only use push_back with it. An std::vector would be a more appropriate container for your use.

Iterations

I love foreach loops too, but when you need to keep track of the index, it probably means that you should use a standard for loop. This is what you did when you iterated on the frequency array.