PHP Simple OOP Class

Question

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); } } 

Answer 1

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.

Answer 2

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!

Answer 3

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; } }