patternphpMinor
PHP data structure review and critique
Viewed 0 times
critiquephpstructureanddatareview
Problem
I have been working lately on an educational framework project to learn more about web development and problems that arise in more complex applications. Part of the framework's principles is to be as Object Oriented as possible. For example, most data returned from queries to a data source will be returned as a particular domain object, or a set of domain objects. I could simply return an array with the appropriate objects, but this isn't really good enough for my needs. So, I need a better data structure.
Some key aspects of the data structure to store these objects:
With these things in mind, I created the following interface:
Below are the classes used in my first implementation of the
```
/**
* This is a base list,
Some key aspects of the data structure to store these objects:
- Must only accept objects of a certain type; the type of object allowed to be stored should be passed in
__construct()and should be astringrepresentation of the interface or class name.
- No other parameters should be needed in the
__constructmethod.
- Should be able to be iterated over in a
foreachloop.
- The data structure should have some mechanism to restrict the number of elements that can be added to it.
With these things in mind, I created the following interface:
DataList. Since the data structure is intended to be used within the framework the objects passed must extend CoreObject, which is a very simple abstract class that provides an equals(CoreObject $Object), hashCode() and __toString() methods. This was done to ensure that there is a method to compare the equality of the objects for searching purposes.Below are the classes used in my first implementation of the
DataList interface. The first class is a BaseIteratingList that provides some basic list functionality and implements the Iterator interface to allow looping. The second class is UniqueList that will not add duplicate objects to the list and implements the remaining methods of the interface. The final class is ObjectTypeValidator used to ensure the objects being add() or set() implement or extends the correct parent type.BaseIteratingList:```
/**
* This is a base list,
Solution
Why reinvent the wheel and not build upon the Traversable, ArrayAccess and Serializable SPL interfaces, or more realistically upon one of their concrete children?
On similar requirements I would have possibly build my
I would suggest returning
Since
And as the "hidden" dependency of
Consider all of the above as extreme nitpicking, your code is way past the point of obvious flaws. The real value of the answer, if any, was to point you towards the SPL. If you did consider it and rejected it, would you care to share why?
Edit in response to the edit in response to my answer :)
That being said, my reference for the DataList interface was the Java List interface and is really what I attempted to model my data structure after.
I definitely wouldn't want to base my CoreObject off of ArrayObject...that is far too much stuff for something really intending to only provide some common methods for other classes.
As this is a learning experience, you should be concentrating on learning the language and its quirks. Java <> PHP, in quite a few ways. You shouldn't limit yourself in copying the Java interface, it's a good interface to clone but now that you've done that you should explore enhancing it with native PHP functionality. You should take as much advantage of native stuff as possible, the performance difference is noticeable.
I have to admit that the ArrayObject was an off hand example, my primary intention was to point you towards the SPL. I did base something similar on an ArrayObject, but the requirements weren't exactly the same.
Still bits and pieces of your code could be rewritten to take advantage of native functionality. For example, this:
could possibly be rewritten as:
and avoid the costly call to
I chose -1 as the return value for index() based on 2 reasons.
...
(2) I don't want users attempting to simply test for falsehood on the return value.
Your users should check for falsehood like that:
You shouldn't have to compensate for users that are not familiar with PHP's comparison operators.
On similar requirements I would have possibly build my
CoreObject upon an ArrayObject./**
* Returns the number representing the index for the passed element, or -1 if
* the element could not be found.
*
* @param CoreObject $Object
* @return int
*/
public function indexOf(CoreObject $Object) { ... }I would suggest returning
false instead of -1 when element not found. It feels a little more natural and expected. public function set($index, CoreObject $Object)Since
Part of the framework's principles is to be as Object Oriented as possible., you should consider using interfaces instead of classes, for object dependencies. And as the "hidden" dependency of
ObjectTypeValidator: It highly depends of what the framework is for. You've gone for the common practical approach instead of the academic one (injection). There's no right or wrong here, it highly depends on the framework's purpose and audience, the academic approach may not always be the educative approach. Consider all of the above as extreme nitpicking, your code is way past the point of obvious flaws. The real value of the answer, if any, was to point you towards the SPL. If you did consider it and rejected it, would you care to share why?
Edit in response to the edit in response to my answer :)
That being said, my reference for the DataList interface was the Java List interface and is really what I attempted to model my data structure after.
I definitely wouldn't want to base my CoreObject off of ArrayObject...that is far too much stuff for something really intending to only provide some common methods for other classes.
As this is a learning experience, you should be concentrating on learning the language and its quirks. Java <> PHP, in quite a few ways. You shouldn't limit yourself in copying the Java interface, it's a good interface to clone but now that you've done that you should explore enhancing it with native PHP functionality. You should take as much advantage of native stuff as possible, the performance difference is noticeable.
I have to admit that the ArrayObject was an off hand example, my primary intention was to point you towards the SPL. I did base something similar on an ArrayObject, but the requirements weren't exactly the same.
Still bits and pieces of your code could be rewritten to take advantage of native functionality. For example, this:
public function remove(CoreObject $Object) {
$objectIndex = $this->indexOf($Object);
if ($objectIndex >= 0) {
$this->dataStorage[$objectIndex] = null;
$this->resizeList();
}
}could possibly be rewritten as:
public function remove(CoreObject $Object) {
$objectIndex = $this->indexOf($Object);
if ($objectIndex >= 0) {
unset($this->dataStorage[$objectIndex]);
$this->dataStorage = array_values($this->dataStorage);
return true;
}
return false;
}and avoid the costly call to
resizeList(). And do return something, tell the user if the operation succeeded or not, regardless of what the Java interface does.I chose -1 as the return value for index() based on 2 reasons.
...
(2) I don't want users attempting to simply test for falsehood on the return value.
Your users should check for falsehood like that:
if ($index !== false) {
// do stuff here
}You shouldn't have to compensate for users that are not familiar with PHP's comparison operators.
Code Snippets
/**
* Returns the number representing the index for the passed element, or -1 if
* the element could not be found.
*
* @param CoreObject $Object
* @return int
*/
public function indexOf(CoreObject $Object) { ... }public function set($index, CoreObject $Object)public function remove(CoreObject $Object) {
$objectIndex = $this->indexOf($Object);
if ($objectIndex >= 0) {
$this->dataStorage[$objectIndex] = null;
$this->resizeList();
}
}public function remove(CoreObject $Object) {
$objectIndex = $this->indexOf($Object);
if ($objectIndex >= 0) {
unset($this->dataStorage[$objectIndex]);
$this->dataStorage = array_values($this->dataStorage);
return true;
}
return false;
}if ($index !== false) {
// do stuff here
}Context
StackExchange Code Review Q#6580, answer score: 4
Revisions (0)
No revisions yet.