Bug#753898: RFS: Looking for sponsor for apg-gui

[Sune Vuorela <sune@vuorela.dk>]

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