Basic cryptography algorithm

Question

I wrote 2 classes in order to implement some basic cryptography algorithm. I tried to use some object-oriented concepts of C++. I'm a beginner, so any comment or suggestion is welcome.

First class: Message

The header:

#ifndef MESSAGE_H
#define MESSAGE_H
#include <iostream>
#include <fstream>
#include <string>
#include <vector>
class Message
{
 public:
 friend class Chiffre;
 typedef std::vector<char> cryptoContainer;
 // constructors
 Message();
 Message(const std::string&);
 Message(const cryptoContainer&);
 Message(const std::string&, const int&);
 // templates for writing original and crypted messages
 template<typename T>
 int writeOriginalMessage(T& stream) const // return original message in an output stream
 {
 for (auto it = m_originalContent.begin(); it != m_originalContent.end(); it++) stream << *it;
 stream.flush();
 return 0;
 }
 template<typename T>
 int writeCryptedMessage(T& stream) const // return crypted message in an output stream
 {
 if (m_cryptedContent.empty()) return 1;
 for (auto it = m_cryptedContent.begin(); it != m_cryptedContent.end(); it++) stream << *it;
 stream.flush();
 return 0;
 }
 // getters
 cryptoContainer getOriginalMessage(void) const;
 cryptoContainer getCryptedMessage(void) const;
 // destructor
 virtual ~Message();
 protected:
 //protected setter
 void setOriginalMessage(cryptoContainer&);
 void setCryptedMessage(cryptoContainer&);
 //attributes
 int m_key;
 cryptoContainer m_originalContent;
 cryptoContainer m_cryptedContent;
};
#endif // MESSAGE_H

The .cpp file:

#include "Message.h"
Message::Message(const std::string& texte):
 m_key(0),
 m_originalContent(texte.begin(),texte.end())
{
}
Message::Message(const Message::cryptoContainer& texte):
 m_key(0), // default key is 0
 m_originalContent(texte)
{
}
Message::Message(const std::string& texte, const int& clef):
 m_key(clef),
 m_originalContent(texte.begin(),texte.end())
{
}
Message::~Message()
{
}
Message::cryptoContainer Message::getOriginalMessage(void) const
{
 return m_originalContent;
}
Message::cryptoContainer Message::getCryptedMessage(void) const
{
 return m_cryptedContent;
}
void Message::setOriginalMessage(Message::cryptoContainer& msg)
{
 m_originalContent = msg;
 return;
}
void Message::setCryptedMessage(Message::cryptoContainer& msg)
{
 m_cryptedContent = msg;
 return;
}

second class: Chiffre (will implement crypto algorithms)

The header:

#ifndef CHIFFRE_H
#define CHIFFRE_H
#include <vector>
#include "Message.h"
class Chiffre
{
public:
 Chiffre();
 virtual ~Chiffre() = 0;
 static void cesar(Message&, const int&); // décale un caractère à partir d'une clef (algorithme de César)
};
#endif // CHIFFRE_H

The .cpp file:

#include "Chiffre.h"
Chiffre::Chiffre()
{
}
Chiffre::~Chiffre()
{
}
void Chiffre::cesar(Message& msg, const int& clef)
{
 Message::cryptoContainer res;
 Message::cryptoContainer orig = msg.getOriginalMessage();
 for (auto it = orig.begin(); it != orig.end(); it++) res.push_back(*it + clef);
 msg.Message::setCryptedMessage(res);
 return;
}

Answer 1

I'm no cryptography guru, so my comments will aim at the code in itself, not the algorithms.

Message.h / Message.cpp:

1. There doesn't seem to be a need to include <iostream> nor <fstream> in the header file. Avoid importing dependencies that you don't use. The cost of an include is in compile-time. One too many big projects suffer with long compile times due to poor management of include files.

2. Keep your parameter names in function/method prototypes. This will self document your code. Take this constructor for instance:

   Message(const std::string&, const int&);
   

   Impossible to tell which parameter is doing what without consulting the implementation.

3. cryptoContainer is a "type alias", nevertheless, it names a type. Your naming convention for types is PascalCase, so CryptoContainer would be more self-consistent.

4. writeOriginalMessage is currently returning a meaningless integer value (always zero). It could very well return nothing (void). If there is a possibility of failure, then return a boolean true or false.

5. Still talking about writeOriginalMessage and writeCryptedMessage, both have a quite similar loop inside:

   for (auto it = m_originalContent.begin(); it != m_originalContent.end(); it++)
   

   You can significantly shorten them by using a range based foreach loop:

   for (const auto & item : container)
   {
    stream << item;
   }
   

   Notice the reference &. Care should be taken to avoid copying things in this style of iteration since the default in C++ is always a copy.

6. Writing void in the parameter list of a C++ function that takes no parameters (as in getOriginalMessage(void)) is a C-ish style. C++ does not require that; () is just as good. So avoid the unnecessary verbosity.

7. virtual destructors are only necessary when your class is meant to be inherited from. In this particular code, that doesn't seem to be the case with Message. If you did that to allow users of your code to inherit from the class, I would advise against, since Composition is usually much more elegant and with less coupling. You should remove virtual from the destructor declaration and perhaps even mark your class final. While your are at it, them make the protected section private and throw the destructor away, since it is empty.

8. I see that you have a couple places with names not in English, while the bigger whole is all in that language. It would be better to keep things uniform, so replace things like texte and clef with the proper English terms.

9. In functions/methods that return void, no need to explicitly return at the end. E.g.: setOriginalMessage and setCryptedMessage.

10. Careful with noisy/meaningless comments. // constructors and // destructor are self evident.

Chiffre.h / Chiffre.cpp:

1. Again, unnecessary includes: you don't need <vector> in the header file.

2. Also, the reasons for Chiffre having a virtual destructor are not clear. If you don't have inheritance, you shouldn't be making it virtual.

3. The empty constructor is also unnecessary. Leave it out if you have no manual initialization to perform.

• youyou: thanks for your answer :) Just 2 points: 1- I declared Chiffre as virtual because there will never need to construct a Chiffre object, this class will just contain crypto methods. 2- is it ok to declare Chiffre as a friend of Message in order to use the crypto methods defined in Chiffre on Message objects (i mean: is it in the spirit of C++) ?
• glampert: @user59330 Oh, so if you don't want to allow instantiation of an object, you should declared the constructor and destructor private. Someone can still instantiate Chiffre as a pointer (e.g. using new). Ideally, you should try to avoid making classes friends of each other. This creates a strong dependency between them. In this case, I think it would probably be better to just make the set*() method part of the public interface of Message.