Besser lesbare Schleifen #78
Conversation
ruderphilipp
force-pushed
the
refactoring/loops
branch
from
b2d499f
to
c972a06
Compare
ruderphilipp
force-pushed
the
refactoring/loops
branch
from
c972a06
to
16e1bc8
Compare
ruderphilipp
force-pushed
the
refactoring/loops
branch
2 times, most recently
from
6ee5672
to
553da56
Compare
| Iterator<Class> keys = actionMap.keySet().iterator(); | ||
| while (keys.hasNext()) | ||
|
|
||
| for (Class key : actionMap.keySet()) |
There was a problem hiding this comment.
final ist hier und in anderen Schleifen auch denkbar, aber kann von mir aus auch so bleiben
There was a problem hiding this comment.
final wäre zwar korrekt, schadet bei kurzen Schleifen aus meiner Sicht jedoch dem Lesefluss (a.k.a. "Füllwort").
final ist ja dazu da, dass der Compiler verhindert, der Variable ein neues Objekt zuzuweisen. Da es sich hierbei um die Schleifenvariable handelt und der Code überschaubar ist, sollte das nicht passieren/ ist das Risiko sehr gering.
| @@ -178,10 +178,9 @@ public boolean isEnabledFor(Object o) | |||
| if (o instanceof AuslandsUeberweisung) | |||
| return !((AuslandsUeberweisung)o).ausgefuehrt(); | |||
|
|
|||
| AuslandsUeberweisung[] t = (AuslandsUeberweisung[]) o; | |||
| for (int i=0;i<t.length;++i) | |||
| for (AuslandsUeberweisung ueberweisung : (AuslandsUeberweisung[]) o) | |||
There was a problem hiding this comment.
Falls ich https://stackoverflow.com/a/9795631 richtig deute, sollte man es nicht an dieser Stelle casten, da das eine "unchecked cast warning" wirft.
Obwohl es natürlich weiter oben mit instanceof auf Kompatibilität geprüft wird. Ich wäre hier dennoch dafür, es konsistent mit der sonstigen Implementierung umzusetzen:
for (Object obj : o)
{
AuslandsUeberweisung ueberweisung = (AuslandsUeberweisung) obj;
Ich sehe gerade, dass es noch weitere Dateien in diesem PR betrifft.
@willuhn : Was denkst Du?
There was a problem hiding this comment.
Ich bin ehrlicherweise kein Fan von solchen reinen Refactoring-PRs. Die betreffen immer etliche Dateien, kosten mich damit viel Zeit für das Review und machen die GIT-History schlechter lesbar. Ich würde es schöner finden, wenn man solche Aufräumarbeiten jeweils an den Stellen macht, wo man ohnehin gerade wegen einem Bug oder neuen Feature beschäftigt ist.
src/de/willuhn/jameica/hbci/passports/pintan/PinTanConfigFactory.java
Outdated
Show resolved
Hide resolved
| GenericObject o = (GenericObject) this.umsaetze.get(i); | ||
| if (o.equals(node)) | ||
| for (Umsatz u : this.umsaetze) { | ||
| if (u.equals(node)) |
There was a problem hiding this comment.
Funktioniert der equals hier, oder muss node noch auf Umsatz gecastet werden?
There was a problem hiding this comment.
Ja, funktioniert. In UmsatzImpl::equals wird stets versucht, auf Umsatz zu casten:
public boolean equals(GenericObject o) throws RemoteException {
if (o == null || !(o instanceof Umsatz))
return false;
try
{
Umsatz other = (Umsatz) o;
...| { | ||
| Umsatz u = this.umsaetze.get(i); | ||
| if (u.equals(node)) |
There was a problem hiding this comment.
Funktioniert der equals hier, oder muss node noch auf Umsatz gecastet werden?
Die selbe Fragen beim nächsten equals.
-> Vermutlich funktioniert es, weil es sonst vorher auch nicht funktioniert haben dürfte (war schon immer <Umsatz>.equals(<GenericObjectNode>).
There was a problem hiding this comment.
Identische Thematik wie bei UmsatzGroup.java. Lass uns dort an einer Stelle diskutieren. Ich lasse diesen Kommentar hier weiter offen, damit wir ihn nicht vergessen, falls dort Änderungen vorgeschlagen werden.
Nutze benannte Variablen statt Iterator mit `i.hasNext()` und `i.next()`
Nutze benannte Variablen statt Index
ruderphilipp
force-pushed
the
refactoring/loops
branch
from
553da56
to
745dd8a
Compare
ruderphilipp
force-pushed
the
refactoring/loops
branch
from
745dd8a
to
add68c9
Compare
|
Bei Deinem Rebase ist folgender Commit verloren gegangen (siehe Dein Kommentar oben #78 (review)): Es wurde stets der erste Dateiname verwendet, wenn das Einlesen einer Datei nicht funktioniert hat, und nicht der Name der Datei, die die Fehler verursacht hat.Modified: src/de/willuhn/jameica/hbci/gui/controller/LicenseControl.java Randnotiz: Ich finde einen Rebase während aktivem Review immer etwas ungünstig, da ich dann unter Umständen alles erneut anschauen muss, anstelle nur den Diff der neusten Commits mit den Änderungen zu bewerten. |
|
Hallo @meigelb,
Das liegt daran, weil @willuhn die Codeanpassung bereits hinzugefügt hat (siehe 87c7c38). Somit ist sie hier nicht mehr notwendig. 😀
Aus meiner Sicht ist ein Rebase deutlich übersichtlicher als ein Aufräumen mit zusätzlichen Commits, solange es sich noch um ein Pullrequest handelt. Grund: Die Änderungen sind noch nicht im Produkt/ Produktivcode ist, sondern erstmal ein Entwurf/ Kandidat -- somit kann man daran feilen und polieren bis es passt, ohne dass nachher im Produktivcode in der Historie das Hin und Her rekonstruiert werden können muss. Dort ist dann nur das Endergebnis ersichtlich (vgl. Punkt 10 hier). Github unterstützt das, indem diejenigen Dateien im Reiter "Files changed" hervorgehoben werden, die sich geändert haben. Bei einem neuen Commit nach Deinem Review, wird jede veränderte Datei wieder aufgeklappt, der Haken bei "viewed" wird herausgenommen und ein Hinweis mit "changed since last view" erscheint. |
Durch das verbesserte
for-Konstrukt für Schleifen wird der Code verständlicher. MitCollections.addAll()wurde weiterhin der individuelle Code durch Standardmethoden ersetzt, um das Fehlerpotential zu senken.