2
\$\begingroup\$

I am still learning OOP and I need a little help, I don't really know how to properly make this not look so terrible.

Any thoughts?

<?php

class Noti {

    public static $app;
    public static $user;
    public static $url;
    public static $result;
    public static $data;
    public static $title;
    public static $text;

    public static function status($title = null, $text = null) {

        // Error check
        if(empty($title)) return false;
        if(empty($text)) return false;

        // Set
        self::$title = $title;
        self::$text = $text;

        // Get the NOTI config
        self::$app  = Config::get('noti.app_id');
        self::$user = Config::get('noti.user_token');
        self::$url  = Config::get('noti.url');

        // Format
        self::$data = self::format();

        // Url
        $ch = curl_init(self::$url.'add'); // Append add to url for status update
        curl_setopt($ch, CURLOPT_POST, 1);
        curl_setopt($ch, CURLOPT_POSTFIELDS, self::$data);

        self::$result = curl_exec($ch);

        // Close
        curl_close($ch);

        return self::$result;
    }

    private static function format() {
        $data = array(
            'app' => self::$app,
            'user' => self::$user,
            'notification[title]' => self::$title,
            'notification[text]' => self::$text
        );

        return http_build_query($data);
    }

}
\$\endgroup\$
  • 2
    \$\begingroup\$ A couple of words on what the code is supposed to do would be appreciated. Otherwise it's pretty hard to say tell if the code's good or not. It's obviously something to do with notifications, but what's the context? \$\endgroup\$ – Flambino Oct 9 '12 at 18:26
  • \$\begingroup\$ Pretty much I call Noti::status(title,text); and it does a cURL post to a url with that data. It's really not anything major :) \$\endgroup\$ – Steven Oct 9 '12 at 18:29
  • \$\begingroup\$ Ok, but I see you've made a static class (you could put abstract in there too), yet there's also some user_token stuff. So is this for a single-user system or are there several users to notify (in which case a non-static class might make more sense)?. I understand how you call this code and what it does, but I still don't know much about the context \$\endgroup\$ – Flambino Oct 9 '12 at 18:35
  • \$\begingroup\$ I think your thinking about it a bit too much. It's literally a single file and it gets called with Noti::status('test', 'testing'); there is not going to be several users, there is a single user and a single app id, and a single url to notify. I could have made this a function, but the way my framework works is that classes are easier to work with as they are autoloaded. \$\endgroup\$ – Steven Oct 9 '12 at 18:40
  • \$\begingroup\$ Fair enough, but that's already more information than was in your original question - which is why I asked. Without a little bit context, others on this site are basically playing the role of a PHP interpreter, trying to run the code in their heads to figure out what's going on. \$\endgroup\$ – Flambino Oct 9 '12 at 18:45
4
\$\begingroup\$

This is not OOP.

Just using class does not make your code object oriented. Writing OO code is about encapsulating the state and behavior into objects.

See this answer which explains why public properties and static are bad.

Static is Already Hurting You

Your code relies on Config::get. Anyone who wants to reuse this class will need to have a class named Config with a method named get. This is what your class depends on. If you try to unit test this class you will find it difficult to test it as a unit. It is already hard-coded to rely on the Config class.

Dependency Injection / Inversion Of Control

To remove the tight coupling to your Config object you should pass in the dependencies to the constructor of the object:

class Noti
{
    protected $app;
    protected $config;

    public function __construct(Config $config)
    {
        $this->config = $config;
    }

    public function getStatus()
    {
        $this->app = $this->config->get('noti.app_id');
        // etc.
    }
}

Note: I would use docblock comments above to explain all of the methods and properties.

The tight coupling to the Config class's get method has been modified. From above we see that any Config object or derived class (We type-hinted for Config in the constructor) can be passed into the Noti class. This could be further decoupled by Type-Hinting for an interface that specifies the contract for the Config object implementation.

The testing is now simple as a mock object can be passed into the constructor and there are no hard-coded dependencies left in the code.

Method Names

Method names should be descriptive. Consider using verb+noun to describe your methods:

getStatus // instead of status
getQuery  // instead of format

Now we know that we are returning the status, and formatting a URL. Note: As mseancole points out you are doing too much in your status method.

\$\endgroup\$
2
\$\begingroup\$

As Flambino said, why are you using static properties and methods? Static really contradicts the whole purpose of OOP and so it should really only be used in special cases. In fact, those special cases aren't likely to present themselves until you are much more familiar with the concept of OOP, therefore, if you ever find yourself using static elements you can stop yourself immediately and ask, "Why?". More than likely there is a better way to do what you are attempting without them.

That being said, the way you are currently listing your properties is fine, but just in case you didn't already know, you only have to declare public once. You can separate each additional property with a comma. And since PHP doesn't care about whitespace, you can make this more legible by adding a newline after each property. This also works for properties that have default values, as demonstrated below. As Corbin pointed out in the comments, this is a preference, you can do either one.

public
    $app,
    $user = 'default user',
    //etc...
    $text
;

PHP's empty() function is really only useful on arrays, and objects. When using strings it is better to use is_null() or just treat the parameter/variable as a boolean directly. Note: Doing the later will have unexpected results if the parameter's value is ever a false value (0, '', "false", etc...). By the way, PHP inherently requires braces. Otherwise, when you add additional lines to a statement, you wouldn't have to then also add braces. As such, you should really use braces all the time, even on single line statements. People might argue this, but I feel that if PHP does not allow completely braceless code it shouldn't allow it at all, otherwise it is only going to cause issues. Additionally, you can combine those two if statements into one using the OR short circuiting operator. This follows the "Don't Repeat Yourself" (DRY) Principle, which, along with the Single Responsibility Principle, are important concepts when trying to learn OOP.

if( is_null( $title ) || is_null( $text ) ) {
    return FALSE;
}
//OR
if( ! $title || ! $text ) {
    return FALSE;
}

Speaking of Single Responsibility Principle, you are doing too much in your status method. The method should only do as much as is required to accomplish its purpose. In this case return the status. It can call as many methods as it needs to accomplish this, but it shouldn't be directly responsible for doing anything else. This ensures completely reusable code, which, as mentioned above, is a key concept of OOP. However, in this context, I don't believe the status() method should really have to call any other methods.

Last, but not least, try to cut back on internal comments. If you follow the Single Responsibility Principle then your code should naturally fall into a "self-documenting" style, and then the only comments you will need are doccomments for recording more abstract functionality.

Hope this helps!

\$\endgroup\$
  • \$\begingroup\$ +1, however: "in case you didn't already know, you only have to declare public once. You can separate each additional property with a comma." That syntax makes me sad (separate declarations are much more legible in my opinion). As far as I'm aware, very few people ever use that syntax. Also, the is_null and boolean casts of the $title/$text might not do what he wants. For example, an object is considered non-null. And an array is considered non-null... So on. (I suppose it's way out of scope to completely explain the oddities of the type system in PHP :p.) \$\endgroup\$ – Corbin Oct 9 '12 at 22:51
  • \$\begingroup\$ To each their own. I find mine much more readable, but I imagine those with a background in Java, C, etc... would find the list a little more familiar. I did try to stress that the current implementation was fine, just trying to point out another way. As to the other, I was going by context and assuming they were strings. I did mention that arrays were more appropriate for empty(), perhaps I should add that about objects too. \$\endgroup\$ – mseancole Oct 10 '12 at 0:34
  • \$\begingroup\$ I just realized looking back at the code that a solution is for him to not provide default parameters. If he wants the function to return false when called with non-non-empty strings, he shouldn't allow a default invocation of it that defaults to the error input he checks for. That makes no sense. Also, for what it's worth, I would do: if (!is_string($x) || $x === "") { ... }. A white list approach seems more direct than a blacklist. Anyway, I'm done rambling now. Hopefully I haven't been too annoying :). \$\endgroup\$ – Corbin Oct 10 '12 at 2:28
1
\$\begingroup\$

Not sure why the properties are marked public, since they're overwritten each time Noti::status() is called. There's no real point in having external code access Noti::$user, for instance (external code can just call Config::get() if it wants the values). So protected or private would be better.

But actually, there's little point in having those static properties at all. For instacne, the format method could be rewritten as

private static function format($title, $text) {
    return http_build_query(array(
        'app' => Config::get('noti.app_id'),
        'user' => Config::get('noti.user_token'),
        'notification[title]' => $title,
        'notification[text]' => $text
    ));
}

... and it wouldn't need to have access to any class properties at all (I'd probably call it build_query or build_request while I'm at it, since format isn't terribly descriptive).

But of course, you can skip the format method entirely. It's private, and there's only one other method in the class, so there's only one place it'll be called. (If there were two or more places it could be called from, then a separate method would definitely make sense, of course.)

You also don't need to store anything going on in status() in static properties. $data = ... accomplishes the same as self::$data = ... (without having to type self::). Same for self::$result.

In the end, this should do the same thing

abstract class Noti {
    public static function status($title = null, $text = null) {
        if(empty($title) || empty($text)) return false;

        $data = http_build_query(array(
            'app'  => Config::get('noti.app_id'),
            'user' => Config::get('noti.user_token'),
            'notification[title]' => $title,
            'notification[text]'  => $text
        ));

        $url = Config::get('noti.url') . "add";

        $ch = curl_init($url);
        curl_setopt($ch, CURLOPT_POST, 1);
        curl_setopt($ch, CURLOPT_POSTFIELDS, $data);
        $result = curl_exec($ch);
        curl_close($ch);

        return $result;
    }
}
\$\endgroup\$

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.