David Soria Parra [2006-09-14 17:59 UTC] Not tested the code right now on my machine but I looked into the code and have a few suggestions. First of all I think a kind of sort whould be nice. I think a callback mechanism is quite usual for linked lists.
function callback(My_Structures_Linked_List_Node $a, My_Structures_Linked_List_Node $b) {
if ($a->data > $b->data) {
return 1;
} else if ($a->data == $b->data) {
return 0;
} else {
return -1;
}
}
The list check if the callback returns -1 while inserting and appends either before or after the current node depending on the settings (STRUCTURES_LINKED_LIST_ADD_BEFORE, STRUCTURES_LINKED_LIST_ADD_AFTER). This could be usefull to implement a priority queue using the linked list. An ADD_APEND option is not needed any longer, because it can be handled using an callback that only returns -1-
Also you should use type hinting if you the package allready depends on PHP 5 (but i'm not sure if that corresponde with the pear coding styles).
So:
public function addNode(Structures_Linked_List_Node $new_node)
Nice approch
greets
Martin Jansen [2006-09-15 09:08 UTC] * I can't open the .tgz file.
* Shouldn't the name of the package somehow reflect that it implements a doubly-linked list? Otherwise we'll run into trouble if someone eventually proposes a singly-linked list implementation.
Scott Mattocks [2006-09-15 12:41 UTC] Why put a singleton errorstack instance into the GLOBALS array? You can get to that instance at any time using the singleton method.
Don't use double quotes unless you have to. (I don't see anywhere that you have to)
I think the API would be cleaner and easier to use if instead of:
addNode($newNode, $mode, $oldNode)
you had:
prependNode($node)
appendNode($node)
insertNode($newNode, $oldNode, $before = false)
I don't see the much value in:
$retval = false;
return $retval;
Just return false instead of wasting an operation assigning it to something else first.
The package should be named Structures_LinkedList with the classes named Structures_LinkedList and Structures_LinkedList_Node.
Dan Scott [2006-09-16 02:26 UTC] David: I agree, a sorting method would be nice; I'm going to focus first on ensuring that the existing code is stable and well-tested, but that ranks high up there in priority. Also, I've added the type-hinting as you suggested; it helped point out a few areas where I was being lax. Thanks!
Martin: You probably have to 'pear install --force' the package, as I put pear.php.net in the package.xml and there's no way the md5sum exists on pear.php.net before the package has been approved :)
I could consider Structures_Doubly_Linked_List as a package name; it just seems pretty long and in a language where arrays dominate I have trouble imagining anybody else wanting to contribute a singly linked list PEAR package... but my imagination may be quite limited.
Scott: I was struggling to understand PEAR::Errorstack, so when I managed to get it to work I was happy to return to focusing on the core code. My intention was to revisit the error-handling after the proposal was accepted and before going stable. After a few discussions with Pierre, though, I believe I will switch the package to use exceptions to comply with the previously accepted RFC on error handling for PHP 5 packages. Which probably means doing some work on PEAR::Exception.
I'll revisit the addNode() API; you're right, having separate functions instead of having to pass a special constant would make things easier. To group the functions alphabetically, though, I'm thinking about something like:
addNodeAppend()
addNodeInsert()
addNodePrepend()
Good point on the package name - I misread the _ per level of hierarchy thing in the coding standards. I'll go through and rename everything once we figure out it we want Structures_LinkedList_Double (to allow Structures_LinkedList_Single, as Martin suggested) or just plain old Structures_LinkedList.
When returning by reference (since PHP 5.1 I believe), the '$retval = false; return $retval;' thing is necessary to avoid an E_STRICT warning (Strict Standards: Only variables should be assigned by reference). And I'm being a good boy and trying to be E_STRICT compatible :)
Justin Patrin [2006-09-16 03:46 UTC] I like appendNode(), prependNode(), and insertNode() myself. There's no need for the additional "add" verb here as the verb is already there in the above. Don't worry about alphabetical order, just order your user docs correctly. :-)
Martin Jansen [2006-09-21 09:02 UTC] I agree that "Structures_Doubly_Linked_List" is stupid. Let's go with your initial naming suggestion.
Dan Scott [2006-09-21 11:34 UTC] Martin: You might have missed it, but in the most recent entry for the changelog, I switched to "Structures_LinkedList" and now provide both a singly-linked list and a doubly-linked list implementation thanks to the wonders of OO inheritance. So we're in sync (and the quality of this package has gone up enormously since the first proposal, thanks to you all).
|
