Using break and return (structured programming): good or bad?
-
Hi guys.
I was rewriting a function of our code to our new internal implementation of some functions, and in the meantime I also rewrote the logic of it. Is something quite simple: asking for a PIN and confirming it. In case of an unaccepted PIN, ask again. In case of cancel in the dialog, stop.
But the way it was implemented previously was quite odd and ugly IMHO (and also had one bug). My version looks way better in my eyes, but I'm using break and return very frequently, which goes against structured programming.
Since I've not studied computer science, etc. (just electrical engineering), I would like to know some opinions. :-)
Original version:
@
bool isNotAsserted = true;
QString newPin, rePin;//New PIN
while(isNotAsserted){
if(this->showNumPad(&newPin, tr("New PIN Number"), true)){
if(newPin.length()!=PIN_LENGTH){
Message m(tr("Incorrect PIN number. PIN number value has a length of %1 digits.")
.arg(QString::number((int)PIN_LENGTH)), tr("WARNING"));
qApp->showMessage(m);
}else{
isNotAsserted = false;
}
}
}isNotAsserted = true;
//Reintroduce new PIN
while(isNotAsserted){
if(this->showNumPad(&rePin, tr("Retype PIN Number"), true)){
if(rePin.length()!=PIN_LENGTH){
Message m(tr("Incorrect PIN number. PIN number value has a length of %1 digits.")
.arg(QString::number((int)PIN_LENGTH)), tr("WARNING"));
qApp->showMessage(m);
}else if(rePin != newPin){
Message m(tr("Retyped PIN does not match with new PIN."), tr("WARNING"));
qApp->showMessage(m);
}else{
this->m_newPIN = newPin;
isNotAsserted = false;
}
}
}
@And here is my version:
@
QString pin;
QVariant input;
input = qApp->keyboardPrompt(tr("New PIN Number"), KeyboardDialog::PIN);
// When the returned QVariant is invalid, the user cancelled.
while (input.isValid()) {
pin = input.toString();
if (pin.length() == PIN_LENGTH)
break; // Break out of the loop. The PIN is acceptable.Message m(tr("Incorrect PIN number. "
"PIN number value has a length of %1 digits.")
.arg(PIN_LENGTH), tr("WARNING"));
qApp->showMessage(m);
input = qApp->keyboardPrompt(tr("New PIN Number"), KeyboardDialog::PIN);
}if (!input.isValid()) // The user cancelled: exit.
return;// Repeat the process for the PIN confirmation.
input = qApp->keyboardPrompt(tr("Retype PIN Number"), KeyboardDialog::PIN);
while (input.isValid()) {
// Now we have 2 checks: length and being equal to the previous.
if (input.toString().length() != PIN_LENGTH) {
Message m(tr("Incorrect PIN number. "
"PIN number value has a length of %1 digits.")
.arg(PIN_LENGTH), tr("WARNING"));
qApp->showMessage(m);
input = qApp->keyboardPrompt(tr("Retype PIN Number"), KeyboardDialog::PIN);
} else if (pin != input.toString()) {
Message m(tr("Retyped PIN does not match with new PIN."), tr("WARNING"));
qApp->showMessage(m);
input = qApp->keyboardPrompt(tr("Retype PIN Number"), KeyboardDialog::PIN);
} else { // All checks OK. Save the PIN.
this->m_newPIN = pin;
return;
}
}
@Feel free to be blunt. I want to learn. ;-)
-
Universal answer: it depends :-)
What bugs me in your version is that you call
@input = qApp->keyboardPrompt(tr("New PIN Number"), KeyboardDialog::PIN);@
before the loop(s) and once inside the loop(s) and therefore you got repeated code. Overall the first version is better readable (in my eyes), but I did not try to look for the bug you fixed.
-
[quote author="hardcodes.de" date="1351525344"]Universal answer: it depends :-)
What bugs me in your version is that you call
@input = qApp->keyboardPrompt(tr("New PIN Number"), KeyboardDialog::PIN);@
before the loop(s) and once inside the loop(s) and therefore you got repeated code. Overall the first version is better readable (in my eyes), but I did not try to look for the bug you fixed.[/quote]
Good point! Indeed, I was thinking in starting another thread in a more general Qt subforum, because I thought that was an issue (or design decision) with QVariant.
I also dislike the fact that I have repeated that like of code there, but at first, I didn't found a way to call .isValid() on the returned value without saving it first.
I completely overlooked that operator== is enough in this case. I'm quite sure that I looked at the documentation, and thought "what a pity, the operator I need doesn't exist". I probably had the help pane with another page opened.... :-(
I'm changing it now.
-
In my opinion people are too much scared of using jumps in their code. If you inspect some low level and kernel code you will find a lot of goto statements, that in my opinion are an even better way of masquerading a return (I mean goto endoffunction). The only problem I see with inner returns is that someone reading the code will miss such statement and will continue reading the code below. With regard to the break instead I use it very often.