Třída PriceCalculator – potřebuji feedback

Tomáš Jacík

Už delší dobu používám ve svých projektech jednoduchou třídu, která mi zajistí spočítání ceny včetně DPH a slevy. Nic složitého, ale už jsem to kopíroval mockrát, tak jsem se rozhodl udělat z toho balík.

Co do objektového návrhu nemám tolik zkušeností jak jiní tady, tak bych rád nějaký feedback, co mám zlepšit, změnit atd. Zatím prosím pomiňme doc commenty.

https://github.com/…e-calculator

Budu rád za každý tip.

enumag

Pokud to používáš v Nette projektech tak se nabízí možnost přidat extension které ti zaregistruje jako službu.

U třídy PriceCalculatorResult mi nedává smysl implementace IteratorAggregate a proč si drží referenci na PriceCalculatorInterface, ale to mi asi jen uniká využití.

Tyhle require jsou zbytečný, composer ti to autoloadne.

Dále doporučuji dát zdrojáky přímo do složky src a použít PSR-4.

Editoval enumag (23. 7. 2015 15:02)

Felix

Prijde mi zvlastni, ze nekde phpDoc je a nekde ne. Hodnoty calculateFrom by mely byt asi konstanty.

Ty tovarny mi prijdou trochu zbytecne, resp. je jich nejak moc. Jestli to ale pouzivas nezavisle na Nette, tak pak mozna najdou vyuziti.

Pavel Janda

Zvážil bych použití private proměnných. Klidně bych třídu použil, ale chtěl rozšířit (třeba kvůli více druhům slev, zaokrouhlení, cojávím).

Tomáš Jacík

@enumag Díky za tipy. PSR-4 už je doděláno a require jsem odstranil (předtím mi to špatně načítalo, tak jsem je tam musel dát).

Ten IteratorAggregate tam mám proto, abych z toho mohl jednoduše získat array pomocí iterator_to_array, ale asi je to blbost a lepší by byla metoda toArray. Co myslíš?

Instanci na PriceCalculator tam mám proto, že v některých aplikacích ten result skládám na jednom místě (a zobrazuji) a pak někde později je potřeba přidat i slevu. Přišlo mi zbytečné dělat novou instanci PriceCalculatoru a většinu hodnot tam kopírovat, tak si vždy získám calculator, doplním slevu a přepočítám. Napadá Tě lepší cesta?

Tu extension asi také udělám, zatím jsem si vystačil s factory.

@Felix Dodělal jsem tam ty konstanty. Díky za tip. phpDoc ještě určitě doplním, jak bude ustálené API po Vašich tipech. Factory je tam jen jedna, jen je k ní interface. Factory je tam proto, že ji registruju do Nette DI. Ten interface je tam možná zbytečný, nevím. V presenteru jsem si dával inject raději na interface než implementaci.

@PavelJanda Máš pravdu, tohle určitě předělám. Také třída PriceCalculatorResult je jako final, to mám asi také dát pryč? Jaké věci by jsi tam chtěl implementovat? Třeba by se mi něco také líbilo a doplním to.

enumag

@TomášJacík

No v tom getIterator stejně vytváříš pole které pak obalíš iterátorem, takže rozhodně radši toArray. :-)

Aha, ano takhle ta reference na PriceCalculator dává smysl.

Interface pro každou třídu je symfony-style. Není to špatně, jen to většinou nemá využití. Osobně interfacy přidávám když mám pocit že by jiná implementace mohla dávat smysl, což tady úplně neplatí. Spíše bys chtěl nějakou funkčnost přidat a pak už by ti ten interface jako typehint stejně nestačil. Tzn. v tomto případě bych interfacy asi odstranil.

Tomáš Jacík

@enumag Vyhodil jsem tedy IteratorAgregate a dodělal metodu toArray.

Interface PriceCalculatorFactoryInterface jsem vyhodil také. Ten druhý tam nechám kvůli konstruktoru PriceCalculatorResult. Přijde mi, že když si rozšíříš třídu PriceCalculator a nepotřebuješ rozšiřovat i result, může se to takto hodit.

@PavelJanda Upravil jsem ty private a final.

Tomáš Jacík

Extension pro Nette také hotova.