28
rant1ng
2y

As someone in charge of reviewing this code, how would you react to this function that contains, what could be, the longest ever readable return statement.

Comments
  • 12
    and he forget a param in the comments, that's a strike
  • 8
    If you use correct indentation and formatting then I would like it.
  • 2
    Is the same query being executed twice or every time it finds an order it needs to check or am I just unfamiliar with laravel orm? The inner transform seems overly complicated
  • 4
    anyway, it's wrong, returns true instead of a modified object, which I think was the intent, because he was using transform

    I personally like it, yeah its a big long string of code, but its readable

    its formatted for no distraction mode in sublime...

    Although the correct version isn't technically really a one liner anymore
  • 5
    It grabs all orders from the DB, applying a transform using a closure to add highlight = true parameter to the object. Vue uses this to highlight the row.

    I personally think its pretty cool, i mean, you can literally almost read it in english.
  • 1
    It's still PHP!
  • 1
    @rant1ng I think it is unreadable. All the steps should be clear in a first view. In this case, you have to check in detail the long string to start making conclusions. Refactor asap.
  • 1
    For PHP this is quite readable. I'd refactor the highlighthing or fetching the orders out to a separate method though, you know, something about single responsibility. But I've seen much less complex PHP code that was way harder to read.
  • 1
    I just literally lol’d and my wife is just staring at me
  • 0
    It’s fine - I’m not fond of static function but I’ve seen far worse. Maybe be a bit more specific in the docblock
  • 2
    Okay.

    ....

    $affliateUser = Auth::User();
    $affliateOrders = Order::where('affiliate_id', $affliateUser->id);
    if (!$orders) {
    // What now? Empty collection?
    }

    $createdBeg = new Carbon(...);
    $createdEnd = new Carbon(...);

    $ordersHighlighted = $affliateOrders
    ->whereBetween('created_at', [$createdBeg, $createdEnd])->get();

    if (!$ordersHighlighted) {
    // What now? Empty collection?
    }

    return $affliateOrders->transform(
    function(??? $order) use (??? $ordersHighlighted) {
    $order->Highlight = in_array(
    $order,
    $ordersHighlighted,
    true
    );
    }
    );
  • 0
    Ew.

    I hate ligatures
  • 2
    Sorry for bad formatting.

    Point is:

    Don't execute statements multiple times.
    Document object classes, use interface / object class in function.
    Validate object sanity.
    Pass objects instead of arrays / scalar values.
    Assure that the return of the object is what you expect - be concise.
    Checking via
    is_array($bla) && !empty($bla)
    makes it very obvious that the returned value is an array.

    Don't initialize objects multiple times.
    Objects are passed via reference and should always be preferred to scalar values / arrays.

    Don't chain 'blindly' calls.

    If the API changes or you need to debug - you are fucked.

    ...

    I know it seems readable and tempting.

    But it's really a mess and clearly non optimal.

    Variables are our friends - and object documentation is our hot lover, hmkay?
  • 2
    I'm not sure about laravel(?) Orm but, if I'm reading it correctly it's executing the inner query once for every item and for every item it fetches a bunch and checks with an in array.

    I guess there would be room for optimization by a) moving the inner function out and/or b) using select for the inner query, to see if there is something matching instead of fetching a collection and traverse it.

    Oh and params/return type on collections. Not sure about laravel but AFAIK for plain doctrine if you fetch a collection of the vlass `Foo` the convention is

    /**
    * @return Foo[]
    */
    public function getFoo: array
    {
    ...
    }

    Note the `Foo[]` part isn't php, but it's understood by IDEs as, there is an either empty array being returned or an array containing (only) `Foo` objects
    (Often anything itteratatble is accepted)
  • 3
    Don't execute statements multiple times.
    Edit: especially Database Statements !!!
    Performance aside, race conditions might occur on heavy write tables - you'll get different results in each loop as new data is added!!!
  • 1
    Functions are used for a reason. And it’s not that one.
  • 0
    Burn it in hell fire, I still have nightmares of my python one-liners, now imagine [c#?]
  • 1
    Premium code lol

    ++ for you using fira code tho, good font
    ( -- for solarized )
  • 0
    @GodlikeBlock not my code. I agree fira.

    Theme is cobalt though, I like it. What do you use?
  • 1
    Does it work, if so then it's good.
  • 2
    @rant1ng i made my "own" colorscheme for VSCode - a port from the vim theme "srcery"; feel free to try it :3

    I also like the color schemes:
    Seth, spirited away, xhi
Add Comment