These are Ed's notes when reviewing the 20120509 changebundle.
Actionable comments addressed to Manfred start with mriem:.
Please look through these suggestions and address my comments. I think I'd like to see another iteration.
SECTION: test changes
SECTION: State Management changes
General comments on this class:
- Overall, this class looks much cleaner, but I'm always suspicious of
code that looks dramatically simpler, because stuff that looks dirty
is often just bugfixes. Nonetheless, I'll trust that your changes
simultaneously add clarity, remove complexity, and preserve bug fixes.
That's a tall order, though!
- mriem: Four or so methods and two inner classes have been removed.
Where did these end up?
- Remove apologies from class javadoc.
- Remove ctor, moved to JspStateManagementStrategy.
- Remove all ivars, moved to JspStateManagementStrategy.
In method saveView():
- mriem: on line 69, you access the context local variable, but check
for its nullness on line 71. That's an NPE waiting to happen.
- mriem: you've removed the id uniqueness check from this method. I
assume it will show up somewhere else. Please verify that it does.
In method restoreView():
- mriem: you've removed the wordy comment about the need to clone the
tree... Is that business handled somewhere else now? Yes: looks like
it's in the new class JspStateManagementStrategy.
- Assume the responsibilities from the former StateManagerImpl.java
- Removed. Functionality taken over by pair of classes:
still has this old implementation. Is that intentional?
- mriem: you have a zero arg ctor, but it's only called by test code
(and cactus code no less). To guard against someone calling that ctor
for real, perhaps invent some kind of magic argument that is checked
by the ctor and throws an IllegalStateException if it's not right?
Something like a string with a value known by the test and the real
code? The test will pass the literal string?
- Again, this looks much cleaner.
Ahh, here is the id uniqueness check.
How about adding a context param that makes this off when ProjectStage
is Development? After sitting through Leonardo Uribe's MyFaces vs
Mojarra performance comparison, I'm ready to entertain such
- mriem: please add a comment that explains why this class looks so
similar to JspStateManagementStrategy. And a question for you, how
does this actually differ from JspStateManagementStrategy?
- change name of method partialStateSaving() to be
isPartialStateSaving(). Otherwise this file is unchanged.
- Change callsite of partialStateSaving().
- I note that FaceletPartialStateManagementStrategy has a
locateComponentByClientId() method, and FaceletViewHandlingStrategy
does too. How are these to incarnations of the same named method
related? Can this be moved to Util?