Kevin Powell - Community•6mo ago

Does this make sense to be a subclass?

I'm trying to create a subclass to implement some custom functionality to a parent class. I'm making a discord bot that has a class Discord. This Discord class has a constructor that takes a token. I've created a subclass Bot that has a constructor that calls the parent function. I've done this so I can implement environment variables.

class Bot extends Discord
{

    ...

    public function __construct()
    {
        $dotenv = Dotenv::createImmutable(dirname(__DIR__, 1));
        $dotenv->load();
        $dotenv->required('UMPIRE_TOKEN')->notEmpty();

        parent::__construct([
            'token' => $_ENV['UMPIRE_TOKEN'],
            'intents' => Intents::getDefaultIntents(),
        ]);
    }

    ...

}

class Bot extends Discord
{

    ...

    public function __construct()
    {
        $dotenv = Dotenv::createImmutable(dirname(__DIR__, 1));
        $dotenv->load();
        $dotenv->required('UMPIRE_TOKEN')->notEmpty();

        parent::__construct([
            'token' => $_ENV['UMPIRE_TOKEN'],
            'intents' => Intents::getDefaultIntents(),
        ]);
    }

    ...

}

Everything runs fine but for some reason, this just looks really off to me. I'm not sure if I'm overcomplicating it or creating an antipattern.

26 Replies

ἔρως•6mo ago

honestly, this looks a bit overcomplicated you're still going to have discord-specific code on the bot class the bot makes sense if it is an abstract class that has something generic that can be used for bots that are for other services, like teams or slack or twitch or something else for example, if you need to send a message from the bot, you can have something like this:

abstract class Bot {
  ...
  abstract public function sendMessage(string $channelID, string $message);
  ...
}

class DiscordBot extends Bot {
  ...
  public function sendMessage(string $channelID, string $message) {
    ...
  }
  ...
}

abstract class Bot {
  ...
  abstract public function sendMessage(string $channelID, string $message);
  ...
}

class DiscordBot extends Bot {
  ...
  public function sendMessage(string $channelID, string $message) {
    ...
  }
  ...
}

so, this forces the bot to have specific methods implemented, which can be used on any other bot now, imagine your bot does some processing, like checking the user you can do something like this:

abstract class Bot {
  ...
  abstract public function sendMessage(string $channelID, string $message);
  
  abstract public function userExists(string $email);
  abstract public function userExistsByID(string $id);
  
  protected function userExistsInDB(string $email) {
    ...
  }
  ...
}

class DiscordBot extends Bot {
  ...
  public function sendMessage(string $channelID, string $message) {
    ...
  }

  public function userExists(string $email) {
    // pretend there's some validation of the email
    if(!parent::emailValid($email)) {
      return null;
    }
    
    return parent::userExistsInDB($email);
  }
  
  public function userExistsByID(string $id) {
    $user = $this->getUser();
    $email = $user->email;
    
    return self::userExists($email);
  }
  ...
}

abstract class Bot {
  ...
  abstract public function sendMessage(string $channelID, string $message);
  
  abstract public function userExists(string $email);
  abstract public function userExistsByID(string $id);
  
  protected function userExistsInDB(string $email) {
    ...
  }
  ...
}

class DiscordBot extends Bot {
  ...
  public function sendMessage(string $channelID, string $message) {
    ...
  }

  public function userExists(string $email) {
    // pretend there's some validation of the email
    if(!parent::emailValid($email)) {
      return null;
    }
    
    return parent::userExistsInDB($email);
  }
  
  public function userExistsByID(string $id) {
    $user = $this->getUser();
    $email = $user->email;
    
    return self::userExists($email);
  }
  ...
}

now, if you decide to implement a bot for something else, you can just do the same and extend from Bot, keep the same contract, guarantee that the same functions exist everywhere and have all the generic code in a generic place that's not repeated for example, doesn't make sense to have discord-specific code in Bot, and doesn't make sense to have database access in DiscordBot

Hashi•6mo ago

That’s pretty great answer but guessing from his previous question - “Discord” class is external/dependency project class, and you are meant to use it by calling its constructor with those 2 params. Thus my guess is his goal is to take out the part of the code where he fetches the needed param(s) from the env file to a “function”/constructor to avoid repetition in case he needs the discord class instance in more than one place in the code🙂

ἔρως•6mo ago

makes sense, but for different bots you will need different configurations which can't be made in a generic sense what you can do is create an abstract function in Bot that demands you to implement the fetching of the settings yourself and, obviously, you can share the same instance of DotEnv between both classes in this case, this:

abstract class Bot {
  public $dotenv = null;
  
  public function __construct() {
    $dotenv = Dotenv::createImmutable(...);
    $dotenv->load();
    
    $this->dotend = $dotenv;
  }
  ...
}

class DiscordBot extends Bot {
  public function __construct() {
    $this->dotend->required('UMPIRE_TOKEN')->notEmpty();
    
    $this->doSomething([
        'token' => $_ENV['UMPIRE_TOKEN'],
        'intents' => Intents::getDefaultIntents(),
    ]);
  }
  ...
}

abstract class Bot {
  public $dotenv = null;
  
  public function __construct() {
    $dotenv = Dotenv::createImmutable(...);
    $dotenv->load();
    
    $this->dotend = $dotenv;
  }
  ...
}

class DiscordBot extends Bot {
  public function __construct() {
    $this->dotend->required('UMPIRE_TOKEN')->notEmpty();
    
    $this->doSomething([
        'token' => $_ENV['UMPIRE_TOKEN'],
        'intents' => Intents::getDefaultIntents(),
    ]);
  }
  ...
}

vinceOP•6mo ago

Sorry guys I haven't gotten a full chance to read everything but I wanted to say I originally wanted to make this a static class. And instead of Bot I should probably rename this to Umpire (my discord bot name). I don't really plan this to be reusable but instead an instance of my custom class that I can call static methods on and initialize with the env vars

ἔρως•6mo ago

this is how i would implement the basic steps if you're not going to make it generic, doesn't make sense to separate both

vinceOP•6mo ago

My thinking is I want to abstract functions from the main file Instead of having a messy main file I have 1 class that handles most of the methods that I can call statically in the main file

ἔρως•6mo ago

yeah, and the best way to implement it is how i explained it this is because each class does it's own thing what you had before, both classes do parts of the same thing both have discord specific code and handling if you decide to make it a slack bot later on, then you will have to do what i did anyways unless you're okay with repeating code

vinceOP•6mo ago

Got it, reading everything now

Hashi•6mo ago

From what I understand what you want I would call it “DiscordBotBuilder”/ “DiscordUmpireBuilder” static class with static functions to tidy your main function that will use them With no goal of decoupling and enabling further extension

vinceOP•6mo ago

Yes, exactly that! I just wanted a nice way to call an object that contains methods I need (that would be extended from Discord) and a tidy way to initialize the Discord class (or some instance of it) with my env vars 🙂 There is no way I will ever use this for anything else in the future and if there comes a time then, I'll be better equipped to rewrite it anyway since I'll know more about oop / php

ἔρως•6mo ago

well, you can do that with my method

vinceOP•6mo ago

Ye not saying I can't, I'm reading abstract classes now That's how little I know about oop lol

ἔρως•6mo ago

but i would make the generic Bot a singleton or create a bot factory

vinceOP•6mo ago

I was thinking that too but I felt that might be a bit overkill but idk anything

ἔρως•6mo ago

a singleton is what you described a class you initialize just once, and use it multiple times and you share the same instance everywhere, with static method or not but the idea is that there's ever just 1 instance, at most

Hashi•6mo ago

The problem with that is he has to write his abstract run() and abstract on() and decoupling those will produce some lines of code.

vinceOP•6mo ago

See I was thinking that I don't even need it to be initialized in an object -- I'd be happy if I was just calling it from the class whch is why I wanted it to be static but I was getting errors trying to do that

Hashi•6mo ago

Cuz he probably needs the on() and run() from Discord class

vinceOP•6mo ago

oh okay I understand this now but yea not sure if this is what im looking for could be wrong tho I was writing this whole thing out but realized that yea, definitely can't make this static Or can I 🤔 I think I definitely need a singleton now that I'm writing it out and thinking about it more Because the Discord class uses a constructor, so I can't get the instance of the Discord class on the static Umpire class

Hashi•6mo ago

If you want umpire to be singleton you can’t make it static, but usually you hide the constructor by making it private if the language allows and you define static function like “getInstance()” that makes sure it creates it’s instance only once and then saves it in its fields and every following call to it just returns that same instance

vinceOP•6mo ago

Okay, I think I got it. Here's what I have:

// UmpireSingleton.php

<?php

namespace Umpire;

use Dotenv\Dotenv;
use Discord\Discord;
use Discord\Parts\Channel\Message;

class UmpireSingleton extends Discord
{
    private static UmpireSingleton $instance;

    private function __construct()
    {
        $dotenv = Dotenv::createImmutable(dirname(__DIR__, 1));
        $dotenv->load();
        $dotenv->required(['UMPIRE_TOKEN'])->notEmpty();

        parent::__construct([
            'token' => $_ENV['UMPIRE_TOKEN'],
        ]);
    }

    public static function getInstance(): UmpireSingleton
    {
        if (!isset(self::$instance)) {
            self::$instance = new self();
        }

        return self::$instance;
    }

    public static function startTopicThread(Message $message, array $topic): void
    {
        ...
    }
}

// UmpireSingleton.php

<?php

namespace Umpire;

use Dotenv\Dotenv;
use Discord\Discord;
use Discord\Parts\Channel\Message;

class UmpireSingleton extends Discord
{
    private static UmpireSingleton $instance;

    private function __construct()
    {
        $dotenv = Dotenv::createImmutable(dirname(__DIR__, 1));
        $dotenv->load();
        $dotenv->required(['UMPIRE_TOKEN'])->notEmpty();

        parent::__construct([
            'token' => $_ENV['UMPIRE_TOKEN'],
        ]);
    }

    public static function getInstance(): UmpireSingleton
    {
        if (!isset(self::$instance)) {
            self::$instance = new self();
        }

        return self::$instance;
    }

    public static function startTopicThread(Message $message, array $topic): void
    {
        ...
    }
}

And then:

// main.php

<?php

include __DIR__ . '/vendor/autoload.php';

use Umpire\UmpireSingleton;

$umpire = UmpireSingleton::getInstance();

...

// main.php

<?php

include __DIR__ . '/vendor/autoload.php';

use Umpire\UmpireSingleton;

$umpire = UmpireSingleton::getInstance();

...

Looks / functions better?

ἔρως•6mo ago

you don't have to call it a singleton that's not exactly descriptive that's an implementation detail i would call it UmpireBot it's php - more lines of code have little to no impact if it was javascript, i would be a lot more worried

vinceOP•6mo ago

I ended up rewriting it to just be a regular class like Epic showed I still called it something specific Umpire rather than Bot Basically just a wrapper class that instantiates the Discord class and has some utility functions to it It's not a subclass but it's own thing -- I didn't want to override the parent props / methods

ἔρως•6mo ago

sounds like a sensible intermediary solution

vinceOP•6mo ago

great i was hoping so 😂 i kept thinking about how to set it up and finally landed on this. i was thinking about it in bed last night too lol i dont think it has to be a perfect solution im gonna make a lot of mistakes at this stage anyway i just want something thats not total shite and does what i need it to do

ἔρως•6mo ago

for a prototype, that's all you need

Gaming

Programming

Does this make sense to be a subclass?