FWD » Core Development » Database

Radu, please fix this in 7026d.

Review of 7026d 14563-14564:

• Extent can be null! Don't use null as a marker to check if it was computed or not. Use some separate variable (extentIsComputed).
• Note that having a null setter is also valid state if isIdAccessor is true. You only throw IllegalArgumentException if the setter is null and !isIdAccessor. Please use a separate setterIsComputed to ensure that null is actually generated and not just a lazy computation maker. If you want to have everything other control, you can also do a getterIsComputed, but this is optional.
• You have if (getter null) getter = getGetter(); if (getter null). Please reduce it to one single check getGetter() null. This also happens for extent. However, I think this is regarded to my last comment.
• You have if (bulk null) bulk = (bulk != null) && (extent != null) && (index == null). This makes the first condition (bulk != null) always true. Note that you have the constructor bulk you are no longer using. You shouldn't lazily initialize bulk. You can save the constructor value into a "allowBulk" or something like this and use it afterwards.
• Inline if (getter == null) getter = getGetter() return getter.getReturnType() into return getGetter().getReturnType(). The null check is already done in the getGetter. The same goes for getSetter and getExtent. The performance we try to save here is not from a simple native method call (which is already super-fast, especially with no parameters), but from some map look-ups and stuff like this. Now that you also have extentIsComputed and setterIsComputed, this whole check gets hard to manage, so leave the lazy checks only in getGetter, getSetter and getExtent.

Review of 7026d, rev. 14570

• In set, can you extract getSetter result into a local setter variable, so that Utils.invoke uses the local variable and not the class field? I am a bit afraid of accessing setter class field directly. Even if it isn't null due to the check, someone may decide to drop that IllegalStateException check and fail to use the right setter in the end. The same goes for getObject and unknownValue.
• In equals, this.extent = getExtent(); that.extent = that.getExtent(); doesn't look quite right. Can you use Objects.equals(getExtent(), that.getExtent()) directly?
• Please use !isBulkComputed instead of isBulkComputed false or isBulkComputed instead of isBulkComputed true.
• For the second constructor, you can set isSetterComputed on true, because the setter is null anyway. The same for bulk and extent.

Review of 7026d/rev. 14574
I am OK with the changes. Have you tested the latest version (rev. 14574) of 7026d with a large customer application? I want to know if all the changes of 7026d are still doing fine now and we are on track with a stable 7026d.

Merged 7026d to trunk as rev. 14587.
This can be closed.