Pozdrawiam
Tymon Radzik
---------- Wiadomość przekazana -----------
Od: "Tymon Radzik" <dwg...@gmail.com>
Data: 6 lip 2014 23:58
Temat: Re: Bug#753898: RFS: Looking for sponsor for apg-gui
Do: "Sune Vuorela" <s...@vuorela.dk>
DW:
I generally agree with reservations.
I currently intensivly work to improve code read-ability and funcionality.
I think next, strongly improved, upstream release of my package will be
sent to DebianExpo in next 2-3 days.
Thank You for Your hints.
Regards,
Tymon Radzik
6 lip 2014 22:28 "Sune Vuorela" <s...@vuorela.dk> napisał(a):
> On Saturday 05 July 2014 23:36:12 Tymon Radzik wrote:
> > I have just uploaded to mentors my package - it is called apg-gui and is
> > complex Graphical User Interface for Automated Password Generator. It
> > supports all it functions. I have made it in C++ Qt. I think it should be
> > put into Debian archive, because:
>
> I think - after a quick browse over the upstream source code, that the
> project
> isn't yet ready to be shipped by Debian.
>
> Some of the initial issues I noted in the first file I looked at:
>
> |int n,m,sx,a;
> |QString mode, exclude, salt;
>
> These looks like they are treated as class members. Please make them class
> members rather than file local variables.
> The variable names also is kind of ... brief.
>
>
> |std::string Scores::exec(char* cmd) {
> | FILE* pipe = popen(cmd, "r");
> | if (!pipe) return "ERROR";
> Using 'magic strings' instead of enums usually makes code harder to
> understand
> | char buffer[128];
> | std::string result = "";
> | while(!feof(pipe)) {
> | if(fgets(buffer, 128, pipe) != NULL)
> | result += buffer;
> | }
> | pclose(pipe);
> | return result;
> |}
> Try look into using QProcess instead of trying to do process handling by
> hand.
>
>
> | ui->progressBar->setValue(5);
> progressBar looks like a autogenerated value. Please use descriptive
> variable
> names.
>
>
> | std::string wyniki = std::string("apg -q -n ") + std::to_string(n) +
> " -m
> | " + std::to_string(m) + " -x " + std::to_string(sx) + " -a " +
> | std::to_string(a);
> Instead of playing hard-to-read scrabble with the strings, try use some
> simpler substitution pattern, maybe even QString, since you have it handy
> and
> use it already.
>
> But given this is to be passed on to a process handling tool, putting every
> component seperately in a QStringList and then handing it off to QProcess
> could
> improve the readability.
>
> | if(exclude != "") wyniki+=" -E " + exclude.toStdString();
> Try use the empty/isEmpty function rather than comparing to the empty
> string.
>
> | char *cstr = new char[wyniki.length() + 1];
> | strcpy(cstr, wyniki.c_str());
> Where is cstr free'd ?
>
>
> | void Scores::on_pushButton_clicked()
> what's the pushButton ?
>
> | void Scores::on_pushButton_2_clicked()
> What's pushButton_2
>
> And the body of this function has all the same issues as mentioned in the
> ctor, including the unhandled memory.
>
> /Sune
> --
> I didn’t stop pretending when I became an adult, it’s just that when I was
> a
> kid I was pretending that I fit into the rules and structures of this
> world.
> And now that I’m an adult, I pretend that those rules and structures exist.
> - zefrank
>
