Review: Needs Fixing

- The implementation looks very clean. Only a couple of minor comments that 
might help removing some code but I couldn't find any major rejection points. 
- ChangeLog entry is missing
- I understand why you wanted to rename the function from replace to edit and 
your concern makes sense. However, the description of the function still uses 
the term "replace" and I kind of like the explanation as it is.
- The comment should mention that you can't replace a node with an object or 
vice versa (mentioning inconsistent kind with the ZDDY0040 error is not enough 
for the user). Also, that it's only possible to replace root nodes.
- Is there no more efficient way to find out whether a JSONItem is the root of 
a tree or not? Currently, it seems that the searching only works for ordered 
collections because it's using findNode.
- Could the Item::swap function be implemented on JSONObjects, JSONArrays, and 
the base-class for all nodes instead of for Item. It seems like that checking 
if a node is the root and swapping the tree in XmlNode::swap would be 
sufficient. That seems to be more consistent with the way other functions are 
implemented. It also seems to avoid the switch in Item::swap and the 
replication of raising ZSTR0050.
Your team Zorba Coders is subscribed to branch lp:zorba.

Mailing list:
Post to     :
Unsubscribe :
More help   :

Reply via email to