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

Standardkonformität #6

Open
N3XT0R opened this issue Apr 4, 2015 · 3 comments
Open

Standardkonformität #6

N3XT0R opened this issue Apr 4, 2015 · 3 comments

Comments

@N3XT0R
Copy link

N3XT0R commented Apr 4, 2015

Moin,
im Grunde genommen finde ich die Library super. Jedoch habe ich einige Punkte zu bemängeln.
Letztlich einige grundlegende Punkte aus der Entwicklung. Einerseits sollten die jeweiligen Klassen in einzelnen Dateien gegliedert werden, nicht in einer.
Der andere Punkt ist die Methodendefinition, diese sollte wie es eigentlich immer der Standard ist im englischen verfasst werden.

Es mag nun recht pinkelig klingen, aber es ist nicht so, dass ein Team welches diese Library nutzt aus deutschsprachigen Mitgliedern besteht und dementsprechend evtl. mit dem Sourcecode rein von der Sprache her nicht klar kommt. Andererseits eignet sich die Library im Grunde genommen gut für eine Integration in andere Systeme, allerdings auch nur dann, wenn die jeweiligen Klassen tatsächlich logisch getrennt werden, gerade wenn das Autoloading der Dateien anhand des Namspaces erfolgt, wie z.B. beim Zend Framework der Standardautoloader mit dem Dateipfad arbeitet.

@b-water
Copy link

b-water commented Apr 4, 2015

Stimme dir da soweit zu. Problem wird aber sein das die von dir angesprochenen Änderungen dazu führen das alle Ihren Code anpassen müssen. Fraglich ob dann wer updatet. Für zukünftige Implementierungen wäre das aber nicht verkehrt!

@N3XT0R
Copy link
Author

N3XT0R commented Apr 5, 2015

Den derzeitigen Sourcecode zu explementieren, bzw. ein komplettes rewriting durchzuführen müsste nicht zwangsläufig der Fall sein. Es können die entsprechenden Methoden ja schließlich auch erst einmal bereit gestellt werden, und anschließend die alten Methoden als deprecated gekennzeichnet werden, wie normalerweise eben auch in der Realität vorgegangen wird, dann haben andere Entwickler auch die Chance parallel zu der aufsteigenden Minor-Version der Library auch die Änderungen über einen definierten Zeitraum anzupassen, bei einer Version 2 (breaking change) könnten die alten Methoden explementiert werden, wenn dahingehend dem Schema der Semantic Versioning nachgegangen wird. Dahingehend sehe ich das persönlich eigentlich unproblematisch.

Anderer Punkt der mir auch noch auffällt ist, die Währung kann nur in der SepaXmlCreator Klasse hinterlegt werden, und ist dann für den kompletten Stapel an Lastschrifteinzügen gültig, evtl. wäre es sinnvoll die Währung auszulagern in die Buchungsklasse.

edit:

Dahingehend habe ich einen Pull-Request an das Team von diesem Repository gestelllt, mit allen Änderungen zu dieser Library. Eine Abwärtskompatibilität besteht.

Davon abgesehen mangelt es in der Methode getGeneratedXml() vollständig an Qualität, die Qualität des Sourcecodes entspricht keinem gelernten / studierten Entwickler, in keinster Form.

Die Library sollte nun den oop-standards in größten teilen entsprechen. Die Methode zur Generierung des XMLs, wurde von mir nur der logischen Folge angehend angepasst, sowie der englischen Sprache. Meiner Meinung nach ist sie noch nicht sauber genug formuliert und insbesondere zu lang, für eine Methode entsprechend der gängigen OOP Standards. Nichts desto trotz, sollte der Pull Request entgegen genommen werden.

Eine Fortführung der Library nehme ich ich in Kauf in meinem eigenen Repository.

@tschiffler
Copy link
Owner

Hi N3XT0R,

danke für Deinen Pull-Request, ich werde mir den die Tage ansehen. Es ist korrekt dass die Implementierung sehr "Dreckig" erfolgte, diese diente einem schnellen Durchstich als Test.

Solltest Du weitere Pull-Requests senden wollen so sind diese natürlich willkommen

Gruß Thomas

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

No branches or pull requests

3 participants