4

I am just about to write a method to convert some billing data into an invoice.

So say i have and an array of objects that contain the data necessary to create the invocie items.

While in the billing controller Which of the following way is correct

$invoice = new Invoice();
$invoice->createInvoiceFromBilling($billingItems);

Then in the Invoice Class

Public Function createInvoiceFromBilling($billingItems)
{
    $this->data = $billingItems;

OR

Invoice::createInvoiceFromBilling($billingItems)

Then in the Invoice Class

Public Function createInvoiceFromBilling($billingItems)
{
    $invoice = new Invoice();
    $invoice->data = $billingItems;

Which way is the correct way?

Regards

Improve this question
9
  • 1
    What you actually need is a factory what create the invoice: $invoice = $invoceFactory->createFromBilling( $billingItems ); Commented Nov 28, 2013 at 10:24
  • Default the 1st, and pass $billingItem through constructor should be better, to present dependence relationship of Billing and Invoice. But if you have many way to create Invoice (not only from billing), it should consider the 2nd, something like Factory in design pattern. Commented Nov 28, 2013 at 10:28
  • I didnt know anything about Factory, just read up on it and it seems to be a good idea, so in a sense you would use a factory to run it similar to the first example Commented Nov 28, 2013 at 10:31
  • @Fwolf, My invoice class has many ways of creating invoices, plus it is also the model class for my Yii framework, so a constructor would be an issue. So if I am using the class to create invoices from various different sources then Use the factory method, and still return an instance of Invoice Commented Nov 28, 2013 at 10:33
  • 2
    Who the hell is voting to close this post as "opinion based"?!? I would understand if you could find a duplicate (because there sure is one), but there is nothing opinion based about this, if you adhere to SOLID principles. Commented Nov 28, 2013 at 10:39

2 Answers 2

Reset to default
2

As tereško pointed out in the comments section above, you should look into using the Factory pattern. A good (and simple) real-world-based example from the linked source:

<?php
class Automobile
{
    private $vehicle_make;
    private $vehicle_model;

    public function __construct($make, $model)
    {
        $this->vehicle_make = $make;
        $this->vehicle_model = $model;
    }

    public function get_make_and_model()
    {
        return $this->vehicle_make . ' ' . $this->vehicle_model;
    }
}

class AutomobileFactory
{
    public function create($make, $model)
    {
        return new Automobile($make, $model);
    }
}

// have the factory create the Automobile object
$automobileFactory = new AutomobileFactory();
$veyron = $automobileFactory->create('Bugatti', 'Veyron');

print_r($veyron->get_make_and_model()); // outputs "Bugatti Veyron"

As you can see, it is AutomobileFactory that actually creates instances of Automobile.

Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

-1: pointless use of statics. This essentially will cause a tight coupling to a class name whenever you need to create new instance.
@tereško Copied and pasted it without realising. In fairness, the site is usually good.
Actually it isn't. It is more like an advertisement site for FIG members =/ .. then again, it's not like we are spoiled for choice. Anyway: downvote revoked.
@tereško My biggest gripe is that it could do with better organization / categorisation, but more often than not, it does recommend some good practices. I suppose your point about not being spoiled for choice stands here :)
-1

The method written first is better because in the second your code will generate object for invoice each time it is being called.

Improve this answer

5 Comments

-1: please learn at least the basics of OOP. The question was not about "when object gets created", but about "how to crate invoice from billing data".
any reason for dislikes ?
please open your eyes it is not a singleton he is creating object of Invoice(); each time this function get called, improve your reading skills
I guess you (and whoever upvoted your ill-informed comment) are not aware that "factory method" (which you are proponent of) is an anti-pattern, and causes severe ramifications to the architecture. Static factory method both acts as a black hole of complexity as it attracts more and more external logic, and creates tight coupling the the name of the class.
It seems you don't have strong opps you just see a class and starts voting for it, even a factory pattern can't resole it, and point is his solution is not containing factory or singleton factory, which I pointed out. Brother don't underestimate any one.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.