SitePoint Sponsor

User Tag List

Results 1 to 7 of 7
  1. #1
    SitePoint Zealot
    Join Date
    Oct 2003
    Location
    Denmark
    Posts
    129
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    Tie up dependencies

    I have a Debtor superclass (to orders, invoices and quotations). After reading Martin Fowler's refactoring, it seems as if I got it wrong.

    My classes as they look now - where the DebtorItems know which Debtor they belong to, and I use the getList in DebtorItems to get the items belonging to a debtor.

    PHP Code:
    class Debtor {
        private 
    $id;
        function 
    __construct($id 0) {
            
    $this->id intval($id);
            if (
    $this->id 0) {
                
    $this->load();
            }

        }

        function 
    load() {
            
    $this->db->query("SELECT  * FROM debtor WHERE id = ".$this->id);

            if(!
    $this->db->nextRecord()) {
                return 
    0;
            }
            
    $this->value["id"] = $this->db->f("id");
            
    $this->value["number"] = $this->db->f("number");
            
    $this->value["contact_id"] = $this->db->f("contact_id");
            
    $this->value["payment_method"] = $this->db->f("payment_method");
            
    $this->value["this_date"] = $this->db->f("this_date");
            
    $this->value["due_date"] = $this->db->f("due_date");
            
    $this->value["date_stated"] = $this->db->f("date_stated");

            
    // gets contact
            
    $this->contact = new Contact($this->db->f("contact_id"));

            
    // gets items
            
    $this->loadItem();
            
    $item $this->item->getList();
            
    $this->value['items'] = $item;

            for(
    $i 0$max count($item), $total 0$i<$max$i++) {
                
    $total += $item[$i]["amount"];
            }

            
    $this->value["total"] = round($total2);
            
    $this->value['payment_total'] = 0;

            return 
    1;
        }

    With the following items:

    PHP Code:
    class DebtorItem {
        var 
    $id;
        var 
    $debtor;
        var 
    $db;
        var 
    $product;
        
        function 
    DebtorItem(&$debtor$id) {
        
            
    $this->debtor = &$debtor;
            
    $this->db = new Db_sql;
            
    $this->id = (int)$id;
            
            if(
    $this->id 0) {
                
    $this->load();
            }
        }
        
        function 
    load() {
            
    $this->db->query("SELECT product_id, id, description, quantity FROM debtor_item WHERE id = ".$this->id." AND intranet_id = ".$this->debtor->kernel->intranet->get("id")." LIMIT 1");
            if(
    $this->db->nextRecord()) {
                
    $this->product_id $this->db->f("product_id");
                
                
    $this->value["id"] = $this->db->f("id");
                
    $this->value["product_id"] = $this->db->f("product_id");
                
    $this->value["description"] = $this->db->f("description");
                
    $this->value["quantity"] = $this->db->f("quantity");
        
                
    $this->product = new Product($this->product_id);
            }
        }

        function 
    getList() {
            
    $db = new DB_sql;
            
    $db2 = new DB_sql;

            
    $db->query("SELECT id, product_id, product_detail_id, quantity, description FROM debtor_item WHERE active = 1 AND intranet_id = ".$this->debtor->kernel->intranet->get("id")." AND debtor_id = ".$this->debtor->get("id")." ORDER BY position ASC, id ASC");
            
    $i 0;
            
    $j 0;
            
    $item_no_vat = array();
            
    $item = array();

            
    $product_module $this->debtor->kernel->useModule('product');
            
    $units $product_module->getSetting('unit');

            while(
    $db->nextRecord()) {
                
    // $product = new Product($this->debtor->kernel, $this->db->f("product_id"), $this->db->f("product_detail_id"));
                // Følgende sql-sætning er indsat fordi New Product() tager for lang tid at loade.
                
    $db2->query("SELECT name, number, unit, price, vat, product_id FROM product_detail
                    WHERE intranet_id = "
    .$this->debtor->kernel->intranet->get('id')."
                        AND product_id = "
    .$db->f('product_id')."
                        AND id = "
    .$db->f('product_detail_id'));

                
    $db2->nextRecord() OR trigger_error("Ugyldig produktdetalje i DebtorItem->getList()"E_USER_ERROR);
                if(
    $db2->f("vat") == 0) {
                    
    $item_no_vat[$j]["id"] = $db->f("id");
                    
    $item_no_vat[$j]["name"] = $db2->f("name");
                    
    $item_no_vat[$j]["number"]= $db2->f("number");
                    
    $item_no_vat[$j]["unit"] = $units[$db2->f("unit")];
                    
    $item_no_vat[$j]["price"] = $db2->f("price");
                    
    $item_no_vat[$j]["quantity"] = $db->f("quantity");
                    
    $item_no_vat[$j]["description"] = $db->f("description");
                    
    $item_no_vat[$j]["vat"] = $db2->f("vat");
                    
    $item_no_vat[$j]["product_id"] = $db2->f("product_id");
                    
    $item_no_vat[$j]["amount"] = $db->f("quantity") * $db2->f("price");
                    
    $j++;
                }
                else {
                    
    $item[$i]["id"] = $db->f("id");
                    
    $item[$i]["name"] = $db2->f("name");
                    
    $item[$i]["number"]= $db2->f("number");
                    
    $item[$i]["unit"] = $units[$db2->f("unit")];
                    
    $item[$i]["price"] = $db2->f("price");
                    
    $item[$i]["quantity"] = $db->f("quantity");
                    
    $item[$i]["description"] = $db->f("description");
                    
    $item[$i]["vat"] = $db2->f("vat");
                    
    $item[$i]["product_id"] = $db2->f("product_id");
                    
    $item[$i]["amount"] = $db->f("quantity") * $db2->f("price") * 1.25;

                    
    $i++;
                }

            }
            return(
    array_merge($item$item_no_vat));
        }


    It seems to me, that there a couple of possible refactorings to do. First of all I could inverse the relationship.

    1) I would first Rename Method - DebtorItem::getList() --> DebtorItem::getItems().
    2) Then I would Move Method - DebtorItem::getItems() --> Debtor::getItems().

    These two refactorings would inverse the relationsship, so Debtor knows about it's items.

    My question is how to manage new Items. Now I will pass in the Debtor to the DebtorItems, and then I could always in a save()-method know which Debtor, I was dealing with. Is this approach the right one, or is it utterly broken (as I suspect)?

    And a big thank you - this forum really boosts my understanding. I have been convinced to using tests, and it is great. Not to mention all the things I am learning about real object oriented design. I thought I was programming decently using OOP, but I was nothing but a novice

  2. #2
    SitePoint Addict
    Join Date
    Nov 2005
    Location
    Germany
    Posts
    235
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    My question is how to manage new Items. Now I will pass in the Debtor to the DebtorItems, and then I could always in a save()-method know which Debtor, I was dealing with. Is this approach the right one, or is it utterly broken (as I suspect)?
    If an item is always bound to one Debtor I would probably do $debtor->addItem(...) while this delegates the real creation to one DebtorItem::create(). So debtor is still the responsible class to the outside world, but it doesn't itself need to know how to create an item.
    I didn't read through all you code, so I'm not sure this applies. (And I don't think that DebtorItem::create($debtor, ...) is so "utterly broken".)

    Regards,
    Christoph

  3. #3
    SitePoint Wizard silver trophy kyberfabrikken's Avatar
    Join Date
    Jun 2004
    Location
    Copenhagen, Denmark
    Posts
    6,157
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    In your example, you're combining table-related operations and row-related operations into a single class (The active record pattern). This can work just fine, but I prefer to separate table gateway from row data gateway. That is, one class finds/persists entities, and another class represents each of these entities. Or in other words - one class deals with the table, one deals with a row in this table. Consider this:
    PHP Code:
    class DebtorGateway
    {
        function 
    findById($id) {
            
    $result $this->db->query("select * from debtor where id = :id", Array(':id' => $id));
            if (
    $row $result->fetch(FETCH_ASSOC))
                return new 
    Debtor($row);
            }
        }

        function 
    create() {
            return new 
    Debtor();
        }

        function 
    save($debtor) {
            if (
    $debtor->id) {
                
    $this->update($debtor);
            } else {
                
    $this->insert($debtor);
            }
            return 
    $debtor;
        }
    }

    class 
    Debtor
    {
        function 
    __construct($row = Array()) {
            foreach (
    $row as $key => $value) {
                
    $this->$key $value;
            }
        }

    This still doesn't deal with relationships between entities. There is a relationship between Debtor and DebtorItem, in which one Debtor has 0..* DebtorItem's. If you follow the same approach with DebtorItem as with Debtor, you end up with a DebtorItemGateway + a DebtorItem. In the DebtorItemGateway, you could then place a method selectDebtorItemsByDebtorId. It would fall natural to delegate this method to the Debtor, so that you could call $debtor->getItems(). That'll mean that Debtor needs a reference to DebtorItemGateway:
    PHP Code:
    class Debtor
    {
        protected 
    $debtorItemGateway;
        function 
    __construct($debtorItemGateway$row = Array()) {
            
    $this->debtorItemGateway $debtorItemGateway;
            foreach (
    $row as $key => $value) {
                
    $this->$key $value;
            }
        }
        function 
    getItems() {
            return 
    $this->debtorItemGateway->selectDebtorItemsByDebtorId($this->id);
        }

    At this point you'll run into a number of problems, which basically boils down to object identity. The following should illustrate:
    PHP Code:
    function test_identity() {
        
    $result $debtor->getItems();
        
    $a $result[0];
        
    $result $debtor->getItems();
        
    $b $result[0];
        
    $this->assertIdentical($a$b);

    This test will fail, because the same data is fetched twice into separate objects. You'll have similar problems when trying to manipulate the collection (Adding removing items).

    There are ways to solve this, but it quickly becomes very complex, so I suggest that you either I) accept the limitations, and put the responsibility for dealing with it in the application layer, rather than trying to solve it in the data access layer, or II) use one of the existing ORM libraries for PHP.

    Quote Originally Posted by FrlB View Post
    And I don't think that DebtorItem::create($debtor, ...) is so "utterly broken".
    Yes, DebtorItem::create is a static dependency, but classes are static, so that's not a major problem. new DebtorItem is just as static.
    You could delegate creation to a factory, which would take the static dependency away, but whether that flexibility is needed is an open question.

  4. #4
    Non-Member
    Join Date
    Jan 2003
    Posts
    5,748
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Umm...

    Kyber, whilst I agree with the need for the delgation, I am wondering why you didn't go with this approach instead? Ie

    PHP Code:
    // ...
    function getItemsGatewayInterface $gateway ) {
            return 
    $gateway->selectDebtorItemsByDebtorId($this->id);
        }
    // etc 
    What advantages (or disadvantages) would there be over this alternative? To me, it keeps the dependency out of the class, but there may be a disadvantage to this, that I'm not aware of

  5. #5
    SitePoint Wizard silver trophy kyberfabrikken's Avatar
    Join Date
    Jun 2004
    Location
    Copenhagen, Denmark
    Posts
    6,157
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Dr Livingston View Post
    What advantages (or disadvantages) would there be over this alternative? To me, it keeps the dependency out of the class, but there may be a disadvantage to this, that I'm not aware of
    Obviously the user will need to know that she has to provide a DebtorItemGateway instance to the getItems method. The point of hiding the call to selectDebtorItemsByDebtorId is to hide the implementation details (how the data is mapped to storage) for the user. If you expect the user to know to pass a certain gateway, you loose some level of abstraction.
    Or in other words - It removes the dependency from the class, but does so by pushing the responsibility up to the user land.

    But it's certainly a matter of taste - In many cases, I don't think a full object model works that well with PHP. Since you only have a limited number of records in memory at each request, it appeals more to me to use really dumb row data gateways (providing not much more than an associative array), and then simply have all logic for querying the database in separate objects. With that strategy, the relationships between entities are not mapped at all - it's an exercise left for the user land programmer.

  6. #6
    simple tester McGruff's Avatar
    Join Date
    Sep 2003
    Location
    Glasgow
    Posts
    1,690
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Are you using double-entry book-keeping?

  7. #7
    SitePoint Zealot
    Join Date
    Oct 2003
    Location
    Denmark
    Posts
    129
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Kyber -> Thanks for the great post. I am still digesting.
    McGruff -> Yes, I am also using double-entry book-keeping. But in this example it is only invoices, orders an quotations (I call it debtors).


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •