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;
}

• apnorton: As an aside, I hope that this is an academic-style exercise and not actually going to end up being used to encrypt anything. (The few known methods that can withstand modern attack are much more sophisticated than this, and it's best to use libraries for real-world encryption.)
• jake.toString: It is also worth mentioning that a password should be hashed, not encrypted.
• Anti-weakpasswords: Please read Thomas Pornin's canonical answer to How to securely hash passwords?. If this is not an academic exercise, please also read Don't be a Dave.
• Khurram Farooq: This is an academic exercise you can say it is a university project ;)

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.

• Khurram Farooq: Thank you very Much Sir!! i know i didn't add the comments. That is my mistake. it was actually Password[90] instead of Password[70] again my mistake. and i'll definitely use variables name correctly Thanks for your suggestion :)

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();