Code:
#include <string>
#include <iostream>
using namespace std;
// +++ Changed all occurrences of strlen(<string>.c_str()) to <string>.length() - this is C++, not C!
int main()
{
// ### I don't know what "predefine" is or what it represents - the comment does not help at all, at is just tells me
// ### the same as the code! Give it a better name, and add an explanatory comment.
// ### Note that "predefine" is a verb, whereas the thing it is describing looks to be a noun.
// +++ Made it const - I can't see where it is altered, so give a clue that it is supposed to be unchanging, and get the compiler to enforce this.
const char predefine[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'}; //predefine array
string plaintext;
cout <<"enter plaintext"; //taking input
cin >> plaintext;
// +++ Shifted declaration of "key" down - try to reduce the scope of variables as much as you can i.e. don't declare until just before it is used.
// ### "key" is a non-descriptive name - it's possibly even misleading, as "key" seems to be a string rather than (as I would expect from the name) a char.
// ### if this is a *cipher* key instead of a key on your keyboard, then call it cipherKey or something.
string key;
cout <<"enter key";
cin >> key;
for(int i = 0; i < plaintext.length() ; i++) //running loop
{
// +++ Made const.
// ### "character" is very generic. "plainTextChar", perhaps? This is a matter of taste.
// +++ Changed "inna" to "in a" variable throughout, although these comments are useless and just make things messy.
// +++ Unless this is homework and you have been told to comment each line, remove them.
const char character = plaintext[i] ; // each time a key takeout &store in a var
cout << character << endl;
// +++ Made const.
// ### I have no idea what "keytake" is supposed to represent.
// ### Also, this is a bug - it assumes that "key" is at least as long as "plaintext", but there is nothing that enforces this.
const char keytake = key[i]; //each time a key takeout & store in a var
cout << keytake << endl ;
// +++ Changed "wrapearound" to "wraparound", although I'd prefer "wrapAround". Made it const.
// ### I have no idea what wraparound represents - it sounds like a boolean, but is an int(?)
const int wraparound = key.length(); //store string length in a var
// +++ Added brackets around the conditional.
// ### It's impossible for this condition to ever be false, so I don't know why it is here. The comment does not help and just makes me more confused :p
if (wraparound == key.length()); //if string length is== string length
{
// ### Why are we re-initializing?
keytake = key[0] ; //reinitialize
}
// ### pdf is always pdf2 - why two variables?
// ### Also, bad naming - I have no idea what the represent, and was wondering why you were using Portable Document Formats ;)
// +++ Made them const regardless.
const char pdf = predefine[i] ;//takeout from array and store in var.pdf
const char pdf2 = predefine[i] ;
// ### What is going on here? "compare character" does not help me! Again, using two different vars with the same value.
if(pdf == keytake && pdf2 == character)//compare character
{
// ### "answer" is a bad name - I didn't ask a question! From the comment, it sounds like
// ### "cipherPos" or something would be a better name, in which case rename it to that and remove the comment.
// ### Describe what the algorithm is doing, here.
// ### What is the significance of "26"? Why are we adding things together and taking the answer modulo it?
// ### Magic numbers in general are undesirable as they tell me nothing and are a pain to update if the number
// ### needs to change. If "26" is supposed to be the length of "predefine", then define it as a constant with a sensible name, right next to
// ### predefine.
// +++ Made const regardless.
int answer = pdf + pdf2 % 26; //var.answer will have position of
//cipher wewill determine
// +++ Replaced switch statement with something more compact.
const char sensibleNameForThisIDoNotKnowWhatItIs = 'a' + answer;
cout << sensibleNameForThisIDoNotKnowWhatItIs;
}
}
// +++ system("PAUSE") is not portable.
system("PAUSE");
return 0 ;
}
Adv Reply
?
Originally Posted by hatsoff
Bookmarks