cletus
8 Jul 2008, 5:57 PM
Generics are a tool and like any tool they can be used well or abused and, after some extensive use of Ext-GWT, I'm sorry to say that this is (mostly) a
case of abused Generics. I'll highlight some of the reasons why.
Firstly however a note about the positive. The widgets themselves look great and theres a good selection (although, oddly, theres no Image Button;
why?). That is primarily what I want from a UI library. The API in a lot of ways is secondary and this is teh case here too. Secondary doesn't mean
its unimportant however.
I don't know the history of GWT-Ext other than it has its roots in MyGWT (which I liked) and ExtJS (which I've never used). The idea to use generics is
good but generics aren't simple. You only need to see a declaration like "<X> X max(Collection<? super X> collection)" to realize that and they need to
be used properly. For better or worse this takes a fair bit of knowhow. Its why the Java Generics FAQ is as large as it is and why it takes someone of
the calibre of Josh Bloch to explain it.
1. Model
Put simply: all models are Maps. Some might disagree with me but that is (imho) not a good thing. For instance, the ModelData interface has a method:
public <X> X get(String property);
This is obviously so you can do things like:
Date date = model.get("startDate")
int pageCount = model.get("pageCount") // auto-unboxing to boot
While this might be convenient in that you don't have to explicitly cast but because of type erasure, you're not actually gaining anything. This is
perfectly legal code:
model.put("date", "testing 123");
Date d = model.get('date");
The result is of course a ClassCastException. Josh Bloch, just to give one example, suggests the use of heteregenous containers eg:
container.put(String.class, "blah");
In Swing, models are basically just POJOs with relevant adapters like TableModel and frankly instead of:
model.get("dateOfBirth")
model.set("dependants", 2)
I prefer:
model.getDateOfBirth()
model.setDependants(2)
I realize some will disagree with this but remember that a strongly typed model like this doesn't preclude the use of Maps whereas the use of Maps makes
strongly typed accessors and mutators little more than syntactic placebos. Granted also that GWT doesn't have access to reflection, which is potentially
limiting.
2. Model Events
BaseModel doesn't fire a lot of events. Basically the only one is Events.Update. Even removing a property fires an Update event where the new value is
null, which--from an event perspective--is indistinguishable from actually setting the property to null (even though the latter leaves the property in
the map and the former does not). This is a problem.
Shouldn't adding a new property trigger an Add event and rmeoving an existing property trigger a Remove event?
This particular code highlights some issues that are worth pointing out as examples of suboptimal Java code:
From BaseModelData:
public void setProperties(Map<String, Object> properties) {
for (String property : properties.keySet()) {
set(property, properties.get(property));
}
}
should be:
public void setProperties(Map<String, Object> properties) {
for (Map.Entry<String, Object> entry : properties.entrySet()) {
set(entry.getKey(), entry.getValue());
}
}
and this:
protected <X> X getNestedValue(ModelData model, List<String> paths) {
if (paths.size() == 1) {
return (X) model.get(paths.get(0));
} else {
Object obj = model.get(paths.get(0));
if (obj != null && obj instanceof ModelData) {
ArrayList<String> tmp = new ArrayList<String>(paths);
tmp.remove(0);
return (X) getNestedValue((ModelData) obj, tmp);
}
}
return null;
}
has two faults:
1. The cast to X on line 3 highlights the issue described earlier regarding get() (ie unchecked cast); and
2. Line 6 can be simplified to:
if (obj instanceof ModelData) {
If obj is null then that returns false.
3. Nested Properties
For those unfamiliar with it, the BaseModel implementation has a feature called nested properties. Basically if you do this:
BaseModel a = new BaseModel();
BaseModel b = new BaseModel();
a.set("count", 3);
b.set("blah", a);
then you can do this:
b.set("blah.count", 5);
Interestingly, this will potentially trigger TWO Update events: one on "blah.count" in b and one on "count" in a.
4. Model Collections
Models are flat except, as noted above, for nested properties. While this is potentially useful it needs to go further. Specifically I'm talking about
property values that are collections (namely lists). I've tried various ways of implementing some kind of ListModel but all the implementations are
pretty ugly because the Model interface is so geared towards Maps. A Model implementation isn't exactly what you want either because then it can
potentially have its own event listeners. If its not a Model a caller could get access to the List and modify it without triggering any events.
BaseTreeModel sort of does this but its also limited in that theres only one type of child (more on trees below).
Collection properties are a fairly compelling and common requirement (eg for items in a combo box) so I think this is something really lacking. About
the cleanest way to handle this would be to use an observer approach. Ignoring nested properties for simplicity, that would mean something like:
public class BaseModel ... {
class ModelList<T> extends AbstractList<T> {
private final List<T> delegate;
private ModelList(List<T> delegate) {
if (delegate == null) {
throw new NullPointerException("delegate is null");
}
this.delegate = delegate;
}
public int size() {
return delegate.size();
}
public T get(int index) {
return delegate.get(index);
}
public T set(int index, T element) {
// Update event at index
return delegate.set(index, element);
}
public void add(int index, T element) {
// fire Add event at index
delegate.add(index, element);
}
public T remove(int index) {
// fire Remove event at index
return delegate.remove(index);
}
}
}
Note: the above is just a rough guide.
Various methods in BaseModel and BaseModelData would need to be modified to handle collection properties.
Additionally, nested properties should support syntax like:
set("names[3]", "John Smith");
5. Tree Model
I've posted on this previously but for completeness I'll mention it here too. Basically the TreeModel interface is a good example of the abuse of
generics and bad design. Why? Because, TreeModel is an invasive interface. It forces every item in your tree model to implement TreeModel and leads to
silly declarations like this:
BaseTreeModel<BaseTreeModel> ...
This needs a far cleaner interface. The basic concept of a container (which TreeModel is or at least should be) is that it can contain anything. Look
no further than the Java Collections API and you'll see things like:
List<T> ...
not
List<T extends ListItem> ...
A good solid example of trees and tree models can be found in the swing JTree class.
The naming isn't clear either. TreeModel is really a node, not the whole tree model. A rough starting poitn for a good TreeModel interface would be:
interface TreeNode<T> {
TreeNode<? extends T> getParent();
List<TreeNode<? extends T>> getChildren();
T getValue();
TreeNode<T> insert(int index, T item);
T remove(int index);
void removeAll();
T setValue(T newValue);
}
The list of children should either be a defensive copy of the children or unmodifiable.
A TreeNode implementation like this will give you the ability to have async loads of children and so forth, something thats currently handled by a
confusing mix of loaders, binders and stores.
Oddly, TreeModel is generic in the current code but TreeItem isn't. If an internal UI-level tree is needed to mirror TreeNode (eg to contain styles and
ohter tree-implementation-specific state) it should be generic.
6. Events and Listeners
Events are an area desperately in need of review in Ext-GWT. The events themselves are integer constants when they should be an enum. You might argue
that this isn't extendible but thats not true. You simply have:
interface EventType {
String getName();
int getNumber();
}
enum BaseEvents implements EventType {
...
}
enum ExtendedEvents implements EventType {
...
}
and so on and then rather than the enums, the interface is used.
Events themselves are a concrete type with public fields. This should be an interface with a largely immutable implementation. For example:
interface Event {
EventType getEventType();
Object getSource();
}
Also, the generic Listener interface is just painful. addListener() says nothing about what events the model or widget is firing. This is perfectly valid:
textField.addListener(Events.BeforeAdd, new Listener ... )
Listener is generic but not in a useful way. This has led to code like:
tabPanel.addListener(Events.Remove, new Listener<ContainerEvent<TabPanel, TabItem>>() {
...
});
This is crazy. I'd much rather have correctly typed listeners and explicit add...Listener methods rather than addListener. For example:
public interface Model ... {
void addChangeListener(ChangeListener changeListener);
}
public interface ChangeListener {
void propertyChanged(PropertyChangeEvent event); // curently ChangeEvent is passed in and you need to cast it
}
7. Inconsistent Generics
Theres an awful lot of code that partially uses generics. For example, BaseTreeModel.adopt() has a parameter of type TreeModel, not TreeModel<something>. You'll find examples like this and assigning generic arguments to non-generic data members all over the place.
8. Conclusion
Theres some pretty scary stuff in here and I know I've only scratched the surface. The current release is 1.0rc2 and I really have to consider this a political 1.0 release. Its nowhere near ready for primetime. Once 1.0 is release this bizarre API is basically set in stone. Hell, its almost set in stone by the RC designation.
On the project I'm working on we were using MyGWT and quite liked it. It was a natural move to Ext-GWT but we're being forced to abandon it entirely as we learn more about it in favour of vanilla GWT. Sure we won't have ContentPanels that way but we won't have BaseTreeModel<BaseTreeModel> either. A non-generic interface is preferable to a badly designed and inconsistent generic interface and thats what Ext-GWT is.
case of abused Generics. I'll highlight some of the reasons why.
Firstly however a note about the positive. The widgets themselves look great and theres a good selection (although, oddly, theres no Image Button;
why?). That is primarily what I want from a UI library. The API in a lot of ways is secondary and this is teh case here too. Secondary doesn't mean
its unimportant however.
I don't know the history of GWT-Ext other than it has its roots in MyGWT (which I liked) and ExtJS (which I've never used). The idea to use generics is
good but generics aren't simple. You only need to see a declaration like "<X> X max(Collection<? super X> collection)" to realize that and they need to
be used properly. For better or worse this takes a fair bit of knowhow. Its why the Java Generics FAQ is as large as it is and why it takes someone of
the calibre of Josh Bloch to explain it.
1. Model
Put simply: all models are Maps. Some might disagree with me but that is (imho) not a good thing. For instance, the ModelData interface has a method:
public <X> X get(String property);
This is obviously so you can do things like:
Date date = model.get("startDate")
int pageCount = model.get("pageCount") // auto-unboxing to boot
While this might be convenient in that you don't have to explicitly cast but because of type erasure, you're not actually gaining anything. This is
perfectly legal code:
model.put("date", "testing 123");
Date d = model.get('date");
The result is of course a ClassCastException. Josh Bloch, just to give one example, suggests the use of heteregenous containers eg:
container.put(String.class, "blah");
In Swing, models are basically just POJOs with relevant adapters like TableModel and frankly instead of:
model.get("dateOfBirth")
model.set("dependants", 2)
I prefer:
model.getDateOfBirth()
model.setDependants(2)
I realize some will disagree with this but remember that a strongly typed model like this doesn't preclude the use of Maps whereas the use of Maps makes
strongly typed accessors and mutators little more than syntactic placebos. Granted also that GWT doesn't have access to reflection, which is potentially
limiting.
2. Model Events
BaseModel doesn't fire a lot of events. Basically the only one is Events.Update. Even removing a property fires an Update event where the new value is
null, which--from an event perspective--is indistinguishable from actually setting the property to null (even though the latter leaves the property in
the map and the former does not). This is a problem.
Shouldn't adding a new property trigger an Add event and rmeoving an existing property trigger a Remove event?
This particular code highlights some issues that are worth pointing out as examples of suboptimal Java code:
From BaseModelData:
public void setProperties(Map<String, Object> properties) {
for (String property : properties.keySet()) {
set(property, properties.get(property));
}
}
should be:
public void setProperties(Map<String, Object> properties) {
for (Map.Entry<String, Object> entry : properties.entrySet()) {
set(entry.getKey(), entry.getValue());
}
}
and this:
protected <X> X getNestedValue(ModelData model, List<String> paths) {
if (paths.size() == 1) {
return (X) model.get(paths.get(0));
} else {
Object obj = model.get(paths.get(0));
if (obj != null && obj instanceof ModelData) {
ArrayList<String> tmp = new ArrayList<String>(paths);
tmp.remove(0);
return (X) getNestedValue((ModelData) obj, tmp);
}
}
return null;
}
has two faults:
1. The cast to X on line 3 highlights the issue described earlier regarding get() (ie unchecked cast); and
2. Line 6 can be simplified to:
if (obj instanceof ModelData) {
If obj is null then that returns false.
3. Nested Properties
For those unfamiliar with it, the BaseModel implementation has a feature called nested properties. Basically if you do this:
BaseModel a = new BaseModel();
BaseModel b = new BaseModel();
a.set("count", 3);
b.set("blah", a);
then you can do this:
b.set("blah.count", 5);
Interestingly, this will potentially trigger TWO Update events: one on "blah.count" in b and one on "count" in a.
4. Model Collections
Models are flat except, as noted above, for nested properties. While this is potentially useful it needs to go further. Specifically I'm talking about
property values that are collections (namely lists). I've tried various ways of implementing some kind of ListModel but all the implementations are
pretty ugly because the Model interface is so geared towards Maps. A Model implementation isn't exactly what you want either because then it can
potentially have its own event listeners. If its not a Model a caller could get access to the List and modify it without triggering any events.
BaseTreeModel sort of does this but its also limited in that theres only one type of child (more on trees below).
Collection properties are a fairly compelling and common requirement (eg for items in a combo box) so I think this is something really lacking. About
the cleanest way to handle this would be to use an observer approach. Ignoring nested properties for simplicity, that would mean something like:
public class BaseModel ... {
class ModelList<T> extends AbstractList<T> {
private final List<T> delegate;
private ModelList(List<T> delegate) {
if (delegate == null) {
throw new NullPointerException("delegate is null");
}
this.delegate = delegate;
}
public int size() {
return delegate.size();
}
public T get(int index) {
return delegate.get(index);
}
public T set(int index, T element) {
// Update event at index
return delegate.set(index, element);
}
public void add(int index, T element) {
// fire Add event at index
delegate.add(index, element);
}
public T remove(int index) {
// fire Remove event at index
return delegate.remove(index);
}
}
}
Note: the above is just a rough guide.
Various methods in BaseModel and BaseModelData would need to be modified to handle collection properties.
Additionally, nested properties should support syntax like:
set("names[3]", "John Smith");
5. Tree Model
I've posted on this previously but for completeness I'll mention it here too. Basically the TreeModel interface is a good example of the abuse of
generics and bad design. Why? Because, TreeModel is an invasive interface. It forces every item in your tree model to implement TreeModel and leads to
silly declarations like this:
BaseTreeModel<BaseTreeModel> ...
This needs a far cleaner interface. The basic concept of a container (which TreeModel is or at least should be) is that it can contain anything. Look
no further than the Java Collections API and you'll see things like:
List<T> ...
not
List<T extends ListItem> ...
A good solid example of trees and tree models can be found in the swing JTree class.
The naming isn't clear either. TreeModel is really a node, not the whole tree model. A rough starting poitn for a good TreeModel interface would be:
interface TreeNode<T> {
TreeNode<? extends T> getParent();
List<TreeNode<? extends T>> getChildren();
T getValue();
TreeNode<T> insert(int index, T item);
T remove(int index);
void removeAll();
T setValue(T newValue);
}
The list of children should either be a defensive copy of the children or unmodifiable.
A TreeNode implementation like this will give you the ability to have async loads of children and so forth, something thats currently handled by a
confusing mix of loaders, binders and stores.
Oddly, TreeModel is generic in the current code but TreeItem isn't. If an internal UI-level tree is needed to mirror TreeNode (eg to contain styles and
ohter tree-implementation-specific state) it should be generic.
6. Events and Listeners
Events are an area desperately in need of review in Ext-GWT. The events themselves are integer constants when they should be an enum. You might argue
that this isn't extendible but thats not true. You simply have:
interface EventType {
String getName();
int getNumber();
}
enum BaseEvents implements EventType {
...
}
enum ExtendedEvents implements EventType {
...
}
and so on and then rather than the enums, the interface is used.
Events themselves are a concrete type with public fields. This should be an interface with a largely immutable implementation. For example:
interface Event {
EventType getEventType();
Object getSource();
}
Also, the generic Listener interface is just painful. addListener() says nothing about what events the model or widget is firing. This is perfectly valid:
textField.addListener(Events.BeforeAdd, new Listener ... )
Listener is generic but not in a useful way. This has led to code like:
tabPanel.addListener(Events.Remove, new Listener<ContainerEvent<TabPanel, TabItem>>() {
...
});
This is crazy. I'd much rather have correctly typed listeners and explicit add...Listener methods rather than addListener. For example:
public interface Model ... {
void addChangeListener(ChangeListener changeListener);
}
public interface ChangeListener {
void propertyChanged(PropertyChangeEvent event); // curently ChangeEvent is passed in and you need to cast it
}
7. Inconsistent Generics
Theres an awful lot of code that partially uses generics. For example, BaseTreeModel.adopt() has a parameter of type TreeModel, not TreeModel<something>. You'll find examples like this and assigning generic arguments to non-generic data members all over the place.
8. Conclusion
Theres some pretty scary stuff in here and I know I've only scratched the surface. The current release is 1.0rc2 and I really have to consider this a political 1.0 release. Its nowhere near ready for primetime. Once 1.0 is release this bizarre API is basically set in stone. Hell, its almost set in stone by the RC designation.
On the project I'm working on we were using MyGWT and quite liked it. It was a natural move to Ext-GWT but we're being forced to abandon it entirely as we learn more about it in favour of vanilla GWT. Sure we won't have ContentPanels that way but we won't have BaseTreeModel<BaseTreeModel> either. A non-generic interface is preferable to a badly designed and inconsistent generic interface and thats what Ext-GWT is.