Overall
A good first go.
I have some minor points below. But the main thing I think you need to read up on is passing parameters by reference.
bool isMatch(string key, string confirmKey)
Here, the parameters key
and confirmKey
are passed by value. This means a copy of the original value is created and passed to the function. When you have large or expensive objects, you would prefer not to copy them (strings are considered expensive as you have to allocate memory and then copy all the bytes in the string to the new location).
Since you don't do much with the strings (all non mutating functions) you could simply pass a reference to the original object (a const version, so you know the function will not change them) and then you can compare those:
bool isMatch(std::string const& key, std::string const& confirmKey)
{
return key == confirmKey;
}
CodeReview
This is your header?
#include "sha256.h"
Doing sha256 is non-trivial, and I would recommend you leave that to experts. As such, I would expect this to be an installed library and thus be included with <>
i.e. #include <sha256.h>
Don't do this.
using namespace std;
This is going to get you in trouble in the long run. You should read the article Why is "using namespace std;" considered bad practice? In my opinion, the second answer is the best.
Rather than doing this, prefix standard library types objects with std::
. It's only 5 characters, and the name std
was designed to be short for that reason.
Please, Please, make sure your indentation is good. This makes it really hard to read.
bool keyExists() {
bool keyExists = false;
ifstream inFile("key.txt");
All this function does is check that the file "key.txt" exists and is openable. You don't do anything with that information in the file!
bool keyExists() {
bool keyExists = false; // Not to keen on the variable haveing
// the same name as the function.
// Feels like you will get confused reading
// this.
ifstream inFile("key.txt");
if (inFile) {
keyExists = true;
}
return keyExists;
}
We can shorten this function considerably if that is all you want to do.
bool doesKeyFileExist()
{
std::ifstream inFile("key.txt");
bool keyFileExist = inFile; // there is an auto conversion from
// ifstream to bool (which you use in
// your if (inFile). You don't need
// to be so roundabout. Just check the
// result and return it.
return keyFileExist;
}
This is an antipattern
bool match = false;
if (key == confirmKey) {
match = true;
}
If you see code like:
bool test;
if (>checkSomeCondition<) {
test = true;
}
else {
test = false;
}
This is just long hand notation of finding the value of >checkSomeCondition<
as this must be a boolean value (or convertible to boolean). So you can simply assign this condition to your variable:
bool test = >checkSomeCondition<;
Does exactly the same thing.
So your code can be re-written as:
bool match = (key == confirmKey);
Put preconditions checks first and bail out if the preconditions fail:
// i.e.
if (>CheckConditionIsOK<) {
// Do all the work
// this could be long
}
else {
// Print error message
}
I would reorder that like this:
if (! >CheckConditionIsOK<) {
// Print error message
// Your pre-condition check has not worked so bail.
return; // or throw exception.
}
// This part of the code is only executed if your precondition
// are all valid. But they are not indented now. So it looks like
// the main part of the work.
// Do all the work
// this could be long
Declare variables as close to the point of use as possible.
string key;
string storedKey;
cout << "Please enter key: ";
getline(cin, key);
SHA256 sha256;
ifstream inFile("key.txt");
getline(inFile, storedKey);
if (isMatch(sha256(key), storedKey)) {
I would re-order this a bit:
std::cout << "Please enter key: ";
std::string key;
std::getline(std::cin, key);
std::ifstream inFile("key.txt");
std::string storedKey;
std::getline(inFile, storedKey);
SHA256 sha256;
if (isMatch(sha256(key), storedKey)) {