Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove redundant local variable #92

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ruderphilipp
Copy link
Contributor

Entfernt doppelt erzeugte Referenzen auf ein identisches Objekt. Diese stellt nur ein Alias dar und hat keinen Mehrwert, wenn direkt darunter bereits schon wieder eine Zuweisung/ ein return erfolgt.

@ruderphilipp ruderphilipp force-pushed the refactoring/redundant_local_variable branch from 3cba074 to e39e737 Compare December 13, 2021 11:40
@@ -116,8 +116,7 @@ public void format(TableItem item) {
item.setForeground(Color.COMMENT.getSWTColor());

// Checken, ob der Auftrag einen Reminder hat oder ob es ein geclonter Auftrag ist
HibiscusDBObject o = (HibiscusDBObject) l;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Durch Entfernen der lokalen Variable o, wird die Semantik der Identität bei ihrer Verwendung gebrochen. Der Code sollte daher an dieser Stelle bleiben, wie er war.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hallo @meigelb,
ehrlich gesagt, verstehe ich nicht, was Du mit "die Semantik der Identität bei ihrer Verwendung" ausdrücken möchtest.

Die Aussage ist "checke ob l ein Klon ist", wozu in Zeile 119 die UUID und in Zeile 133 das Template verwendet wird.

Damit der Code compiliert, ist der Downcast nicht notwendig, da die Variable l vom Typ SepaSammelTransfer ist, welches wiederum ein HibiscusDBObject ist, und somit automatisch beim Funktionsaufruf entsprechend gecastet wird:

public interface SepaSammelTransfer<T extends SepaSammelTransferBuchung> extends HibiscusDBObject, Terminable, Duplicatable, SepaPayment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, ich weiß auch nicht, was ich damals dort gesehen habe. Aus heutiger Sicht passt es. Vielleicht habe ich nicht erkannt, dass l ein Objekt vom Typ SepaSammelTransfer ist, sondern das als eine 1 gelesen. Danke fürs kritische Hinterfragen.

@@ -95,8 +95,7 @@ else if (l.ausgefuehrt())
item.setForeground(Color.COMMENT.getSWTColor());

// Checken, ob der Auftrag einen Reminder hat oder ob es ein geclonter Auftrag ist
HibiscusDBObject o = (HibiscusDBObject) l;

This comment was marked as resolved.

@ruderphilipp ruderphilipp marked this pull request as draft January 11, 2022 07:15
Copy link
Contributor

@meigelb meigelb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bitte die Vorschläge anschauen, dann beantworten oder ggf. Code ändern. Danke.

@willuhn
Copy link
Owner

willuhn commented Jan 18, 2022

Ehrlichweise würde ich diesen PR ungemerged schliessen. Die Änderungen bringen jetzt nun nicht den riesen Vorteil, erzeugen aber an zwei potentiellen Stellen Probleme und erzeugt derzeit einen Merge-Konflikt. Das ist doch schade um unser aller Zeit.

@ruderphilipp ruderphilipp force-pushed the refactoring/redundant_local_variable branch from e39e737 to 6e4e0f2 Compare January 23, 2022 09:13
Entfernt doppelt erzeugte Referenzen auf ein identisches Objekt.
@ruderphilipp ruderphilipp force-pushed the refactoring/redundant_local_variable branch from 6e4e0f2 to 8013389 Compare January 23, 2022 09:16
@meigelb
Copy link
Contributor

meigelb commented Jan 26, 2022

Ehrlichweise würde ich diesen PR ungemerged schliessen. Die Änderungen bringen jetzt nun nicht den riesen Vorteil, erzeugen aber an zwei potentiellen Stellen Probleme und erzeugt derzeit einen Merge-Konflikt. Das ist doch schade um unser aller Zeit.

Änderungen passen aus meiner Sicht. (Insgesamt stimme ich aber zu, dass die Änderungen kaum etwas bewirken werden, zumal die meisten Stellen bisher bestimmt sogar vom Compiler rausoptimiert wurden.)
Merge-Konflikt ist nur ein Datei-Konflikt, innerhalb der Dateien aber konfliktfrei.

Copy link
Contributor

@meigelb meigelb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passt von meiner Seite.

@ruderphilipp ruderphilipp marked this pull request as ready for review January 26, 2022 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants