Simple password encryption / decryption in C++

Question

The below code is for a simple password encryption/decryption program. The code is working perfectly, but the only thing I want to know from you is if this is good logic.

PEND.h

#ifndef PEND_H
#define PEND_H


class PEND
{
    private:
        char UString[30];
        char Password[70];
        int key;
        int raw;
        int Rid;
    public:
        PEND();
        void menu();
        void Encryption();
        void Decryption();
        void WFile();
        bool CheckID();
        void about();
};

#endif // PEND_H

PEND.cpp

#include <iostream>
#include <conio.h>
#include <string.h>
#include <windows.h>
#include <ctime>
#include <fstream>
#include "PEND.h"

#define OR ||
#define AND &&

using namespace std;

PEND::PEND()
{
    raw=0;
    srand(time(0));
    for(int i=0;i<=90;i++)
    {
        if(i%2==0)
        {
            Password[i]=rand()%26+65;
            raw++;
            if(raw==2)
            {
                Password[i]=rand()%26+97;
                raw=0;
            }
        }
        else
            Password[i]=rand()%10+48;
    }
}

void PEND::menu()
{
    cout << "Password Encryption And Decryption"
          << "\n[E] Encryption"
          << "\n[D] Decryption"
          << "\n[A] About"
          << "\n[Q] Quit"
          << "\n\nPlease Note That Original and Encrypted Both Passwords Are Case Sensitive."
          << "\n\nPlease Choose Any Option : ";
}

void PEND::Encryption()
{
    cout << "\n\nPlease Choose A Secret ID For Your Password : ";
    cin >> Rid;
    cout << "\nEnter Password For Encryption. B/W (6-30) Characters : ";
    cin >> UString;
    key=strlen(UString);
    if(key < 6 OR key > 31)
        {   cout << "\nPassword Must be B/w 6 to 30 Characters\n\n";    }
    else
    {
        for(int i=0;i<key;i++)
        {
                Password[i*3]=UString[i]+(key/5);
        }

        Password[key*3] = '\0';
        cout << "\nEncrypted Password is : " << Password;
        cout << "\nPlease Don't Give Your Secret ID to Anyone. You Can Only Access Your Password With The Secret Key Provide By You :)\n\n";
        WFile();
    }
}

void PEND::Decryption()
{
    cout << "\nEnter Encrypted Password : ";
    cin >> Password;
    cout << "\nEnter Secret ID : ";
    cin >> Rid;
    bool frv=CheckID();
    if(frv==0)
    {
        cout << "\nRecord Doesn't Exist Please Check Your Secret ID or Encrypted Password. Please Make Sure That Password is Case Sensitive\n\n";
    }
    else
    {
        key=strlen(Password);
        raw=0;
        for(int i=0;i<key;i+=3)
        {
            UString[raw]=Password[i];
            raw++;
        }
        UString[raw]='\0';
        key=strlen(UString);
        if(key>5)
        {
            for(int i=0;i<key;i++)
            {
                    UString[i]-=(key/5);
            }
        }
        else
        {
            for(int i=0;i<key;i++)
            UString[i]-=key;
        }
        UString[key] = '\0';
        cout << "\nDecrypted Password is : " << UString << endl << endl;
    }
}

void PEND::about()
{
    cout << "\nThis is About Me Function :)\n\n";
}

void PEND::WFile()
{
    ofstream PFile("Password.txt", ios::app);
    PFile << Rid << ' ' << Password << endl;
    PFile.close();
}

bool PEND::CheckID()
{
    int Sid;
    string searchp;
    ifstream PFile("Password.txt");
    while(PFile >> Sid >> searchp)
    {
        if(Sid==Rid AND searchp==Password)
            return 1;
    }
    return 0;
}

main.cpp

#include <iostream>
#include <conio.h>
#include <windows.h>
#include "PEND.h"

#define OR ||

using namespace std;

int main()
{
    PEND a1;
    char op;
    do
    {
        a1.menu();
        op=getche();
        switch(op)
        {
            case 'E':
            case 'e':
                a1.Encryption();
                break;
            case 'D':
            case 'd':
                a1.Decryption();
                break;
            case 'A':
            case 'a':
                a1.about();
                break;
            case 'Q':
            case 'q':
                exit(0);
            default:
                cout << "\nPlease Choose From Given Options :) \n\n";
        }
        getch();
        system("cls");
    }
    while(op!='q' OR op!='Q');

    return 0;
}

Answer 1

Naming:

I'm not very comfortable with your naming of variables and methods. Some are quite unclear. For instance: UString, Rid, Sid, raw, those are very vague names. Try to name things in a way to not leave margin for misinterpretation.

UString should probably be unencryptedPassword, then you could symmetrically rename Password to encryptedPassword.

Rid is a bad abbreviation for the passwordId. I'm not exactly sure what int raw is representing...

Your naming convention for methods is not uniform. You have most of them with PascalCase, but menu() and about() break that pattern.

WFile() should just be WriteFile(), avoid abbreviations that create awkward to read names; prefer naming methods/functions with phrases that imply an action or command.

PEND is another unnecessary abbreviation, in my view. You should be explicit and name it PasswordEncryption.

Unnecessary macros and using namespace:

This feels like a code smell:

#define OR ||
#define AND &&

If you you are not comfortable with using the punctuation characters for these operators, then you can use the standard name aliases or, and, not, etc... You don't have to define your own ones. Though I would suggest that you don't use them and stick to the standard way of writing logic ops using the punctuation characters. The wordy operators are not very popular and rarely found in any production code. The ||, &&, ! operators are a lot more idiomatic in C and C++.

using namespace std in an implementation file is not a big deal, though I tend to think of that as sign of laziness, since typing std:: is not going to cost you that much. We are not talking about a_very_long_namespace_name. But this is an open-ended discussion anyway.

Too many hardcoded constants:

Your code is filled with magic numbers. Taking the constructor of PEND, for instance, there are five or six different constants used, none of these are explained with comments. Try to turn them into named constants, or at least comment why you've chosen the given values.

Miscellaneous:

• This loop counter seems wrong:

  for(int i=0;i<=90;i++)
  

  i is being used to index the Password array, which is only 70 chars in size! Aren't you getting a memory access violation when running this code? Instead os using raw arrays, consider std::array or std::vector, using the size() method to query the array length.

• The way these lines are laid-out, makes them hard to read:

  if(key < 6 OR key > 31)
      {   cout << "\nPassword Must be B/w 6 to 30 Characters\n\n";    }
  

  Would be better to place each { in its own line, like it is with the rest of the code.

• Finally, some spacing between arithmetical operators and expressions tends to improve code readability. Examples:

  for(int i=0;i<key;i++)
  

  vs:

  for (int i = 0; i < key; i++)
  

  And:

  Password[i*3]=UString[i]+(key/5);
  

  vs:

  Password[i * 3] = UString[i] + (key / 5);
  

  The spacing gives more emphasis and visibility to the operators.

Answer 2

In addition to the excellent suggestions made by @glampert...

Remove unnecessary member variable

1. Remove member variable key. It is derived from UString as:

   key=strlen(UString);
   

   in couple of functions. You can just as easily make it a function local variable.

   size_t key=strlen(UString);
   

2. For the same reasons as above, raw can be removed from being a member variable and used as a function local variable.

3. For the same reasons as above, UString can be removed from being a member variable and used as a function local variable.

4. I don't see the need for Rid as a member variable either. You are asking the user to provide it at the time of encrypting the password and decrypting the password. Why store it in the class and the "Password.txt" file at all?

   It can be used as a local variable in the functions where it is needed.

After all those changes, the only member variable PEND needs is Password.

Avoid platform specific headers and functions

When you use windows.h, conio.h, getche, and getch, you make your program harder to port to another platform. You can use fgetc(stdin) to get a character from the user.

Make function names verbs

Functions are meant to do something. Making them verbs makes the code more readable.

  void menu();         ->   void DisplayMenu();
  void Encryption();   ->   void Encrypt();
  void Decryption();   ->   void Decrypt();
  void WFile();        ->   void WriteToFile();
  bool CheckID();      ->   No change needed. It is already a verb.
  void about();        ->   void DisplayAbout();