Pozdrawiam
Tymon Radzik
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
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