Differences between revisions 7 and 8
Revision 7 as of 2012-06-13 15:38:32
Size: 6312
Editor: MattBenson
Comment:
Revision 8 as of 2012-06-13 15:39:10
Size: 6336
Editor: MattBenson
Comment:
Deletions are marked like this. Additions are marked like this.
Line 10: Line 10:
   Agreed about the first point; added https://issues.apache.org/jira/browse/FUNCTOR-13 . As to why the ctor is public, the javadoc says "This constructor is public to permit tools that require a JavaBean instance to operate." Not sure how many tools actually still need this. Velocity was the primary suspect for ages; does anyone know if it still needs a bean instance? '''UPDATE''' it was quick work to google that Velocity does now support calling static methods, so we can IMO end this practice for current and future development throughout Commons.    Agreed about the first point; added https://issues.apache.org/jira/browse/FUNCTOR-13 . As to why the ctor is public, the javadoc says "This constructor is public to permit tools that require a JavaBean instance to operate." Not sure how many tools actually still need this. Velocity was the primary suspect for ages; does anyone know if it still needs a bean instance? '''UPDATE''' it was quick work to google that Velocity does now support calling static methods with no object instance, so we can IMO end this practice for current and future development throughout Commons.

Address open questions:

  • there are several marker interfaces, but maybe some annotations would make more sense?

    • This and the next question seem to dovetail since "several" marker interfaces == Functor/NullaryFunctor/UnaryFunctor/BinaryFunctor. See my comments on that question for why I think it is a good idea to continue strongly distinguishing these n-ary functor "profiles." These marker interfaces were added because I thought it might occasionally be handy to be able to broadly classify functors, n-ary functors. I can't think of a more usable way to accomplish the same goals using annotations.
  • couldn't Function, UnaryFunction and BinaryFunction be unified with a single interface using a vararg parameter?

    • (Same question would apply to -Predicate and -Procedure in addition to -Function) Using a vararg parameter would preclude the binary (and hypothetical ternary, etc.) functors from supporting different strongly typed arguments. I think it would be a step backward. MJB
  • EachElement should be able to work on any Iterable. Also I'm not sure to understand why its constructor is public.

    • Agreed about the first point; added https://issues.apache.org/jira/browse/FUNCTOR-13 . As to why the ctor is public, the javadoc says "This constructor is public to permit tools that require a JavaBean instance to operate." Not sure how many tools actually still need this. Velocity was the primary suspect for ages; does anyone know if it still needs a bean instance? UPDATE it was quick work to google that Velocity does now support calling static methods with no object instance, so we can IMO end this practice for current and future development throughout Commons.

  • Shouldn't Generator implement Iterable?

    • It doesn't seem so to me. It's possible some more expository name could be found for Generator.
  • why are equals, hashCode and toString defined in the Functor interface?

    • The original author/s of [functor] presumably used this as a natural place to put their javadoc comments urging that functor implementations properly implement these methods. Project Lambda will refer to single-method interfaces as "functional interfaces." For this reason I now believe we should plan ahead for interoperability and drop the explicitly-referenced #equals()/#hashCode()/#toString() in oacf.Functor, i.e. so that [functor]'s interfaces will satisfy this definition and thus be immediately usable with Lambda-style closures. https://issues.apache.org/jira/browse/FUNCTOR-20 has been created to track this item.

  • why Predicate isn't an extension of Function<Boolean> ?

    • I personally support the original authors' decision to let each functor type have a simple and semantically straightforward API. [functor] already has adapters to handle this transformation; forcing Predicate to be Function<Boolean> would require either a less semantically pleasing API, or a more complicated implementation task.

  • why Procedure isn't an extension of Function<Void> ?

    • See answer to previous question.
  • Why are constants available through both a static field AND a static method? For example Identity.INSTANCE and Identify.instance(), or Constant.TRUE and Constant.truePredicate() ?

    • The static method is the option to choose for strong typing. I don't feel strongly about retaining the static fields which of course predated the generics work. If you want, JFDI.
  • The Javadoc for Limit states "A predicate that returns true the first n times it is invoked.", but what happens after? Is it the opposite of Offset?

    • Same Javadoc comment says "and <code>false</code> thereafter." It is indeed the opposite of offset.

  • Why aren't Limit and Offset serializable like the other classes in the core package?

  • Limit and Offset could probably use an AtomicInteger instead of a synchronized block

    • I'm not filing an issue for this yet, but feel free to make this change if on further investigation you still feel it to be proper, or I will do the same. I haven't made use of the Atomic- number classes myself so I would have to study them first. MJB
  • the site has no example easily accessible, the reader is invited to browse the JUnit tests. That's not really user friendly.

  • I see IllegalArgumentExceptions thrown for null values, shouldn't this be changed to throw NullPointerExceptions ?

  • @inheritDoc tags should be removed if no additional description is provided in the subclasses. This tag is only useful for extending the description from the method of the super class.

    • It's also useful for establishing some comfort level that someone has at least thought about whether further javadoc is needed for a given method, and satisfies e.g. checkstyle rules that exist for the same reasons. This is one of those religious topics that will keep people busy for weeks with no resolution, but if you feel that strongly let's have a Commons-wide vote once and for all about whether to require javadoc etc. etc. Otherwise it's just per-project preference IMO, much like the other style rules (personally I'd rather have a uniform code style across Commons than optional javadoc in X component, but that's just me). MJB

  • what's the point of having equals(T) methods in addition to equals(Object) ? It clutters the API.

  • CollectionTransformer has a todo stating : "TODO revisit this class..."

    • True; the issues noted need to be addressed before a release.
  • There are several untested classes in the composite package

  • Most of the equals/hashcode methods are not tested

Sanity Check of APIs, etc. (last edited 2012-06-13 15:39:10 by MattBenson)