Re: Alternator and AlternatorTool

[Daniel Rall <dl...@collab.net>]

Nathan Bubna wrote:
...
> yeah, i guess that make sense.  thanks!

:)

>>More comments inline...
>>nbubna@apache.org 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 {@link Alternator} from the values
>>>       * in the specified collection.
>>>       */
>>>      public Alternator make(Collection collection)
>>>      {
>>>          return make(false, collection);
>>>      }
>>>
>>>      /**
>>>       * Make an {@link 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: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org

Re: Alternator and AlternatorTool

[Nathan Bubna <na...@esha.com>]

Daniel Rall said:
> Nathan Bubna wrote:
...
> >>More comments inline...
> >>nbubna@apache.org 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.

<confession type="guilty">
I've yet to use JUnit or any other automated code testing.
</confession>

this is just one of those things that i've always felt i should do but was
never motivated enough to get around to doing.  if anyone more familiar with
this stuff is willing to help, that'd be great.

> >>>  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.

that's good enough for me. :)

> 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?

not at the moment, javadoc is all i've got for these classes so far.  i did
fix up a few places in the javadoc that you missed though. :)

> >>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).

sounds good to me.  go for it.

> ...
> >>>      /**
> >>>       * 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*

yeah, i guess the difference between implementation and interface is
confusing.

> 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..

hey, i'm always a fan of having more javadoc. :)

> > 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?
...

i really don't care either way.

Nathan Bubna
nathan@esha.com


---------------------------------------------------------------------
To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org