DiveShare patch

Mon Oct 13 13:26:07 PDT 2014

On Monday 13 October 2014 21:23:59 Salvo Tomaselli wrote:
> How about now?
> 
> I used astyle and I think I've addressed all the comments.

Thanks Salvo.

This looks better. I haven't run your code yet, but it looks like it does what 
you're proposing to do. 

I only have one issue: your code leaks memory if the user clicks the "Upload 
dive data" button twice before the request finishes, as in doUpload you simply 
assign the new QNetworkReply object to this->reply. You probably want to 
disable the button right after the PUT request and reenable it in the 
finishedSlot. You should also set the mouse cursor to a busy indicator.

The rest of the comments are nitpicking:

> +DiveShareExportDialog::~DiveShareExportDialog()
> +{
> +       delete this->ui;
> +       delete this->reply;
> +}

Nitpicking: we don't usually use the "this->" syntax, though I do see it in a 
few places in the current source code. Unless Dirk has a strong preference, 
I'd leave it.

> +static QString generate_html_list(QByteArray data)
> +{
> +       QStringList dives = QString(data).split("\n");
> +       QByteArray html;
> +       html.append("<html><body><table>");
> +       for (int i = 0; i < dives.length(); i++ )
> +       {
> +               html.append("<tr>");
> +               QStringList dive_details = dives[i].split(",");
> +               if (dive_details.length() < 3)
> +               {

Optimisations:
1) make the function take a const QByteArray &data
2) use 
	QList<QByteArray> dives = data.split('\n');
	QList<QByteArray> dive_details = dives[i].split(',');
note the single quotes. And use QByteArray instead of QString further down. 
Your code is going back and forth between QString and QByteArray, which is 
inefficient.

Nitpicking: the brace for the for and if should be on the lines above. Your 
original patch had it in the right place, but this one is wrong. There are 
other places in the code that need this change.

> +    this->ui->txtResult->setHtml(reply->errorString());

Optimisation: you probably want setPlainText instead of setHtml, since the 
error string is plain text, thus we don't need to parse anything. I don't 
think we return anything that could be parsed as HTML, but better be safe than 
sorry.

> +       QObject::connect(
> +
> +           this->reply, SIGNAL(finished()),
> +           this, SLOT(finishedSlot())
> +       );

Nitpicking: excess wrapping. I guess astyle didn't understand the SIGNAL and 
SLOT macros...

And the indentation of the continuation lines don't look to be one-tab.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
      PGP/GPG: 0x6EF45358; fingerprint:
      E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358