8

I have a large class (1500 lines, but will soon be several times that) that I'd like to split so that it fits better with SRP (and so that each file is smaller and more manageable.)

The class contains 50-100 properties, and has several different types of actions that are performed against it - one of which would be an update which in turn does several steps such as update the database, and send emails.

So I think I want 4 classes.

How should I structure the classes?


Here's a simplified version of what I have now:

class Foo {
    public function __construct ($params) {}

    public function update () {
        $this->updateDatabase();
        $this->sendEmails();
    }

    private function updateDatabase () {}
    private function sendEmails () {}
}

$foo = new Foo($params);
$foo->update();

updateDatabase() and sendEmails () each call many other methods - hundreds of lines of code each, and they have several sibling methods doing other tasks.

A basic rewrite to use static methods

class Foo {
    public function __construct ($params) {}
}

class FooUpdate {
    public static function update ($fooParam) {
        FooUpdateDatabase::main($fooParam);
        FooSendEmails::main($fooParam);
    }
}

class FooUpdateDatabase {
    public static function main ($fooParam) {}
}

class FooSendEmails {
    public static function main ($fooParam) {}
}

$foo = new Foo($params);
FooUpdate::update($foo);

A basic rewrite to use instantiated objects

class Foo {
    public function __construct () {}
}

class FooUpdate {
    private $foo;
    public function __construct ($fooParam) {
        $this->foo = $fooParam;
    }
    public function main () {
        $fooTemp = FooUpdateDatabase($this->fooParam);
        $fooTemp->main();
        $fooTemp = FooSendEmails($this->fooParam);
        $fooTemp->main();
    }
}

class FooUpdateDatabase {
    private $foo;
    public function __construct ($fooParam) {
        $this->foo = $fooParam;
    }
    public function main () {}
}

class FooSendEmails {
    private $foo;
    public function __construct ($fooParam) {
        $this->foo = $fooParam;
    }
    public function main () {}
}

$foo = new Foo($bar, ...);
$fooTemp = new FooUpdate($foo);
$fooTemp->update();

Or should I use inheritance or traits somehow?

5
  • 2
    You would get the most (appropriate) feedback at Code Review I'm thinking. Commented May 18, 2016 at 16:45
  • 1
    @Marcus Please note that Foo/Bar identifiers would make it an inappropriate Code Review question. Commented May 18, 2016 at 16:47
  • @200_success Good point. With that said, I see this question as being highly opinion-based. Commented May 18, 2016 at 16:48
  • 1
    @Redzarf - I would advise against creating such restrictive classes such as FooSendEmails{}, or even FooUpdateDatabase{} Consider creating an Email class which can house the various methods to perform your varying actions. This would be a good opportunity for you to implement polymorphism using a Factory class to determine, say, what kind of email to send (E.g. HTML, plain-text, attachments, etc), then create the additional email type classes which implement a common interface. And what about FooInsertDatabase? FooSelectDatabase? Create separate classes to perform those actions? Commented May 18, 2016 at 17:00
  • @Marcus - yes, I have separate classes that handle the sending of emails and database actions. So FooSendEmails() does things like build display versions of data to be inserted into the email template, and work out who the email should be sent to - actions that are unique to Foo. Commented May 18, 2016 at 17:13

2 Answers 2

3

As @SparK says,

  • Your object (Foo)
  • A class that handles database communication (FooRepository)
  • A class that sends mails (Mailer)
  • A class that wraps it all (FooManager)

    $foo = new Foo($params);
    $fooManager = new FooManager(FooRepository, Mailer);
    $fooManager->update($foo);
    $fooManager->notify($foo); //this could be inside the update or an event.
    

This way you can also decompose your classes (ie: separate a class that handles database connections and inject it in the FooRepository, etc). But I don't think having classes that represent an action is the way to go?

Classes are objects that can do actions (among other things), not actions (this is just a comment because of the names you used in your example :p).

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

5 Comments

Thanks, I think the Manager class is the concept I need to read more about. Can you explain your second line of code? Or is it shorthand for $fooRepository = new FooRepository(); $mailer = new Mailer(); $fooManager = new FooManager($fooRepository, $mailer);
Is there any reason to create an instance of FooManager instead of it just having static methods (seeing as $foo is going to be passed in as a param anyway).
@Redzarf Yes just a shorthand. But you could also pass interfaces instead of those classes (ie: MailerInterface) in the constructor definition of FooManager. When creating it you will then need to pass a Mailer object that implements that interface. That is useful if your app is growing and other people is going to work on it. --- If FooManager has static methods how will you pass the FooRepository and the Mailer. You can create them in the static functions but I don't see a reason for that to be, maybe I'm wrong. Also, if another object needs to use a FooManager, you can just inject it.
@Blitu, you could call the FooManager a façade then. I guess it wouldn't have the notify method, as it would coordinate the notification inside the manager (update method, et.al.) code (if mailer is not null).
@SparK Yes, and also implement it as a Singleton. Most important thing is to reduce complexity and let each element be concerned about its role and not others.
2

I guess sending e-mails is one thing, representing your data is another and read-write operations to database would be a third.

So class Foo, class FooPersistence, class FooMailer.
And whoever calls the FooPersistence::update($foo) should also call FooMailer::sendUpdateNotification($foo).

As a side note:
If you had something like Events set up I would trigger the "update event" inside the persistence class and add a listener to it which sends e-mails.

1 Comment

@Redzarf I'll just leave it here as a comment because it's not really part of the answer nor the main topic of the question but a complement of the side note: github.com/SparK-Cruz/elpho/blob/master/samples/events/…

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.