Ok, given that you're after some CR on your understanding (and usage) of OO techniques, I'll go through your code almost line by line. If my criticisms strike you as blunt, please keep in mind that my goal is to help, not to offend.
Coding standards
This has become a bit of a hang-up of mine, but I can't stress enough how important it is to write code that conforms to some form of standard. PHP, of course, does not (yet) have an official standard as such, but the unofficial PHP-FIG standard is widely accepted, and all major players (Zend, Symfony, CakePHP, ... the list is on the site) subscribe to this standard. As should you.
This entails, among other things:
- property names don't start with an underscore to indicate visibility (private, protected). That's what we did in the PHP4 days. I think none of us is willing to go back to that era.
- Opening braces for methods, functions and classes go on a new line
- Code is indented by spaces, 4 of them, tabs are not to be used. If you use nothing but tabs, then that's an acceptable deviation from the standard, but be consistent. SVC's like Git will give you grief otherwise.
- access modifiers aren't optional, they are required.
- Method names are camelCased, variables/properties are either camelCased or you can use underscores. Either way: be consistent. I prefer camelCased properties.
Type-hinting all the way
I see you use the occasional type-hint. That's great! Fantastic even, but you're not using them as much as you can. You can't hint for primitive types, but you can use doc-blocks, casts and you can hint for array variables.
More on this later.
Your code, at a glance
Rather than listing my recommendations and linking to the wiki of the SOLID principles, let's get stuck in and review your actual code. I'll already change a thing or 2 to make your code more readable (ie conforming to the PHP-FIG standards):
class SchoolSoap
{
/**
* @var array
*/
protected $config = array();
Stop here (yes, already). I've removed the underscore, but I wonder: why are you initializing your property to an empty array, if you have a constructor that will always re-assign that property? Just initialize the properties to null if you don't need them to have a specific default value.
I've also added a doc-block that makes it clear to everyone what this property is expected to be, what you'll assign it to, and how it'll be used. This will help your IDE provide you with useful auto-completion info, and error reporting.
/**
* @var \SoapClient
*/
public $soap = null;
/**
* @var string|null
*/
public $error = null;
//add your own doc-blocks from now on :)
public $params = null;
public $result = null;
Now, the methods:
/**
* Some info about the construct, will show in auto-complete
* with any decent IDE
* @param array $params = array()
* @return $this
*/
public function __construct(array $params = array())
{
$this->config = $params;
try
{//this is non-standard, but I use allman indentation
//that's my guilty pleasure
$this->soap = new SoapClient(
$this->config['url'],
array(
'trace' => 1,
'exception' => 1
)
);
}
catch (Exception $e)
{
$this->error = $e->getMessage();
}
}
So, I've cleaned up a bit, but there are still quite a few issues left. For starters: you allow for the user to pass an array $params to the constructor. If he fails to do so, then that's fine, but the array defaults to an empty array. That, too, isn't a problem, but what is problematic is that you create the instance of SoapClient using $this->config as though it is an associative array and as though it has certain keys set!. You don't do any isset checks (meaning your code might emit notices).
Then, if the SoapClient could not be constructed, you catch any Exception that might have been thrown. But SoapClient doesn't throw Exception instances, it throws a specific type of exceptions: SoapFault.
Catching any higher (as in parent of a particular exception) type of exception is known as pokemon exception handling. It reads as if the code was written by somebody who didn't really know what to expect.
Not that this really matters, because your entire class revolves around this SoapClient instance being set. If it fails, then your class shouldn't be allowed to exist. It's likely that the code creating the instance contains a bug, and provided faulty data. That kind of thing should be communicated immediately: the error shouldn't be caught by you, because you can't fix it: the error should be thrown back to the user! That's Why I'd suggest you write this as a constructor:
/**
* Simple wrapper constructor
* @param string $wsdl
* @param array $options = null
* @return $this
* @throw SoapFault
*/
public function __construct($wsdl, array $options = null)
{//make options null by default, but force wsdl to be passed explicitly
$this->config = $options
$this->soap = new SoapClient(
$wsdl,
$this->config
);
}
We end up with less code, but again: with a decent IDE, you'll get more information about what this constructor does when you type new SchoolSoap(.
You no longer need the $this->error property, and don't have to hope that the user will check that property's value: if an error occurred, he'll get an exception thrown at him, which is as it should be!
The other methods should be easy to improve, along the same lines:
/**
* Perform a soapCall to $function, passing $parameters
* If no parameters are provided, $this->params will be used
* @param string $function
* @param array $parameters = null
* @return bool
* @throw SoapFault
*/
public function actionSoap($function, array $parameters = null)
{
if ($parameters === null)
$parameters = $this->params;
//assign to property directly, don't use temp var... no need
$this->result = $this->soap->__soapCall(
$function,
array(
'parameters' => $parameters
)
);
return true;//wouldn't return $this->result; be more usefull?
}
The setter methods are a great idea. I especially like their being fluent (chainable), nothing but formatting needs to change there:
public function setParam($params)
{
$this->params = $params;
return $this;
}
A wrapper is best when it exposes its object
What you have written here is, essentially, a wrapper object: it wraps an instance of SoapClient, to offer a different (possibly neater, safer and clearer) API.
However, I need a reason to use a wrapper, it should allow me to do what I want. This wrapper severely restricts my usage of the SoapClient class. What's to stop me from simply writing this:
$schoolSoap = new SoapClient($wsdl, $options);
$result = $schoolSoap->__soapCall($function, $params);
A wrapper should somehow expose some of the internals of the wrapped object, but perhaps check the arguments that the user is trying to pass, or at least offer constants that the user can use to facilitate the manipulation of the wrapped object. The blindest, slowest, but easiest way to do so is by implementing a magic __call method in your SchoolSoap class:
public function __call($method, array $args)
{
if (method_exists($this->soap, $method))
{//perhaps add an is_callable check here, too
$return = call_user_func_array(
array($this->soap, $method),
$args
);
if ($return === $this->soap)
return $this;//don't return the wrapped object
return $return;
}
throw new \BadMethodCallException(
sprintf(
"Method '%s' does not exist in classes %s and %s",
$method,
__CLASS__,
get_class($this->soap)
)
);
}
Of cours, a safer way is to implement certain methods of the wrapped objects in your wrapper, check the arguments and then call the wrapped object's method:
public function getLastResponse($fromObject = false)
{
if ($fromObject === false)
return $this->result;//as set in the `actionSoap` method
return $this->soap->__getLastResponse();//from wrapped object
}
This is a clean, easy to maintain and safe way to expose your wrapped object, and it doesn't give me, as a user, a reason not to use your class.
Of course, if you do this, then you don't need that slow magic __call method anymore, and you can simply do away with that (as well you should).
An in-between solution is to have a private property, holding an array with methods that can be called by the user, and use those in your __call method. That saves you some method_exists calls.
Ok, I'll leave you with this final recommendation concerning the first of your classes:
Passing arrays to constructors is easy, and shortens the number of arguments your methods take. I can see why one would do this. However, if you are going to write OO code, then it can be safer/better still to pass data objects containing that same information. You can type-hint for those specific objects, and be sure that the properties you need will exist.
I could post an example here, but instead I'd invite you to check this github fork of mine, and in particular the Config class. The Config is what, logically, configures the behaviour of all classes that make API calls. By hinting for an instance of that object in the constructor, I can rest assured that things like the url, usage mode, user and API-KEY will be set, and ready to be used. I use getters and setters throughout the code, to ensure my data is correctly formatted and sanitized, too.
there are some magic getters and setters, and I thing also a magic __call method somewhere, but I'm still working on the code, to get rid of them.
If you compare my fork to the code I started off with, I think it's pretty clear that passing data objects is a big step forwards.
There are other things in your code that I will discuss further (like your use of the dreaded static keyword), so I will be adding to this answer from time to time. For now, though, I think I've given you sufficient food for thought.
Update
As promised: Back for some more reviewing-stuff. I'll start off with that one helper class you've included, and take it from there. I already mention the static keyword isn't to my liking. Why isn't it? Well, static properties/variables are variables that retain their state. That means that, if a function has a static variable, and you assign a value to that var, that value is kept in memory in between function calls.
Class properties work similarly, but that value is shared between all instances of that class. You can also access that property at without an instance, as though it were a global variable. And that's the problem with statics: they are, essentially globals in OO disguise, and we all know globals are bad.
Same goes for your method: it works like a function, and is globally accessible, like a global function. Only, static method calls are slower than function calls! Then why bother with a class, and not simply create a function?
That would, indeed be better than what you have now, but it's not in tune with your wanting to write more OO code
So let's make it object oriented, and make good use of the advantages of OOP: you can use properties to store certain things, and like, for example your SoapClient instance. At the moment, you create a new SoapClient instance every time you instantiate SchoolSoap. Wouldn't it be nice if we only had to do that once? Your static method now creates a new SchoolSoap instance every time it is called, without being passed an existing instance already, because its functionality depends on that SchoolSoap being there. What's more: it's function in your program is a specific use-case of the capabilities of SchoolSoap. So its job is the same (performing a SOAP call), only its a specific type of call.
In OO code, it wouldn't be uncommon to see StudentHelper being a child of SchoolSoap. Therefore, I suggest you go down that route, and use inheritance:
class StudentHelper extends SchoolSoap
{
public function getStudentList($params)
{
$result = $this->setParam($params)
->actionSoap('studentList');
if (!$result)
return $result;
return $result->studentList;//or $this->_result->studentList;
}
}
All the calls you have, that deal with students go in this class, and for each group of calls, you can create a new child class of SchoolSoap.
The usage is very simple:
$student = new StudentHelper($config);//what you pass to SchoolSoap's constructor
$studentList = $student->getStudentList($params);
$student->addStudent($params);//if that call exists
All of the goodies that SchoolSoap (methods and properties) are also properties of StudentHelper now (except for private properties), same goes for methods.
Of course, now the name StudentHelper is a bit weird, because it isn't a helper anymore: it's a full-blown wrapper, too, so I'd change the name of SchoolSoap to BaseSoap, because it's your base class, and change StudentHelper to StudentSoap or something, to better reflect what the class is.
I've posted a review of a singleton class on this site a while back. In it, I go into more details on why statics are to be avoided, feel free to check it out:
Singleton design pattern
