Nathan Bubna wrote: ...
yeah, i guess that make sense. thanks!
:)
More comments inline... [EMAIL PROTECTED] wrote: ...
This class is an ideal candidate for JUnit tests.
I was looked very briefly for a way to write a JUnit test, but didn't notice a framework for doing so in the Velocity Tools package.
public class Alternator { private List list; private int index = 0; private boolean auto = false;
/** * Creates a new Alternator for the specified list. */ public Alternator(List list) { this.list = list; }
Why not default to auto? This will be the de facto use case for this tool.
i was trying to follow the principle of least surprise and felt that have toString() work like that be default might be pretty surprising. but i don't feel strongly about this. if anyone else chimes in on this, feel free to change it.
I see what you mean. Still, I feel pretty strongly that "auto" is the common use case, and that the default behavior should support that use case. Regarding your point about side-effects, I see documentation as the answer. I'll add the JavaDoc, but is there somewhere else I should document that as well?
All the ctors might be best off throwing IllegalArgumentException when passed a null list.
ok. but only as long as the AlternatorTool catches them or otherwise ensures they don't happen during template rendering. i generally prefer tools return nulls rather than throw exceptions.
For this case, me too. I was thinking that AlternatorTool would retain its null checking behavior, but Alternator itself would acquire the exception raising. I see this as better support for systems which don't use the toolbox concept (e.g. side-stepping AlternatorTool all together).
...
/** * Returns the current item without shifting the list index. */ public Object getCurrent() { return list.get(index); }
/** * Returns the current item before shifting the list index. */ public Object getNext() { Object o = getCurrent(); shift(); return o; }
getNext() is a really confusing name for this method, and doesn't provide behavior orthogonal to that of getCurrent(). What it's doing seems to be more along the lines of "get current then shift", which would be more appropriately be named along the lines of "getThenShift()".
getNext() is so named to parallel the Iterator paradigm of "next", but in a way that is more VTL friendly (e.g. $color.next instead of $color.next() ). i think it is a common enough paradigm to not confuse people for too long. and i don't like the idea of $color.thenShift i think that would be more confusing.
Yeah, $color.thenShift sucks. It's somewhat confusing that what's internally the "current" state is externally the "next" state. *shrug* When you explain it like that, it makes more sense. Perhaps more JavaDoc to differentiate between getCurrent() and getNext() is what I'm looking for here. Dunno..
getCurrent() exists only to provide a means for retrieving the current object without shifting. this isn't very important for non-auto alternators, but i figure it may come in handy with auto-alternators when both toString() and getNext() would shift.
Yeah.
public class AlternatorTool {
public AlternatorTool() {}
Any reason for this ctor to exist?
no practical one.
The only reason I could come up with is "just in case someone wants to Class.forName().newInstance()-it", which didn't seem to compelling, but you never know. Think I should remove it?
.../** * Make a non-automatic [EMAIL PROTECTED] Alternator} from the values * in the specified collection. */ public Alternator make(Collection collection) { return make(false, collection); }
/** * Make an [EMAIL PROTECTED] Alternator} from the values * in the specified collection. */ public Alternator make(boolean auto, Collection collection) { if (collection == null) { return null; } return make(auto, new ArrayList(collection)); }
Use of java.util.Collection implies that order is not important. For the Alternator class, order IS important. If you're really set on retaining this the API, I strongly recommend that you document that the list will be created in order returned by the Collection's Iterator. Personally, I think this sort of behavior is something which should be left up to the caller.
yeah, the Collection support was a last minute thought. i just figured since #foreach can handle Maps as well as Lists and Object[]s, we might as well support Maps with Alternator. this was just the simplest way. i'm not too attached to it if any thinks it should go.
Okay, done. Worst case, this means an extra "new ArrayList(collection)" for the caller when collection isn't an instance of List.
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
