Keyed list entry builders' copy constructor checks for null key
Description
Activity
Show:
Robert Varga October 10, 2018 at 4:34 PMEdited
The second part of this issue is split off in https://lf-opendaylight.atlassian.net/browse/MDSAL-375#icft=MDSAL-375. An additional improvement to builders lifecycle is tracked in https://lf-opendaylight.atlassian.net/browse/MDSAL-376#icft=MDSAL-376. This issue should provide baseline refactoring of AbstractBuilderTemplate so that the two follow-ups work on independently.
Done
Details
Details
Assignee
Robert Varga
Robert VargaReporter
Robert Varga
Robert VargaComponents
Fix versions
Priority
MediumCreated October 10, 2018 at 3:58 PM
Updated October 12, 2018 at 12:25 PM
Resolved October 12, 2018 at 12:25 PM
When we generate a Builder and its corresponding internal implementation we are performing the following check:
if (base.key() == null) { this.key = new LstKey( base.getKey() ); this._key = base.getKey(); } else { this.key = base.key(); this._key = key.getKey(); } this.augmentation = ImmutableMap.copyOf(base.augmentation);Note that getKey() here refers to the leaf which is a part of the key. This is something we are reusing between builder and implementation, hence they look essentially the same.
Ever since we correctly override key() from Identifiable, Eclipse is warning us that the invocation in Builder's copy constructor is superfluous: key() is required to return non-null, hence there is simply no point in checking for it being null and having that boiler plate present.
key() method here is also special (a bit), as it should not be a straight-up getter, but it should instantiate the key lazily on-demand. That obviously means that key constituent fields need to be checked for nullness during implementation instantiation – note that IllegalStateException can be thrown from Builder.build().
These two points should addressed separately, with the builder copy constructor piece being addressed first.