0

I am converting my procedural php project to OOP. I stuck on one problem. What is the best way of retrieving table data?

Passing the id with constructor 

new Rate(new Currency($currencyId), $amount);

or first setting the id and calling getId() method?

$currency = (new Currency())->setId($currencyId);
new Rate($currency->getId(), $amount)
Namal
  • 2,061
  • 3
  • 18
  • 26
  • Why would `->setId()` return an object? – Ja͢ck Jul 05 '13 at 07:52
  • @Jack all methods returning void should return $this; in php to be able to combine function calls of objects, Zend Framework makes very much and good use of it – Daniel W. Jul 05 '13 at 07:58
  • @DanFromGermany IMO, that's a retarded idea, no offense. – Ja͢ck Jul 05 '13 at 08:00
  • @DanFromGermany , method's name should describe what it does. As you might notice, it was not called `setIdAndGetThisInstance()`. – tereško Jul 05 '13 at 08:03
  • its VERY common and useful in php... what should setId return? Its a void function. Look at Iterator classes: Iterator->next() gives you an object, Iterator->last() gives you last Object,.. http://stackoverflow.com/questions/6138758/return-this-design-pattern-or-anti-pattern – Daniel W. Jul 05 '13 at 08:05
  • Well .. `next()` also is not a **setter**. It is there to return the next value (as the name for the method suggests). And it returns an instance that is not an `Iterator`. – tereško Jul 05 '13 at 08:07
  • @DanFromGermany `Iterator::next()` returns the next element, not the iterator itself. – Ja͢ck Jul 05 '13 at 08:07
  • ok, bad example. I don't want to encourage discussion here, but what about chaining stters: `$person->setId(1)->setName('peter')->setLastSeen(time());` – Daniel W. Jul 05 '13 at 08:11
  • `$person->setId_and_getPerson(1)->setName_and_getPerson('peter')->setLastSeen_and_getPerson(time());` .. fixed that for you – tereško Jul 05 '13 at 08:13

3 Answers3

2

The Rate object shouldn't care about what $currencyId is, rather the only thing it should know is that there's a currency:

$rate = new Rate(new Currency($currencyId), $amount);

If you want to prevent too many Currency objects from being instantiated, you could introduce a mapper:

$mapper = new CurrencyMapper();
$rate = new Rate($mapper->getCurrencyWithId($currencyId), $amount);

It should be noted that when doing this all objects must be immutable, according to the rules of Flyweight.

Ja͢ck
  • 170,779
  • 38
  • 263
  • 309
1

Ask a question on OOP design and you'll get 100 different answers. There are many different design patterns that will accomplish the same thing.

It is hard to say since we dont see your full design, however given your 2 examples, I would go with the first one.

Kris
  • 6,094
  • 2
  • 31
  • 46
  • To agree with Kris' answer, your second example would be the same as `new Rate($currencyId, $amount);` so your first example is way better, making use of dependency injection design pattern. – Daniel W. Jul 05 '13 at 08:02
0

Just from the viewpoint of readability and the number of calls made I'd go with the first one. The second one makes two separate calls (one to the constructor and another one to set $currencyId).

Adi H
  • 668
  • 7
  • 9