chkal / mvc-spec-migration-test

0 stars 0 forks source link

Review Models #28

Open chkal opened 9 years ago

chkal commented 9 years ago

Original issue MVC_SPEC-28 created by rmannibucau:

Here few point to think about before considering model as aprt of the API IMO:

1) Map shouldn't be a contract for Models: why would map be part of the API, how to control the API with several version of java? Why so many methods 2) Should Models be fluent (big +1 from me), ie put("xx", x).put("yy", y).... 3) is Models needed at all? Why not using plain Pojo mapping only?

chkal commented 8 years ago

Comment by Manfred Riem:

From my perspective the current design of Models is sufficient.

chkal commented 8 years ago

Comment by Manfred Riem:

Note you can use the CDI @Named facility if you do not like the Models API

chkal commented 8 years ago

Comment by Santiago Pericas-Geertsen:

The use of Map integrates nicely with view engines. There are already POJO alternatives using CDI as was pointed out.

chkal commented 8 years ago

Comment by rmannibucau:

1 for 1) and 2).

Using Map as contract leads to several uneeded issues (doesnt prevent the Model to have a asMap() method), you have to respect a contract you can not need, you rely on methods you don't control (see java 7-> java 8 Map contract) and you can't ensure you use one method to fill the model (= pevent future updates with model listeners for instances).

Not being fluent makes the API not nice enough for several simple but common cases (in particular today) if 1) is done no reason to not being fuent.

chkal commented 8 years ago

Comment by Christian Kaltepoth:

To be honest, I agree with some of Romain's arguments. I don't think it makes sense to have all the methods required by the Map interface in Models. And we lose control over or own API. I think we will be more flexible in changing the API in future spec versions if we define the contract instead of relying on Map. Perhaps we should prefer a much simpler and more fluent API. Something like.

public interface Models extends Iterable<String> {

    Models set(String name, Object model);

    Object get(String name);

    Map<String, Object> asMap();

}

Please note that this is very similar to our first draft of the Models class. Not sure why the API was changed back then.

Perhaps it would at least make sense to discuss this on the mailing list?

chkal commented 8 years ago

Comment by Santiago Pericas-Geertsen:

Frankly, I see pros and cons in using Map as I see pros and cons in creating our own interface. This is why after discussing with Manfred, we decided not to make any more changes for now.

In particular, I don't buy the "losing control over or own API" argument --sometimes it is better to reuse than reinvent. The concern about the number of methods is an implementation problem, not an application one, and easily solved via delegation. The concern about the API not being fluent is a valid one, provided that the number of models set on average is, say, 3 or more.

We can certainly discuss this on the alias, but all this discussion should be public there as well.

chkal commented 8 years ago

Comment by Christian Kaltepoth:

I agree that there are pros and cons for both options. That's why I think it is important to hear what others think about this topic.

I just posted a summary on the alias:

https://java.net/projects/mvc-spec/lists/jsr371-experts/archive/2015-08/message/22

Thanks everyone for sharing your initial thoughts. I hope my summary doesn't miss any argument.