Code Review: ArmbreakerEntity.php

[NaGeL182]

👏Code👏Review👏

Welcome to the first Code Review of this piece of a shit repo. I'm gonna go through the codes here and comment on them, in the hopes that I learn something new, or I can show you something new. or just waste my time having fun snarking at someone else code!

So let's look at the very first file in the list and luckily something that other files depend on!

ArmbreakerEntity.php

the snarking reviewing will be on the file according to the 10bf5a99decfb5e7828349b9db6217dee5660e97 commit. Let's start!

/*
 * The MIT License
 *
 * ...

The usual several pages long licenses padding the file, increasing their size for no reason! Love em! It's not that you need to scroll to the code to view it! we are all interested in the whole body of the license!

...yeah if you didn't notice I don't like this style. in my opinion, they are annoying and don't add anything meaningful. Just add a LICENSE.md to the repo: GitHubHelp: Adding a license to a repository or something like chromium did here:

// Copyright (c) 2012 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

and just link to the license file! anyway moving on!

namespace Armbreaker;
/**
 * [DESTINATION] [AGREEMENT] [TRAJECTORY] [AGREEMENT]
 *
 * @author sylae and skyyrunner
 */
class ArmbreakerEntity
{

YAY! Now we are doing science with Golden space whales! Tho DocBlock should explain what this class is about but we got jack diddle! very useful! \o/ but yeah I know it's a joke, but still...

Also class... I see you there... I ain't forgetting you... I just will come back latter..I'm watching you...

    /**
     * Stores an AWS SQS client. Unused right now :v
     * @var \Aws\Sqs\SqsClient
     */
    protected $aws;
    /**
     * Reference to the db object for convenience
     * @var \Doctrine\DBAL\Connection
     */
    protected $db;
    /**
     * Hey you like sending web requests yeah?
     * @var \Slim\App
     */
    protected $slim;

Hey there sexy... can I get your number? No? okay :(

I like this. docBlock used correctly annotating the type the variable will hold, helping your IDE out and by extension yourself with autocompletion! And even the description even a bit jokey convey enough information to say what the variable holds or do.

👍

Wait. Why is there the Slim variable? will all the classes that inherit this serving web requests? then sure it has its place here, if not then it just straight up useless here and a waste of memory.

    const ENTITYTYPE_MASTER  = 0;
    const ENTITYTYPE_SCRAPER = 1;
    const QUEUE_INTERNAL     = 0;
    const QUEUE_SQS          = 1;

const! I like consts! they never change! You can count on them being always the same. Except when they don't.

Also TYPE? why TYPE? If I understand this correctly ( no Docblock yay! \o/ ) you want to check later I the child of this either a MASTER or SCRAPPER. Correct? no? well I assume it is now :V

This is kinda redundant. Why? Because if you do inherit and extend this you will most likely name the next class will include ether master or scrapper then why not check the class itself? It's not like you cant know the class name, right?

but there is a better way! Ladies and gentleman lets clap for Interfaces!
👏👏👏👏👏👏👏👏👏

Why are Interfaces better? Let me explain: When you check a variable's type you check it if it really is what you ASSUME it is, so you can use the function on it mostly for scalar types or use its method if its an object.

If you check an object you want to make sure it has the methods you want to use so your code doesn't die when the user clicks on that god damn button you tell them don't press because it releases the sharks from the tank if its bugs out. BOBBY FOR THE THOUSAND TIME DONT TOUCH THAT BUTTON!

AHEM

So usually you expect to have the function you use so you check the object's class...but what if that isn't the class you wanted? Maybe the Dependency Injection gave you a different class! if you use instanceof and it inherited from that class than you are okay...but what if it isn't? what if its a different implementation? ok, your code doesn't run then or gives back an error, then you debug then you find out that object isn't what you want yet it does have that method you want, then what gives?

That's why you implement interfaces and check against that? Interfaces define the function and only that! They don't give them a body so on their own they are useless!

Useless you say? then why use it?

Well dear, Bobby because whoever implemented said interface you can be sure to have that method! Interfaces require you to define the same function as they did AND implement them. AKA give them a body. if the interface used type hint they need to use type hint, if they used return type they need to use that return type. There is no compromise only MOTHER RUSSIA the Interface.

Any Class can implement an Interface any number of it and if you check against that you can be sure that they have it! So use that instead of consts! it even allows to have multiple types and if you need to have a specific class check that class!

You could say you could use Traits here as well, but traits have different uses, so it's not really recommended here, but if you want to look into it this function could help.

Moving on...oh wait, the QUEUE... hmm honestly the same can be said about them...you just checking the queue type.

    /**
     * Constructor. Just does some basic setup :v
     */
    public function __construct()
    {
        $this->sqs  = null; // new \Aws\Sqs\SqsClient(ConfigFactory::get()['sqs']);
        $this->db   = DatabaseFactory::get();
        $this->slim = new \Slim\App();
    }
}

Hellooooo constructor! You are setting up something I see! But why are you so distant? So emotionally guarded? so Hardcoded? Why can't you accept some Interfaces into your heart? You(Me) would so much happier!

Why do you ask? because then I could give you what database interface to use! Or my own \Doctrine\DBAL\Connection Object! Hell, I could even mock that and UnitTest you! would it be wonderful? Like so:

    /**
     * Constructor. Just does some basic setup :v
     */
    public function __construct($sqs, $db, $slim)
    {
        $this->sqs  = $sqs;
        $this->db   = $db;
        $this->slim = $slim;
    }

Hah! You're so dumb now I can give whatever I want to that and it will break! Now what?!

Yes, you are correct Bobby boi. We can improve on that!

    /**
     * Constructor. Just does some basic setup :v
     */
    public function __construct(\Aws\Sqs\SqsClient $sqs, \Doctrine\DBAL\Connection $db, \Slim\App $slim)
    {
        $this->sqs  = $sqs;
        $this->db   = $db;
        $this->slim = $slim;
    }

Now only those objects can get passed...well that's a bit limiting, isn't it? well if you don't have interfaces then you need to use this, but if you have Interface I recommend using those!

Still! we can further optimize this! What if we don't have all the objects when I initialize this? well, then I'm fucked ain't I? Well yes. Unless...

    /**
     * Constructor. Just does some basic setup :v
     */
    public function __construct(\Aws\Sqs\SqsClient $sqs = null, \Doctrine\DBAL\Connection $db = null, \Slim\App $slim = null)
    {
        $this->sqs  = $sqs ?? null;
        $this->db   = $db ?? null;
        $this->slim = $slim ?? null;
    }

Great! So now I can give only one or two of the object and give later that third?

That's right Bobby! I see you are learning! Of course, for this to work you need to have a setter set! like so:

    public function setSlim(\Slim\App $slim)
    {
        $this->slim = $slim;
    }

and voila you are done! Your code Is Dependency Injection Ready! Isn't that great!? your class turned from an introvert into an extrovert! What a big softie! ❤️

So with that, I do...wait ain't I forgetting something? hmm...

hey boss. I found this thing in the corner crying.

Oh right, thank you, Bobby boy. The Class! I told you I come back to you! you look like to me you won't ever be initiated on your own because let's look at your body... its just parameters and setters at best. If anybody initializes you, they cant use you.. you are useless... USELESS I TEL YOU!

Unless...you are something that other build on...like an abstract class! you see abstract class prevents themselves to be initialized, so if somebody is idiot enough to do so will get a BIG FAT ERROR! Also, it tells other developers that they should not initialize, but I think the error drives the point in better.. ¯\_(ツ)_/¯

So yeah you should be an abstract class!

AND NOW I am done! WOO! ... How the hell did I write so much about 69 lined file? ¯\_(ツ)_/¯

Oh well. I at least hope you enjoyed reading it as I enjoyed writing and researching some things for this!

Until next time!

Oh right, Feel free to reply, agree, disagree, argue about my points! the goal is to learn from each other! and we cant if we don't talk :V

[sylae]

Hi! sorry for mucho delay here, so...

I like the shorter license header thing, that seems like a good idea :)

I think we talked about the consts elsewhere, but those are going to get moved to a config object once that's more than a glorified wrapper around an array.

I've never used interfaces seriously, but I do agree it's probably a better way to go about this. Along with the abstract class thingy. Not sure if there's a way to combine those?

Same thing on not allowing the slim/aws/db stuff to be passed as arguments.

(will close this once i've implimented these adjustments :))

[NaGeL182]

I've never used interfaces seriously, but I do agree it's probably a better way to go about this. Along with the abstract class thingy. Not sure if there's a way to combine those?

of course there is a way. Abstract Classes and Classes use the extends keyword and only one class can be extended. Interfaces use the implements key word and you can implement several of them!

Same thing on not allowing the slim/aws/db stuff to be passed as arguments.

I don't get this? same thing on not allowing? not allowing like what?

[sylae]

i don't get that either, but basically: i like that idea, passing them as args. i think i had a stroke whilst writing that or something :v