Password generator with multiple charset











up vote
4
down vote

favorite
1












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 setn"); 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("%sn", tmp);
counter3++;
}
}
}
else
printf("%sn", tmp);
counter2++;
}
}
}
else
printf("%sn", tmp);
}
}
}
}









share|improve this question




















  • 1




    Please see What should I do when someone answers my question?. The code in the question must not be modified after it has been reviewed. I have rolled back Rev 17 → 10.
    – 200_success
    14 mins ago















up vote
4
down vote

favorite
1












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 setn"); 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("%sn", tmp);
counter3++;
}
}
}
else
printf("%sn", tmp);
counter2++;
}
}
}
else
printf("%sn", tmp);
}
}
}
}









share|improve this question




















  • 1




    Please see What should I do when someone answers my question?. The code in the question must not be modified after it has been reviewed. I have rolled back Rev 17 → 10.
    – 200_success
    14 mins ago













up vote
4
down vote

favorite
1









up vote
4
down vote

favorite
1






1





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 setn"); 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("%sn", tmp);
counter3++;
}
}
}
else
printf("%sn", tmp);
counter2++;
}
}
}
else
printf("%sn", tmp);
}
}
}
}









share|improve this question















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 setn"); 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("%sn", tmp);
counter3++;
}
}
}
else
printf("%sn", tmp);
counter2++;
}
}
}
else
printf("%sn", tmp);
}
}
}
}






c++ recursion






share|improve this question















share|improve this question













share|improve this question




share|improve this question








edited 9 mins ago









200_success

127k15148411




127k15148411










asked Nov 9 at 14:41









Yvain

1296




1296








  • 1




    Please see What should I do when someone answers my question?. The code in the question must not be modified after it has been reviewed. I have rolled back Rev 17 → 10.
    – 200_success
    14 mins ago














  • 1




    Please see What should I do when someone answers my question?. The code in the question must not be modified after it has been reviewed. I have rolled back Rev 17 → 10.
    – 200_success
    14 mins ago








1




1




Please see What should I do when someone answers my question?. The code in the question must not be modified after it has been reviewed. I have rolled back Rev 17 → 10.
– 200_success
14 mins ago




Please see What should I do when someone answers my question?. The code in the question must not be modified after it has been reviewed. I have rolled back Rev 17 → 10.
– 200_success
14 mins ago










3 Answers
3






active

oldest

votes

















up vote
6
down vote



accepted










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





share|improve this answer





















  • 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
    – Yvain
    Nov 10 at 12:01










  • 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.
    – Yvain
    Nov 10 at 12:40










  • also I'm reading about brace initializers, they are meant for const data
    – Yvain
    Nov 10 at 12:56












  • @Yvain niether of those things are true.
    – bruglesco
    Nov 10 at 13:17










  • @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.
    – bruglesco
    Nov 10 at 14:02




















up vote
5
down vote













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)







share|improve this answer





















  • 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.
    – Yvain
    Nov 10 at 19:35


















up vote
4
down vote













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 newed 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] = '';


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






share|improve this answer





















  • 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.
    – Yvain
    Nov 10 at 12:19






  • 1




    @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.
    – ratchet freak
    Nov 12 at 9:00











Your Answer





StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");

StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");

StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});

function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});


}
});














 

draft saved


draft discarded


















StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f207314%2fpassword-generator-with-multiple-charset%23new-answer', 'question_page');
}
);

Post as a guest















Required, but never shown

























3 Answers
3






active

oldest

votes








3 Answers
3






active

oldest

votes









active

oldest

votes






active

oldest

votes








up vote
6
down vote



accepted










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





share|improve this answer





















  • 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
    – Yvain
    Nov 10 at 12:01










  • 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.
    – Yvain
    Nov 10 at 12:40










  • also I'm reading about brace initializers, they are meant for const data
    – Yvain
    Nov 10 at 12:56












  • @Yvain niether of those things are true.
    – bruglesco
    Nov 10 at 13:17










  • @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.
    – bruglesco
    Nov 10 at 14:02

















up vote
6
down vote



accepted










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





share|improve this answer





















  • 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
    – Yvain
    Nov 10 at 12:01










  • 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.
    – Yvain
    Nov 10 at 12:40










  • also I'm reading about brace initializers, they are meant for const data
    – Yvain
    Nov 10 at 12:56












  • @Yvain niether of those things are true.
    – bruglesco
    Nov 10 at 13:17










  • @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.
    – bruglesco
    Nov 10 at 14:02















up vote
6
down vote



accepted







up vote
6
down vote



accepted






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





share|improve this answer












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






share|improve this answer












share|improve this answer



share|improve this answer










answered Nov 9 at 16:39









bruglesco

1,2632520




1,2632520












  • 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
    – Yvain
    Nov 10 at 12:01










  • 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.
    – Yvain
    Nov 10 at 12:40










  • also I'm reading about brace initializers, they are meant for const data
    – Yvain
    Nov 10 at 12:56












  • @Yvain niether of those things are true.
    – bruglesco
    Nov 10 at 13:17










  • @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.
    – bruglesco
    Nov 10 at 14:02




















  • 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
    – Yvain
    Nov 10 at 12:01










  • 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.
    – Yvain
    Nov 10 at 12:40










  • also I'm reading about brace initializers, they are meant for const data
    – Yvain
    Nov 10 at 12:56












  • @Yvain niether of those things are true.
    – bruglesco
    Nov 10 at 13:17










  • @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.
    – bruglesco
    Nov 10 at 14:02


















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
– Yvain
Nov 10 at 12:01




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
– Yvain
Nov 10 at 12:01












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.
– Yvain
Nov 10 at 12:40




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.
– Yvain
Nov 10 at 12:40












also I'm reading about brace initializers, they are meant for const data
– Yvain
Nov 10 at 12:56






also I'm reading about brace initializers, they are meant for const data
– Yvain
Nov 10 at 12:56














@Yvain niether of those things are true.
– bruglesco
Nov 10 at 13:17




@Yvain niether of those things are true.
– bruglesco
Nov 10 at 13:17












@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.
– bruglesco
Nov 10 at 14:02






@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.
– bruglesco
Nov 10 at 14:02














up vote
5
down vote













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)







share|improve this answer





















  • 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.
    – Yvain
    Nov 10 at 19:35















up vote
5
down vote













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)







share|improve this answer





















  • 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.
    – Yvain
    Nov 10 at 19:35













up vote
5
down vote










up vote
5
down vote









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)







share|improve this answer












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)








share|improve this answer












share|improve this answer



share|improve this answer










answered Nov 10 at 17:16









Calak

1,822214




1,822214












  • 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.
    – Yvain
    Nov 10 at 19:35


















  • 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.
    – Yvain
    Nov 10 at 19:35
















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.
– Yvain
Nov 10 at 19:35




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.
– Yvain
Nov 10 at 19:35










up vote
4
down vote













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 newed 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] = '';


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






share|improve this answer





















  • 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.
    – Yvain
    Nov 10 at 12:19






  • 1




    @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.
    – ratchet freak
    Nov 12 at 9:00















up vote
4
down vote













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 newed 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] = '';


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






share|improve this answer





















  • 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.
    – Yvain
    Nov 10 at 12:19






  • 1




    @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.
    – ratchet freak
    Nov 12 at 9:00













up vote
4
down vote










up vote
4
down vote









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 newed 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] = '';


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






share|improve this answer












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 newed 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] = '';


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







share|improve this answer












share|improve this answer



share|improve this answer










answered Nov 9 at 15:24









ratchet freak

11.6k1340




11.6k1340












  • 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.
    – Yvain
    Nov 10 at 12:19






  • 1




    @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.
    – ratchet freak
    Nov 12 at 9:00


















  • 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.
    – Yvain
    Nov 10 at 12:19






  • 1




    @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.
    – ratchet freak
    Nov 12 at 9:00
















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.
– Yvain
Nov 10 at 12:19




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.
– Yvain
Nov 10 at 12:19




1




1




@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.
– ratchet freak
Nov 12 at 9:00




@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.
– ratchet freak
Nov 12 at 9:00


















 

draft saved


draft discarded



















































 


draft saved


draft discarded














StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f207314%2fpassword-generator-with-multiple-charset%23new-answer', 'question_page');
}
);

Post as a guest















Required, but never shown





















































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown

































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown







Popular posts from this blog

Ottavio Pratesi

Tricia Helfer

15 giugno