Hi,
Hope you don’t mind me sharing a bit of a design problem with the v3.1 charts api:
Here’s an example:
Subtitle st = getConfiguration().getSubtitle();
This always gives me back a subtitle. It also always sets a subtitle for the configuration.
So, this method cannot be used to check if there is a subtitle.
This type of ‘auto-setting’ within getters:
public Subtitle getSubTitle() {
if (subtitle == null) {
subtitle = new Subtitle();
}
return subtitle;
}
is generally not a great practice for api-based libraries, as it fundatmentally ‘changes the rules’ of what is expected by a getter.
Of course, now that the api is released, it’s too late to change the behaviour.
Instead, could you please add hasXX()… methods for each of the attributes get/set in this way?
e.g. public boolean hasSubtitle()
This way, the API stays consistent but at least there’s a way to test if attributes are set or not.
The reason behind this change was to unify the developer experience within Vaadin Charts API, some getters were already creating the instance if it didn’t exist and some others were returning null.
In the case you mention (getSubTitle) returning a new instance has been the behavior since Vaadin Charts 1. But other getters used to return null.
With this change a lot of boilerplate code is avoided like in the following example (a bit exagerated, I know):
Configuration conf = chart.getConfiguration();
NoData noData = new NoData();
Position position = new Position();
noData.setPosition(position);
position.setVerticalAlign(VerticalAlign.TOP);
conf.setNoData(noData);
Can be reduced to a single line:
chart.getConfiguration().getNoData().getPosition().setVerticalAlign(VerticalAlign.TOP);
I’m not saying that every line should look like that, but in many cases a single property is going to be set and a lot of meaningless lines of code are avoided.
As a note, this instantiation is only done for classes that are in charts model package.
If you just need to overwrite the old value you can still use the property setter. And if you need to check if it was previously set you can check for a property in the instance like the following code does:
conf.getSubTitle().getText();
I know it’s a bit silly that the getter is creating the instance if you just want to check if it existed before, but that’s the way it works at the moment.
Adding boolean hasProperty() methods is something we will definitely consider, thanks for the suggestion.
Hi Guille,
The: conf.getSubTitle().getText() is exactly the workaround is used!
It’s a bit counter-intuitive, and causes more boilerplate code. Actually I like the idea of adding hasProperty()… - it has many advanatages, including not having to change the existing api!
Many thanks for your prompt response.
Peter
Hi Peter. I am the Product Owner for Pro Tools, which also includes Vaadin Charts. I’m the one to blame for the decisions made regarding the getters.
We had it like you wanted in the start, that getSubTitle() returns null if it is not set, but the code just got so verbose, as Guillermo’s code example showed. Adding a property, which now can be done in one line, usually took something like six lines of instanciating objects and setting them, before you actually was able to set the one property that you wanted.
The other argument to go that route was that we didn’t really find use cases for when you needed to check the getters if they are set or not. The normal case is that developers instanciate a chart, call all the setters and adds the charts to a layout, without checking values of getters. You clearly have a need for getting the values from the getters, which we weren’t able to antipicate. Could you share more info to us on why you check if the properties are set or not? This way we could understand better to which end you need the info and can discuss about changes that we can do to fix the use case itself.
I don’t want to blindly add hundreds of has*() methods without having considered the use and implications of such a change. If the same use case can be achieved in otherwise, I would consider the added methods as API bloat.
Hi Jens,
Oh, it’s not a question of blame ! It’s design choices…and there’s nothing wrong at all with your thinking about ‘making lots of calls vs making one line call’ (the ‘builder’ pattern).
The Use Case for using the getter to ‘test’ for presence is quite straightforward: A Chart is instantiated (through a class hierarchy) and set to configured ChartType, for example, Pie. The user can decide to change the chart type to, say, Bar. At this point, the chart object’s Configuration is modified to reflect the new settings (Bar, axes visible, data labels visible, colors etc.). It is here where we need to test things before we change them (e.g. is there a subtitle existing already?). The subtitle one is a bit of a workaround, as the way Titles (not Subtitle) are centered is separate from the way the chart is centered (but that’s a separate issue).
To summarize, during chart Configuration modification, there are many scenarios where we need to test what the Configuration looks like before changing it. Currently ‘getSubTitle()’ does change the Configuration object if it wasn’t set before.
I would agree that adding a whole bunch of hasXXX() methods and maintaining them over time could be a bit cumbersome, which is why the getter()/setter() pattern is a good one - everyone knows what to expect. Changing the getter to a ‘builder’ pattern changes the expected behaviour, even if for valid reasons. I personally prefer the ‘verbose’ listing, as six lines of clear concise code is far less prone to error and easier to maintain than one line of code that contains ‘implicit’ (and unconventional) behaviour. A bit of typing never hurt anyone!
Now the API is released, to change behaviour yet again presents its own problems, and you’re the Project Owner, so that’s your call! There is a fair amount of refactoring that probably should take place to get the camelCase naming conventions back to ‘standardized getter/setter’ (e.g. getSubTitle() returns a Subtitle), so maybe that’s the time to add hasXX or some other mechanism?
I hope this feedback helps, and thanks for your time and efforts on vaadin-charts!
