2
\$\begingroup\$

I've always struggled to create and sustainable, organizated, clean code. I tried to use a Factory method and it's working better now that I created another class.

I know I should write documentation, but that's not the advice I'm looking for yet. Because I don't think documentation will help with organizating my code.

This code is scraping multiple twitch IRC chats and using my Laravel API to count the subscribers of multiple streamers. The code works but I have the feeling it's getting harder to implement features, change the code and even read it.

Is there any design pattern should I use for this? How can I make it better?

Request:

class Request
{
    private $requestUrl;
    private $requestFields;
    private $requestResult;
    private $requestStreamer;
    private $requestStatus;
    private $requestType;
    private $ch;

    public function __construct($streamer = null, $status = null, $url = null, $customFields = null)
    {
        $this->requestStreamer = $streamer;
        $this->requestStatus = $status;
        $this->requestFields = $customFields;
        $this->requestUrl = $url;
        $this->start();
    }
    public function setFields($fields = null)
    {
        if ($fields != null)
            $this->requestFields = $fields;

    }
    public function start()
    {
        if ($this->requestUrl == 'online') {
            $this->requestUrl = 'http://localhost:8000/api/streamers/changeOnline';
            $this->setFields(['streamer' => $this->requestStreamer, 'is_online' => $this->requestStatus]);
            $this->request();
            return;
        }
        if ($this->requestUrl == 'status') {
            $this->requestUrl = 'http://localhost:8000/api/streamers/changeStatus';
            $this->setFields(['streamer' => $this->requestStreamer, 'run' => $this->requestStatus]);
            $this->request();
            return;
        }
        if ($this->requestUrl == 'sub') {
            $this->requestUrl = 'http://localhost:8000/api/create/sub';
            $this->setFields();
            $this->request();
            dump($this->requestFields);
            return;
        }
        if ($this->requestUrl == 'checkTwitchOnline') {
            $this->requestUrl =  'https://api.twitch.tv/helix/streams/?user_login=' . $this->requestStreamer;
            $this->requestType = 'get';
            $this->setFields(array('Authorization: Bearer gokyy7wxa9apriyjr2evaccv6h71qn', 'Client-ID: gosbl0lt05vzj18la6v11lexhvpwlb'));
            $this->request();

            return $this->decode();
        }
        if ($this->requestUrl == 'getStreamers') {
            $this->requestUrl =  'http://localhost:8000/api/streamers/getAll';
            $this->requestType = 'get';
            $this->request();
            return $this->decode();
        }
        if ($this->requestUrl == null) {
            $this->requestUrl = 'http://localhost:8000/api/streamers/changeStatus';
            $this->setFields(['streamer' => $this->requestStreamer, 'run' => $this->requestStatus]);
            $this->request();

            $this->requestUrl = 'http://localhost:8000/api/streamers/changeOnline';
            $this->setFields(['streamer' => $this->requestStreamer, 'is_online' => $this->requestStatus]);
            $this->request();

            return;
        }
        return;
    }

    public function setRequestType()
    {
        curl_setopt($this->ch, CURLOPT_URL, $this->requestUrl);

        if ($this->requestType == 'get') {
            curl_setopt($this->ch, CURLOPT_HTTPGET, true);
            $this->setHeader();
        } else {
            curl_setopt($this->ch, CURLOPT_POST, true);
            $this->setHeader();
        }
        curl_setopt($this->ch, CURLOPT_RETURNTRANSFER, true);
    }
    public function setHeader()
    {
        if (!empty($this->requestFields)) {
            $fields_string = http_build_query($this->requestFields);

            if ($this->requestType == 'get') {
                curl_setopt($this->ch, CURLOPT_HTTPHEADER, $this->requestFields);
            } else {
                curl_setopt($this->ch, CURLOPT_POSTFIELDS, $fields_string);
            }
        }
    }
    public function request()
    {
        $this->ch = curl_init();
        $this->setRequestType();
        $this->requestResult = curl_exec($this->ch);

        return $this->result();
    }
    public function decode()
    {
        return json_decode($this->requestResult, true);
    }
    public function result()
    {
        //dump($this->requestResult);
        return $this->requestResult;
    }
}
class RequestFactory
{
    public static function create($streamer = null, $status = null, $url = null, $customFields = null)
    {
        return new Request($streamer, $status, $url, $customFields);
    }
}
$changeStatus = RequestFactory::create(null, null, 'getStreamers');

Main file:


use GhostZero\Tmi\Client;
use GhostZero\Tmi\ClientOptions;
use GhostZero\Tmi\Events\Twitczh\SubEvent;
use GhostZero\Tmi\Events\Twitch\AnonSubGiftEvent;
use GhostZero\Tmi\Events\Twitch\AnonSubMysteryGiftEvent;
use GhostZero\Tmi\Events\Twitch\ResubEvent;
use GhostZero\Tmi\Events\Twitch\SubGiftEvent;
use GhostZero\Tmi\Events\Twitch\SubMysteryGiftEvent;

include('requestFactory.php');
$streamers = RequestFactory::create(null, null, 'getStreamers');
$streamers = $streamers->decode();

for ($i = 0; $i <= count($streamers) - 1; ++$i) {
    $pid = pcntl_fork();
    if ($pid == -1) {
        die('could not fork');
    } else if ($pid) {
        // we are the parent
        // pcntl_wait($status); //Protect against Zombie children
    } else {
        $GLOBALS['streamer'] = ($streamers[$i]['streamer']);
        cli_set_process_title($GLOBALS['streamer'] . 'run.php');

        $request = RequestFactory::create($GLOBALS['streamer'], '1', 'status');
        $request = RequestFactory::create($GLOBALS['streamer'], null, 'checkTwitchOnline');
        $data = $request->decode();
        
        if (empty($data['data'])) {
            $request = RequestFactory::create($GLOBALS['streamer'], '0');
            die();
        }

        $request = RequestFactory::create($GLOBALS['streamer'], '1', 'online');

        $client = new Client(new ClientOptions([
            'options' => ['debug' => false],
            'connection' => [
                'secure' => true,
                'reconnect' => true,
                'rejoin' => true,
            ],
            'channels' => [$GLOBALS['streamer']]
        ]));

        /**
         * @param SubGiftEvent $event
         */
        function giftedRequest($event, $type): void
        {

            $fields = ['recipient' => $event->recipient, 'plan' => $event->plan->plan, 'type' => $type, 'gifter' => $event->user, 'streamer' => $GLOBALS['streamer']];
            RequestFactory::create($GLOBALS['streamer'], null,'sub',$fields);
        }

        /**
         * @param SubEvent $event
         */
        function subbedRequest($event, $type): void
        {
            $fields = ['recipient' => $event->user, 'plan' => $event->plan->plan, 'type' => $type, 'gifter' => NULL, 'streamer' => $GLOBALS['streamer']];
            RequestFactory::create($GLOBALS['streamer'], null,'sub',$fields);
        }

        $client->on(SubEvent::class, function (SubEvent $event) {
            subbedRequest($event, 'SubEvent');
        });
        $client->on(AnonSubGiftEvent::class, function (AnonSubGiftEvent $event) {
            print_r($event);
        });

        $client->on(AnonSubMysteryGiftEvent::class, function (AnonSubMysteryGiftEvent $event) {
            print_r($event);
        });
        $client->on(ResubEvent::class, function (ResubEvent $event) {
            subbedRequest($event, 'ResubEvent');
        });
        $client->on(SubGiftEvent::class, function (SubGiftEvent $event) {
            giftedRequest($event, 'SubGiftEvent');
        });
        $client->on(SubMysteryGiftEvent::class, function (SubMysteryGiftEvent $event) {
            subbedRequest($event, 'SubMysteryGiftEvent');
        });
        $client->connect();
    }
}

This is my first question here, let me know how can I make it better.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How to Ask for examples, and revise the title accordingly. \$\endgroup\$ – Mast Apr 24 at 20:14
  • \$\begingroup\$ If you want more tips on how to write a great question, please start with Simon's guide to posting questions. \$\endgroup\$ – Mast Apr 24 at 20:16
  • \$\begingroup\$ @Mast Thanks for your time. I'm really struggling to find a good question. Because, what I really struggle is organizating the code. Although I changed the title, but I don't know if it's better. \$\endgroup\$ – hamon Apr 24 at 20:28
1
\$\begingroup\$

Title idea: How to refactor simple API Client code in PHP CLI

Here are a couple of tips:

  1. I would add if's and switch'es as soon as possible. For example, you are setting fields and headers of Request::class inside it, just before the execution of cURL request. Why not at Request::class' creation time? Refactor your factory to add correct parameters at Request instantiation. Parameters could as well be mapped like this: public static array $requestMapper = ['online' (something how you determine which request should be created) => [ 'type' => 'GET', 'uri' => '/changeOnline', ... (parameters) ], ...]' Then you can do something like: IF isset($mapper[$something]) THEN (new Request())->setUri($mapper[$something]['uri']). At this point you should have a Request class ready to go and just execute it. Focus on isolating and extracting/abstracting elements you repeat.

  2. Your Request class should either hold the data or do the work. Right now, it executes curl calls and holds the data about the request. This is known as single-responsibility principle (SRP). I would keep the data, and move the execution part to a CustomClient::class for example. There you would also have the instantiation of Guzzle Client so that part from the main file should also be removed. Request should only contain the info needed to do it, and another class should do the actual call using Request::class as data transfer object - which will contain info about URI, headers, etc.

  3. I would move event handlers to another class or classes.

  4. Any hardcoded strings that are not part of the code should be moved to meaningfully named constants.

public const REQUEST_ONLINE = 'online'; 
...
public const EVENT_RESUB = 'ResubEvent';
...
  1. Try to avoid elses as much as possible, they just give you more stuff to do, can be pain in the ass to test, but that can be case specific and not necessarily true for you. For example:
$pid = pcntl_fork();
if ($pid == -1) {
   die('could not fork');
}

$GLOBALS['streamer'] = ($streamers[$i]['streamer']);
cli_set_process_title($GLOBALS['streamer'] . 'run.php');
$request = RequestFactory::create($GLOBALS['streamer'], '1', 'status');

Notice no "else". You may notice this one more impactful when you start writing Unit tests

  1. Use common solutions to common problems

Instead of: for ($i = 0; $i <= count($streamers) - 1; ++$i) {

You could use for ($i = 0, $streamerIndex = 1; $i < count($streamers); $i++, $streamerIndex++) {

It's a personal preference but when you use common patterns like these, it's much easier for other developers to go through that part without spending much time wrapping a head around a raw code. Now this part also has a little trick, assigning a $streamerIndex there, might cause people to overlook that part i.e. seeing ...$i < count($streamers); $i++) I could stop reading right there and miss the part where we assign $streamerIndex, so you might think about that too. :)

Or to completely remove confusion

for ($i = 0; $i < count($streamers); $i++) {
$streamerIndex = $i + 1;
...
  1. I would advise against using superglobals ($GLOBAL) in a way that they are changed. It may lead to unexpected issues, adds to tech debt, ugly, etc.
\$\endgroup\$
5
  • 1
    \$\begingroup\$ Can you explain the benefit of $streamerIndex? I don't follow. Why do you only increment it on the first iteration? I think I am failing to follow the code. \$\endgroup\$ – mickmackusa Apr 25 at 1:09
  • \$\begingroup\$ My bad, it was for example purposes. $streamerIndex should be incremented as well. Point of extracting it to a variable is to prevent having to do something like $data[$i +1] later in code. \$\endgroup\$ – Domagoj Apr 25 at 1:21
  • 1
    \$\begingroup\$ ...that's what I thought, but I wanted to check. Please edit when you have time. \$\endgroup\$ – mickmackusa Apr 25 at 1:21
  • \$\begingroup\$ So is there any problem in setting the curl request type in the Request? What about returning the result? I'm very confused because it's hard for me when to apply SRP It looks like this right now: github.com/komen205/twitch-scraper/blob/main/Scraper/… I'm trying to move everything to the Factory @Domagoj \$\endgroup\$ – hamon Apr 25 at 13:37
  • \$\begingroup\$ I wonder if setting my setHeader and setRequestType should be inside the Factory? \$\endgroup\$ – hamon Apr 25 at 14:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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