Need a second opinion on that piece of code...

[homerun4711]

Hello!

I wrote a class to send mail using Qxt, it works but I am sure lots of things could be done better. Any suggestions?


Mailsender::Mailsender()
{
...
connect(smtp, SIGNAL(mailSent (int)), this, SLOT(sendingMailSucceeded ( int )));
connect(smtp, SIGNAL(mailFailed (int,int)), this, SLOT(sendingMailFailed ( int , int )));
...
int rv = smtp->send(*message); //send mail

rvMailSend = 1; //private variable to set mail status

//private QProgressBar*
progress = new QProgressDialog("Sending mail...","abort" ,0 , 0, this);
progress->setWindowModality(Qt::ApplicationModal);
progress->setCancelButton (0); //do not show

//private QTimer*
Timer1 = new QTimer(this);
Timer1->start(1000);
connect(Timer1, SIGNAL(timeout()), this, SLOT(mailSendingLoop()));

Timer2 = new QTimer(this);
Timer2->setSingleShot(true);
Timer2->setInterval(10000);
Timer2->start();
connect(Timer2, SIGNAL(timeout()), this, SLOT(mailSendingTimeout()));

progress->exec(); //show progressbar

} //end of constructor


void MailSender::sendingMailFailed ( int mailID, int errorCode )
{
rvMailSend = 1; //set via signal if mail failed
}

void MailSender::sendingMailSucceeded ( int mailID )
{
rvMailSend = 0; //set via signal if mail succeeded
}


int MailSender::mailSendingLoop() //called regular by a QTimer
{
if(rvMailSend == 0)
{
progress->close();
QMessageBox msgBox;
msgBox.setText("Mail send!");
msgBox.setStandardButtons(QMessageBox::Ok);
msgBox.setDefaultButton( QMessageBox::Ok);
Timer1->stop();
Timer2->stop();
int ret = msgBox.exec();

if (ret == QMessageBox::Ok)
{
this->close();
return 0;
}

}


}

int MailSender:: mailSendingTimeout() //Timeout, called on Timer2
{
progress->close();
QMessageBox msgBox;
msgBox.setText("Sending mail failed!");
msgBox.setStandardButtons(QMessageBox::Ok);
msgBox.setDefaultButton( QMessageBox::Ok);
Timer1->stop();
Timer2->stop();
int ret = msgBox.exec();

if (ret == QMessageBox::Ok)
{
this->close();
return 1;
}

}

Kind regards,
HomeR[/CODE]

[high_flyer]

From design perspective, you are doing a lot of actual work in the constructor - its not the place for it - constructors are for initialization.
You say it works, so I wont argue, but your constructor is sending a message, without taking the message as parameter - so do you always send a hard coded message?
Also, you are using a progress bar, but never update it, you only look if the message is sent or not (by the way you need to correct "send" to "sent" when you use past tense) - so what is the progress bar for?

[homerun4711]

Yes, true, the constructor is a bit overloaded, I often do this first and split the stuff into functions later, bad habbit :)

The progress bar is just used a busy indicator, this is possible by setting setMinimum and setMaxium to 0:


progress = new QProgressDialog("Sending mail...","abort" ,0 , 0, this);


but your constructor is sending a message, without taking the message as parameter - so do you always send a hard coded message?

I don't know what you mean, could you please explain?

[stampede]

Having some kind of timeout mechanism is sounds sensible to me, but what is the purpose of checking if a value is 0 every second ? Can't you stop the timers and close progress when you receive "mailSent" signal ? Code could be clearer with just timeout timer.
Another thing is that there is no error reporting at all, if sending fails you'll have no clue why, maybe it was not your fault at all ( some kind of server problems for example ).
Last thing I'd change is the message box code:

QMessageBox msgBox;
msgBox.setText("Mail send!");
msgBox.setStandardButtons(QMessageBox::Ok);
msgBox.setDefaultButton( QMessageBox::Ok);
Timer1->stop();
Timer2->stop();
int ret = msgBox.exec();

if (ret == QMessageBox::Ok)
{
this->close();
return 0;
}

could be simpler by using:


QMessageBox::information(this,"",tr("Mail sent!"));
Timer1->stop();
Timer2->stop();
this->close();
return 0;

Same for timeout message box (maybe with QMessageBox::warning or critical).
I hope that it helps you somehow

[high_flyer]

I don't know what you mean, could you please explain?
See line 7 in your code above.

[homerun4711]

Thanks for the good hints so far, some way to go :)



but your constructor is sending a message, without taking the message as parameter - so do you always send a hard coded message?

Yes, this was my original plan, to check this return value and go on from there, but as this does only return the "message number" and does not show if the message was send or not. This is the funtion:


int QxtSmtp::send(const QxtMailMessage& message)
{
int messageID = ++qxt_d().nextID;
qxt_d().pending.append(qMakePair(messageID, message));

if (qxt_d().state == QxtSmtpPrivate::Waiting)
qxt_d().sendNext();

return messageID;
}


Can't you stop the timers and close progress when you receive "mailSent" signal ? Code could be clearer with just timeout timer.

Yes, I thought of this, but since I am not so familiar with Qt yet, I don't know how long the life cycle of the MailSender-object is. So I thought to keep it alive with a QTimer, does this sound stupid?
Ah, I guess I get what you mean - let just the singleShot-QTimer run, and wait for the signal?


Another thing is that there is no error reporting at all, if sending fails you'll have no clue why

Yes, this will be done for sure. And I will change the MessageBoxes, too.

[stampede]

Ah, I guess I get what you mean - let just the singleShot-QTimer run, and wait for the signal?
Yes, leave just the "timeout" timer, and move the "finalizing" code from mailSendingLoop to sendingMailSucceeded method.

I don't know how long the life cycle of the MailSender-object is
Well, its up to you to make sure that smtp pointer will remain valid as long as you need it :) Btw is it a member of MailSender class ? It should be IMHO.

[homerun4711]

Btw is it a member of MailSender class ? It should be IMHO

Yes, it is.

Thanks for all the good hints!