[MDSAL-374] Keyed list entry builders' copy constructor checks for null key Created: 10/Oct/18 Updated: 12/Oct/18 Resolved: 12/Oct/18 |
|
| Status: | Resolved |
| Project: | mdsal |
| Component/s: | Binding codegen |
| Affects Version/s: | None |
| Fix Version/s: | 3.0.1 |
| Type: | Improvement | Priority: | Medium |
| Reporter: | Robert Varga | Assignee: | Robert Varga |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||
| Description |
|
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. |
| Comments |
| Comment by Robert Varga [ 10/Oct/18 ] |
|
The second part of this issue is split off in MDSAL-375. An additional improvement to builders lifecycle is tracked in MDSAL-376. This issue should provide baseline refactoring of AbstractBuilderTemplate so that the two follow-ups work on independently. |