Skip to content

Fix cloning#148

Merged
SKoschnicke merged 3 commits intosoftware-challenge:masterfrom
xeruf:master
May 17, 2018
Merged

Fix cloning#148
SKoschnicke merged 3 commits intosoftware-challenge:masterfrom
xeruf:master

Conversation

@xeruf
Copy link
Copy Markdown
Member

@xeruf xeruf commented Feb 26, 2018

CloneNotSupportedException sollte nicht an den Client weitergeleitet werden, da sie nicht eintreten kann und einfach nur nervt. Außerdem habe ich das lästige GameState warning "Muss aussetzen" auf debug geschoben, wäre sogar dafür es ganz zu entfernen.

@SKoschnicke
Copy link
Copy Markdown
Contributor

Wir sollten darueber nachdenken, clone durch copy constructors zu ersetzen, siehe https://dzone.com/articles/java-cloning-copy-constructor-vs-cloning und https://dzone.com/articles/java-cloning-even-copy-constructors-are-not-suffic

Was denkt ihr dazu, @soerendomroes und @Poehli ?

@soerendomroes
Copy link
Copy Markdown
Member

Ja das halte ich für sinnvoll, es wird ja auch nicht mehr super.clone() von uns aufgerufen

@KahlerTak
Copy link
Copy Markdown
Contributor

Wir sollten Clone ersetzten, aber dann auch wie in den Links beschrieben. Das klingt durchaus sinnvoll. Ich befürchte, der reine copy constructor könnte zu schwer nachzuvollziehenden Fehlern führen.

@SKoschnicke
Copy link
Copy Markdown
Contributor

was meinst du mit "dann auch wie in den Links beschrieben". Ich wuerde cloning constructor + defensive copy method implementieren und die jetzigen clone methoden (mit den Verbesserunge von Xerus) als deprecated markieren (wir koennen nicht mitten in der Saison die API aendern).

@KahlerTak
Copy link
Copy Markdown
Contributor

Ja genau das meinte ich und hatte den Namen defensive copy method nicht parat.

@xeruf
Copy link
Copy Markdown
Member Author

xeruf commented Apr 23, 2018

wie sieht's aus? Ich hab mal noch nen zweiten Commit gemacht der schon so ziemlich einen copy constructor implementiert und ein paar Sachen verbessert

@SKoschnicke SKoschnicke self-requested a review May 9, 2018 08:18
@SKoschnicke
Copy link
Copy Markdown
Contributor

koennen wir fuer naechste saison mergen

@xeruf
Copy link
Copy Markdown
Member Author

xeruf commented May 17, 2018

Hm toll nächste Saison bringts mir nicht mehr viel. Es sind doch keine breaking changes dabei?

@SKoschnicke SKoschnicke merged commit e8ddabf into software-challenge:master May 17, 2018
@SKoschnicke
Copy link
Copy Markdown
Contributor

Richtig, ich musste mir das noch genauer ansehen. Sieht alles gut aus, danke fuer die Hilfe!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants