1. My Attempt
The first example of bad code in the Anti-If Campaign's website seems pretty trivial and standard to me. The solution, as plenty of books, other StackOverflow questions and websites have pointed out, is polymorphism.
However, the second, server example doesn't seem so straightforward to me — which is an indication that I'm not that good a programmer, I guess. I would like for someone to please confirm if my solution attempts are right; or, if not, give a better one. (My points are numbered so the answer can refer to them more easily, this is not a hierarchical ordering.)
- Even though there is only one if-null condition highlighted, there are plenty of such cases throughout that example. My guess at the best way of solving this type of issue is to use the Null Object Pattern.
- Probably the biggest issue with that piece of code isn't even the if's themselves, but the huge amount of nesting, of which we can encounter up to 5 levels.
- The
if (getArea() == 1)
conditional doesn't seem that bad to me if it is only in this piece of the codebase. But it surely doesn't reveal that much meaning and intention to the reader. What does the 1 mean? Is it a suffiency clause? Then maybe it would have been better to replace it by something likeif (validArea())
. It is also not attached to any object, so I can't easily easily establish relationships.- I think this if clause could also benefit from either a Factory or a Template Method, depending on what the relationship of the objects is — it isn't very clear to me what would be the correct choice from only that piece of code, but maybe someone more experienced could have a better grasp of the solution.
2. The Referenced Code
For reference, this is the piece of code (Java) in the website (there is some weirdness with the indentation and commenting on the else
line I think):
public ArrayList eseguiAnalisi() {
ArrayList listaSegnalazioni = new ArrayList();
List domande = null;
List risposte = null;
Domanda domanda = null;
DAOReques req = new DAOReques();
DAOArea ar = new DAOArea();
ar.setIdQuestionario(getIdQuestionario());
boolean quizDomandaNonForzata = false; // warning 7
try {
//set Aree e
if (getArea() == 1) { // Highlight #1
List aree = ar.getAree();
Area area = null;
for (int i = 0; i < aree.size(); i++) {
area = (Area) aree.get(i);
domande = req.getDomande(getIdQuestionario(),area.getIdArea());
if (domande != null) {
for (int j = 0; j < domande.size(); j++) {
domanda = (Domanda) domande.get(j);
risposte = req.getRisposteDomanda(getIdQuestionario(),domanda.getIdDomanda());
if (risposte != null)
domanda.setRisposte(risposte);
}
}
}
area.setDomandeArea(domande);
if (aree != null) // Highlight #2
setAreeDomande(aree);
} //else { // I think there was an indentation mistake in the website...
// set ListaDomande
domande = req.getDomande(getIdQuestionario());
for (int i = 0; i < domande.size(); i++) {
domanda = (Domanda) domande.get(i);
risposte = req.getRisposteDomanda(getIdQuestionario(),domanda.getIdDomanda());
if (risposte != null)
domanda.setRisposte(risposte);
}
if (domande!=null)
setDomande(domande);
//}
} catch (DAOException de) {
de.printStackTrace();
}
Aucun commentaire:
Enregistrer un commentaire