calrissian / mango

Common utilities for rapid application development
Apache License 2.0
17 stars 7 forks source link

Entity/Event builder to provide attr-like methods specifically for adding a relationship to another entity/event #173

Closed cjnolet closed 9 years ago

cjnolet commented 9 years ago

Currently, in order to implement a relationship to another event/entity, the user has to do this:

.attr("name", new EntityIndex("type", "id"))

Perhaps this could be done in the EntityBuilder to make it a little easier for the user:

.entityRel("name", "type", "id");

Or even like this:

.entityRel("name", entity);

eawagner commented 9 years ago

-1

I don't like to make the distinction of relationships as something special that requires special handling. One of the main reasons for this is that it implies that all users of the API are forced to handle relationships, even if they never intend to support relationships. For instance if I only want to support simple java type values in my application, I will have to let users know that they shouldn't use the entityRel methods on the builder.

As an alternative to the verbose 'new EntityIdentifier("type","id")' notation we created a static method on the EntityIdentifier class called id() which creates the entity identifier for them. With static imports this ends up being a very clean approach.

cjnolet commented 9 years ago

So a couple comments:

1) Not everyone likes doing static imports. In fact, this is the only project i know of in which it has become standard practice.

2) Given #1, are we hiding the constructor for the EntityIdentifier and just making id() the only method for construction? Won't this get confusing to users?

eawagner commented 9 years ago

I was offering it as a suggestion as something we had done in the past to help deal with the verbosity. The static method constructor approach is a pattern picked up from Guava. Doesn't mean that it is necessarily the best approach here, but thought I would throw it out.

I am against having special entityRel() methods on a builder though, the main point behind the original comment.

cjnolet commented 9 years ago

I can see your point about having special logic that's assuming a specific type in the types system. I'm not challenging you on that. I'm trying to figure out what one of the alternatives would look like. Not allowing users to new up EntityIdentifier() directly seems confusing- but if we were to provide a static factory method, having it be called "id()" seems very confusing especially if they still don't have a constructor.

Again, the only reason I bring up not knowing too many projects that specifically do static imports everwhere is because the only way the id() creation method works to make the creation significantly more terse is if the static import is used. I agree that static factory methods can make things look a lot better (and allow things to change much easier behind the scenes) but I see why Guava used "create()" everywhere for consistency.

eawagner commented 9 years ago

I wasn't suggesting to get rid of the constructor. On our project, we simply provided a static factory constructor because it was less verbose, not to replace the equivalent constructor. Honestly, I am inclined on not providing such utility mechanisms in Mango, as they are trivial to provide outside the library, and add no additional utility functionality other than to deal with verbosity. Personally, I am fine with the

.attr("name", new EntityIndentifier("type", "id"))

While, someone could always make it shorter with utility methods, I don't see that as being essential.

I think we would be better off providing utility methods for accessing identifiers in objects that use them for their primary identification information. For instance:

.attr("name", entity.getIdentifier())

While this is not one such case, there are a lot of other reasons that static factory methods are beneficial which is why I assume Guava uses them. I won't get into those here though as google will provide pages with better descriptions.