4
\$\begingroup\$

This is just straight bruteforce with the ability to select multiple charset for one word, being able to mash up the linearity with a modulo. This code is also posted on GitHub.

Generateur.h

#include <stdlib.h>
#include <stdio.h>
#include <string.h>

class Generateur
{
private:
    bool two, three;
    int length1, length2, length3, min, max, loop, size3;
    char *first, *middle, *last, *tmp, *ptr;
    long counter2, counter3, *array;
    void exiterr(char *msg);
    void gen(long index0);

public:
    Generateur(int argc, char *argv[]);
    ~Generateur();
};

zhou.cpp

#include "Generateur.h"

int main(int argc, char *argv[])
{
    Generateur gen(argc, argv); 
}

Generateur.cpp

#include "Generateur.h"

Generateur::Generateur(int argc, char *argv[])
{
    if  (argc < 4)              { printf("./zhou min max [first] middle [last [length]].\n");exit(-1); }
    if(!(min = atoi(argv[1])))  { printf("./zhou min max [first] middle [last [length]].\n");exit(-1); }
    if(!(max = atoi(argv[2])))  { printf("./zhou min max [first] middle [last [length]].\n");exit(-1); }

    long index0;//this one runs
    two = three = false;
    index0 = loop = 0; //index is the core variable with the array, loop is needed in case max > min
    counter2 = counter3 = 0;
    size3 = 1; //when not used we need it anyway because we are substracting it to the password's length for looping 

    array = new long[max];
    tmp = new char[max];
    first = new char[strlen(argv[3])];
    strcpy(first, argv[3]);
    length1 = strlen(first);

    if(argc > 4)//is there a second charset ?
    {
        two = true;
        middle = new char[strlen(argv[4])];
        strcpy(middle, argv[4]);
        length2 = strlen(middle);
    }
    if(argc > 5)//a third ?
    {
        if(argc < 7)    { printf("Needs expand of last character set\n"); exit(-1); }
        three   = true;
        last    = new char[strlen(argv[5])];
        strcpy(last, argv[5]);
        length3 = strlen(last);
        size3 = atoi(argv[6]);
    }
    for(int i=0; i<max; i++)
    {
        array[i] = 0;   
    }
    gen(index0);//Entry
}

Generateur::~Generateur()
{
    delete(tmp);
    delete(first);
    if(two) 
        delete(middle);
    if(three)
        delete(last);
}

void Generateur::gen(long index0)
{
    if(index0 == 0)
    {
        for(loop=0; loop<max-min+1; loop++)
            for(array[index0]=0; array[index0]<length1; array[index0]++)
                if(index0 < min+loop-1) gen(index0+1);
    }
    else
    {
        for(array[index0]=0; array[index0]<length1; array[index0]++)
        {
            if(index0 < min+loop-1 && !two) gen(index0+1);
            else
            {
                ptr = &tmp[0];
                for(int a=0; a < min+loop; a++)
                {
                    snprintf(ptr, 2, "%c", first[(array[a]+counter3)%length1]);
                    ptr++;
                }
                if(two)
                {
                    for(array[index0]; array[index0]<length2; array[index0]++)
                    {
                        if(index0 < min+loop-size3) gen(index0+1);
                        else
                        {   
                            ptr = &tmp[1];
                            for(int a=0; a < min+loop-size3; a++)
                            {
                                snprintf(ptr, 2, "%c", middle[(array[a+1]+counter2)%length2]);
                                ptr++;                          
                            }
                            if(three)
                            {
                                for(array[index0]; array[index0]<length3; array[index0]++)
                                {
                                    if(index0 < min+loop-1) gen(index0+1);
                                    else
                                    {
                                        ptr = &tmp[min+loop-size3];
                                        for(int a=0; a<size3; a++)
                                        {
                                            snprintf(ptr, 2, "%c", last[(array[min+loop-size3+a])%length3]);
                                            ptr++;
                                        }
                                        printf("%s\n", tmp);
                                        counter3++;
                                    }
                                }
                            }
                            else
                                printf("%s\n", tmp);
                            counter2++;
                        }
                    }
                }
                else
                    printf("%s\n", tmp);
            }
        }
    }
}
\$\endgroup\$
6
\$\begingroup\$

This is very much C style code. You could likely extract the functionality out of your Generateur class into reusable functions and the reclassify this as C.

If you wanted to keep this in C++ I would completely rewrite this.

Start with your headers. Use the C++ versions of C Library headers. They are prefixed with c and don't end in .h:

#include <cstdlib>
#include <cstdio>
#include <string>

Also use conventional C++ constructs. C++ has RAII containers that handle memory management for you. It has iterators as well so you don't need to maintain raw pointers.

I/O is typically done with <iostream>

You would also benefit from not declaring multiple variables on a single line. It is error prone and difficult to read.

It is also customary in C++ to put the type specifier with the type not the variable. This is part of the reason that doing multiple declarations on a single line can be error prone.

char *first, *middle, *last, *tmp, *ptr;

would look like this:

char* first;
char* middle;
char* last;
char* tmp;
char* ptr;

You would also benefit from the use of more whitespace around your operators. It's simply more readable. Also prefer prefix increment/decrement to postfix. Postfix returns an unmodified copy, so unless you need the copy don't use it.

for(int a = 0; a < (min + loop); ++a)

class is private by default. It is however conventional to list public members and functions first and then declare private members and functions. The reason is anyone (including you) using your class needs to know what publicly accessibly parts they have available. The implementation details however rarely matter (once you get them working right).

You should also make the class reusable. your Generator runs on construction but there is no public function to use it again. Once it's used it can't be used again. might as well make it a function then. Make a callable function like generate_pass() and have it make a new password for you. You can have the function accept input if needed. perhaps seed it from a PRNG.

You should also initialize member variables in your class whenever possible. Assignment in the constructor is not the same thing. There are a couple different methods for this but I will just start with brace initialization because that is the most preferred where possible. It looks something like this:

private:
    bool two{ false };
    bool three{ false };
\$\endgroup\$
  • \$\begingroup\$ Hello, thank you for these advices, I just fixed the program where it broke for now, because there was huge mistaking with the use of modulo, and changed headers, appart from that, what you said I will fix it on my main files that I host on github first: github.com/e2002e/zhou \$\endgroup\$ – Yvain Nov 10 '18 at 12:01
  • \$\begingroup\$ I want to say that what you read about postfix and prexfix increment you might just forget it, postfix increases before the loop starts, prefix increases after an iteration of the loop ends, i have the oreilly c++ reference with me and i'm not even gonna argue about that. I need a at 0 so I use postfix. \$\endgroup\$ – Yvain Nov 10 '18 at 12:40
  • \$\begingroup\$ also I'm reading about brace initializers, they are meant for const data \$\endgroup\$ – Yvain Nov 10 '18 at 12:56
  • \$\begingroup\$ @Yvain niether of those things are true. \$\endgroup\$ – bruglesco Nov 10 '18 at 13:17
  • \$\begingroup\$ @Yvain Try for (int i = 0; i < 10; ++i) { std::cout << i; } and see for yourself. And const data must be initialized, that doesnt mean it's not good practice to initialize non-const members wherever possible. \$\endgroup\$ – bruglesco Nov 10 '18 at 14:02
5
\$\begingroup\$

Your functions are so long, and do so much things.

Yeah, sometimes, the size matter. Although, there is no true limit number of lines, usually, a too long function expose a poor design.

One function (or method) have to do one thing, and only one. If this "one thing" can broken into smaller things, each of those should be placed into a separated function. It's a kind of "Single Responsability Principle".

Perfection is finally attained not when there is no longer anything to add, but when there is no longer anything to take away. - A. de Saint Exupéry

Eg, talking about me, I almost always try to don't excess 30-35 lines of codes for a function, because it force me to separate logic units. With a short function, you can immediately determine it purpose.

A shorter function, by definition (most of the time), is simpler to understand. It's also easier to debug if you get a problem later. It's the "KISS principle"

Everything should be made as simple as possible, but not simpler. - A. Einstein

Therefore, a good practice to following is to avoid deep nesting because it force you to keep like a "stack trace" in mind and overload your brain. In Generateur::gen you have 21 structure of control and the deepest is at the 11th level. Don't you think it's too much?


Also, in many place, you repeat same code, again and again. So would be to try to factorize your code, applying the Don't Repeat Yourself principle. This one make reading, writing, editing of your code easier and faster, since we only have to read, write or modify once.

That also prevent your code to being a huge "copy and past" fest. So, instead of hard-coding three times how using your program, write a ´usage` function where you print that, all call it from where you want.

For your arguments parsing, you can try to code a tiny reusable utility for parsing command lines or rely on an already existing solution.


A lot of your code can be simplified using Standards C++ classes, but I will not go into too much detail about what should be changed in your code because I think it's all the code that needs to be rewritten taking into account everything I've said. In summary:

  • Make the functions shorter
  • Less use of conditional branching
  • Factorize into pieces of reusable code

  • Don't use old C Library header when you have Standard C++ features that allow you to do it easier, better, faster, stronger and safer (Hello Daft Punk). In last resort, if you have to use C Library, use the <cXXX>version instead of the <XXX.h> one.

  • Use the language facilities offered by the STL and C ++ in general.
  • Your arguments parsing can (and should) be rewrites with: std::string, std::vector and std::optional.

  • Use std::stoi (and his family) , std::stringstream or the C++17 std::from_chars instead of the old C atoi.

  • Use std::string first = argv[3]; instead of char *first; first = new char[strlen(argv[3])]; strcpy(first, argv[3]); length1 = strlen(first);
  • Learn C++ algorithms

As a final word, here is a quote from Bjarne Stroustrup, the father of C++

Using Standard C++ as a glorified C or glorified C with Classes only would be to waste the opportunities offered by Standard C++. - Bjarne Stroustrup (Learning Standard C++ as a New Language)

\$\endgroup\$
  • \$\begingroup\$ Calak, what you said I will take it into account when I start implementing markov chain on next week. The code will be hosted on github, thank you. \$\endgroup\$ – Yvain Nov 10 '18 at 19:35
4
\$\begingroup\$

Do arg parsing in main and pass the result of that to the constructor of Generateur.

Use std::vector<long> instead of the new long[] array. Use std::string or std::vector<char> instead of the new[]ed char arrays.

This lets you remove the destructor because the fields of Generateur will take care of leaks themselves. As an aside when you use new[] you must use delete[] instead of plain delete.

snprintf(ptr, 2, "%c", middle[(array[a+1]+counter2)%length2]);

can be replaced with:

ptr[0] = middle[(array[a+1]+counter2)%length2];
ptr[1] = '\0';

however adding the null terminator only matters when printing it out.

\$\endgroup\$
  • \$\begingroup\$ Hello, thanks for the review, first i want to let you know about the fact that I cannot replace snprintf by a constant reference to the pointer because my code is recursive and prints before return, try to understand what is does (now that i edited) with just a single charset. I got it for the delete[] instead of delete, now what is std::vector<long> ? according to cplusplus.com it is an array that can grow dynamically. new[] is allready implementing malloc, that is implementing kernel's mmap, I might get too high but it might come handy. \$\endgroup\$ – Yvain Nov 10 '18 at 12:19
  • 1
    \$\begingroup\$ @Yvain after snprintf is inlined the code I posted will be exactly what it is optimized to. Wether the surrounding code is recursive or not makes no difference. The purpose of using std::vector is to manage the memory allocated and making sure it is freed after you use it. With std::vector this is automatic and much more foolproof. \$\endgroup\$ – ratchet freak Nov 12 '18 at 9:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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