0

I have a simple class Notification, which has a few properties (title, body, op) and only getters/setters. Among all my project I'll use different kind of notifications, this is Notification objects but with different title, body, op depending of the logic of that part.

I'm wondering what would be the best aproach to implement it. I've though of using some kind of polymorphism that would work but not sure if that would be overcoding. Here is the class Notification:

<?php

namespace App\Library\Notifications;

/**
 * Class Notification
 */
class Notification
{
    /** @var string $title*/
    private $title;
    /** @var string $body */
    private $body;
    /** @var string $op */
    private $op;

    /**
     * Constructor
     */
    public function __construct()
    {
        $this->title = '';
        $this->body = '';
        $this->op = '';
    }

    public function getTitle(): string
    {
        return $this->title;
    }

    public function getBody(): string
    {
        return $this->body;
    }

    /**
     * String that tells the apps which kind of notification
     * we are sending so they can act depending of the type
     *
     * @return string
     */
    public function getOp(): string
    {
        return $this->op;
    }

}

And here is a subclass:

<?php

namespace App\Library\Notifications;

use Illuminate\Support\Facades\Lang;

/**
 * Class Notification
 */
class OtherAcceptedTripNotification extends Notification
{
    /**
     * Constructor
     */
    public function __construct()
    {
        parent::__construct();

        $this->title = Lang::get('notifications.other_accepted_trip_title');
        $this->body = Lang::get('notifications.other_accepted_trip_body');
        $this->op = 'OA';
    }
}

2 Answers 2

4

This is abusing inheritance.

An inherited class is used when it changes something about the base class (but it retains some of the existing base class' logic).

In your example, nothing is changed except the value of the properties. You're not adding/changing any logical behavior.

This is effectively the same as doing (forgive the C# syntax):

public class Person()
{
    public string Name { get; set; }

    public Person(string name)
    {
        this.Name = name;
    }
}

public class John : Person
{
    public John() : base("John") {}
}

public class Bob : Person
{
    public Bob() : base("Bob") {}
}

public class Tom : Person
{
    public Tom() : base("Tom") {}
}

Or, even more blatantly:

public class MyNumberClass()
{
    public int Value { get; set; }

    public MyNumberClass(int value)
    {
        this.Value = value;
    }
}

public class Five : MyNumberClass
{
    public Five() : base(5) {}
}

public class Twenty : Person
{
    public Twenty() : base(20) {}
}

This is not what inheritance is used for! You should be using simple variables here:

var myAge = new MyNumberClass(20);

var myBoss = new Person("Bob");

var otherAcceptedTripNotification = new Notification ("title", "body", "op");

The factory pattern.

However, I do understand your intention here. You're trying to create a default preset, so you don't have to use hardcoded values. If you use the same OtherAcceptedTripNotification type across your codebase, you don't want to have to repeat yourself.

That is an actual problem that is worth solving. The factory pattern is a good solution here.

The linked reference (and other online resources) go into a lot of detail, and in my opinion a lot of complexity. To boil it down to the most barebones example to illustrate the point (again, forgive the C#):

public static class NotificationFactory
{
    public static Notification CreateOtherAcceptedTripNotification(string bodyText)
    {
        return new Notification(
                      "THIS HEADER CANNOT BE CHANGED",
                      "FIXED PREFIX : " + bodyText,
                      "FIXED OA"
                    );
    }
}

This gives you the benefit you want (only needing to define the fixed values of a notification "preset" once) without the drawback (inheritance abuse).

2

In this case you can either use a factory, or have a set of these created and accessed by name. My pseudo-code is not the same language you are using, but you can adapt the pattern.

public class Notification {
    internal Notification(string title, string body, string op) {
        Title = title;
        Body = body;
        Op = op;
    }

    public string Title { get; private set; }
    public string Body { get; private set; }
    public string Op { get; private set; }
}

public static class Notifications {
    public static Notification OtherAcceptedTrip = new Notification(
        Lang.Get("notifications.other_accepted_trip_title"),
        Lang::get("notifications.other_accepted_trip_body"),
        "OA");
}

At this point, you can use that notification any time you want just like this:

SendNotification( Notifications.OtherAcceptedTrip );

Essentially, you are controlling the instances of the notification. If you can't reuse the same instance everywhere because the Notification object is doing other things besides what you have listed here, you would use a factory. The factory will create an instance of the Notification object with the right values that you can use, and it won't be shared with anything else.

7
  • I knew about the factory pattern, didn't instead about your aproach, does it have a name? thanks Commented Jun 28, 2018 at 15:02
  • It's essentially the way Java does Enumerations. I think the name is "Type-Safe Enum Pattern" (infoworld.com/article/3198453/application-development/…) Commented Jun 28, 2018 at 17:04
  • As long as you keep it simple, and keep the enums read-only, it's OK. However, you should be careful, as this article goes in to detail on: codeblog.jonskeet.uk/2014/10/23/… Commented Jun 28, 2018 at 17:06
  • While the core of the answer is correct (not using inheritance), keep in mind that this requires you to be able to deep clone the object if you want more than one object of that particular "type" (OtherAcceptedTrip).
    – Flater
    Commented Jun 29, 2018 at 9:04
  • 1
    @Flater, if that's required, then use a factory. Immutable read-only objects can be safely shared across threads without any locking mechanisms, so your example would have to be a class that does more than what the OP demonstrated. Commented Jun 29, 2018 at 10:42

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.