6
\$\begingroup\$

I am currently learning C++, using this book: C++ Primer, 5th Edition. I made this program to help me understand the usage of vectors in C++. I have prior programming experience (mainly Python), and this just seems way too messy to be the right way of doing things. If I'm doing this incorrectly, or inefficiently, then please guide me in the right direction.

The purpose of this program is to take user input, and convert it into random letters (yes, I know that there is a lack of support for #'s and symbols, but that does not matter), somewhat simulating a hashing system.

This isn't homework, and it's not a dire issue. The program runs, but I want to make sure (since I'm new to C++), that I'm doing things in a somewhat correct way.

#include <iostream>
#include <string>
#include <vector>
int main(){
    srand(time(0));
    std::cout << "Welcome, to the scrambler: " << std::endl;
    std::cout << "Enter the string(all lowercase letters) you want to be scrambled: " << std::endl;
    std::string user_password;
    std::cin >> user_password;


    std::cout << "Select the type of encryption you want: " << std::endl;
    std::cout << "1. Standard alphabet scrambler " << std::endl;
              //Add support for other types...
    int choice = 0;
    std::cin >> choice;

    if (choice == 1) {

        std::vector<char> vuser_pass;

        for ( decltype(user_password.size()) i = 0; i <= user_password.size(); ++i ) {
            char letter_hold = user_password[i];
            vuser_pass.push_back(letter_hold);
        }
        //for ( decltype(vuser_pass.size()) i = 0; i <= vuser_pass.size(); ++i) {
            //std::cout << vuser_pass.at(i) << std::endl;
        //}

        std::string alpha = "abcdefghijklmnopqrstuvwxyz";
        std::vector<char> valpha;


        for ( decltype(alpha.size()) cnt = 0; cnt <= alpha.size(); ++cnt) {

            valpha.push_back(alpha[cnt]);
        } 


        //for ( decltype(valpha.size()) cnt = 0; cnt <= valpha.size(); ++cnt) {
            //std::cout << valpha[cnt] << std::endl;

        //}

        auto limit = vuser_pass.size();
        limit -= 1;

        for ( decltype(vuser_pass.size()) sindex = 0; sindex < limit; ++sindex ) {
            decltype(valpha.size()) vindex = rand() % 25;
            vuser_pass[sindex] = valpha.at(vindex);
        }

        for ( decltype(vuser_pass.size()) i = 0; i < vuser_pass.size(); ++i) {
            std::cout << vuser_pass.at(i);
        }

    }
return 0;
}
\$\endgroup\$

1 Answer 1

6
\$\begingroup\$

Before discussing whether the algorithm is good or not, there are many small things that you could improve to make the code more readable:

  • Your program won't compile with some compilers if you don't explicitly include <cstdlib> for rand and srand and <ctime> for time.

  • It is good practice to always fully qualify with std:: the names of the standard library components you use. Therefore, you should explicitly write std::rand, std::srand and std::time.

  • Don't keep commented out comments. They will hardly help you and will probaby just be in the way. If you need to look again at old pieces of code, revision control software are there for you.

  • This loop:

    std::vector<char> vuser_pass;
    for ( decltype(user_password.size()) i = 0; i <= user_password.size(); ++i ) {
        char letter_hold = user_password[i];
        vuser_pass.push_back(letter_hold);
    }
    

    If I am not mistaken, you are simply asiigning every element of the string user_password to the vector vuser_password. In this case, please use the std::vector's constructor which takes two iterators:

    std::vector<char> vuser_pass(std::begin(user_password), std::end(user_password));
    

    This do what you need and you will avoid having to manually write a for loop. Note that it is also valid for the alpha/valpha conversion.

  • That said, do you need std::vectors at all? You can as well iterator directly on std::string since it implements the containers methods.

  • This loop:

    for ( decltype(vuser_pass.size()) i = 0; i < vuser_pass.size(); ++i) {
        std::cout << vuser_pass.at(i);
    }
    

    If you use get rid of the vectors and simply use strings, you can directly output the resulting string to std::cout:

    std::cout << user_password;
    
  • You don't need to return 0; at the end of main. If nothing is returned, the compiler wil automagically add a return 0; for you at the end. Not wirting it by yourself amy help to document the fact that you won't ever be returning anything else than 0.

  • You could a range-based for loop to simplify the latest remaining loop:

    for (char& c: user_password) {
        c = alpha[rand() % 25];
    }
    
  • Also, std::rand and std::srand are kind of advised against in modern C++ since there are safer alternatives in the new <random> header. However, the alternative is not that easy to use yet.

    std::random_device rd;
    std::mt19937 gen(rd());
    std::uniform_int_distribution<> dis(0, 25); // will produce ints in [0, 25]
    

    Now, you can create random integers between 0 and 25 by calling dis(gen).

  • Since alpha is not meant to be modified, it would be better to make it const.


So, let's take all of these modifications into account and write your code again:

#include <algorithm>
#include <iostream>
#include <random>
#include <string>

int main(){

    // random number generator in [0, 25]
    std::random_device rd;
    std::mt19937 gen(rd());
    std::uniform_int_distribution<> dis(0, 25);

    std::cout << "Welcome, to the scrambler: " << std::endl;
    std::cout << "Enter the string (all lowercase letters) you want to be scrambled: " << std::endl;
    std::string user_password;
    std::cin >> user_password;

    std::cout << "Select the type of encryption you want: " << std::endl;
    std::cout << "1. Standard alphabet scrambler " << std::endl;
              //Add support for other types...
    int choice = 0;
    std::cin >> choice;

    if (choice == 1) {

        const std::string alpha = "abcdefghijklmnopqrstuvwxyz";

        for (char& c: user_password) {
            c = alpha[dis(gen)];
        }

        std::cout << user_password;
    }
}

Voilà! Your program has been converted to somewhat clean modern C++. Now, all you have to do is to provide more scrambling algorithms and to provide a mechanism to tell the user that they have chosen an invalid option and that they should chose again.

\$\endgroup\$
8
  • \$\begingroup\$ Didn't you meant to use dis() for this line: c = alpha[rand() % 25];, otherwise, the uniform_int_distribution is not being used. \$\endgroup\$
    – glampert
    Nov 29, 2014 at 14:08
  • \$\begingroup\$ @glampert Right, I overlooked that. Thanks :) \$\endgroup\$
    – Morwenn
    Nov 29, 2014 at 16:31
  • \$\begingroup\$ I believe alpha can use constexpr here. \$\endgroup\$
    – Jamal
    Nov 29, 2014 at 16:35
  • \$\begingroup\$ @Jamal, I've tested constexpr std::string on Clang and it failed to compile. This is the expected because std::string doesn't have a constexpr constructor. But at any rate, the alpha variable should indeed be a const. \$\endgroup\$
    – glampert
    Nov 29, 2014 at 16:47
  • \$\begingroup\$ @glampert: That'd be better, then. I do keep forgetting the rules of constexpr. \$\endgroup\$
    – Jamal
    Nov 29, 2014 at 16:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.