FWD » Core Development » Database

I committed an early implementation for the RetrieveResults, the code is in 9683a/15725. Code formatting, javadoc and proper encapsulation are not done yet.

9683a seems to be on the right track.

At this is in an early stage, I may say that it has quite a high cognitive complexity, mostly due to the high number of conditionals:

• RetrieveResults has 8 class members and 4 constructors. The constructors are setting only 0, 1 or 2 such members.
  • this represents a problem because, for example, hasRow is set only in 1/4 constructors.
• getRow has around 6 if conditionals and 3 switch conditionals. These generate many possible execution paths (in theory) - so we need to ensure all execution paths are tested completely and work as expected.
  • I don't see a place where cachedRow is instantiated, but I see several places where it is used.

• CollectingRetrieveResults will implement collectRowData and will be returned only in the place where collectRowData was actually used in trunk.
• PeekingRetrieveResults, PreselectingRetrieveResults will implement their own switch-case I suppose. However, I think these can be merged into one single CachedRetrieveResults

This is just an early design idea to kickstart a plan for segregating RetrieveResults's implementation in smaller parts, so please feel free to interpret it as you need and continue implementation.

I committed rev 15726. It contains a new interface and a few implementing classes.

Review of 9683a:

-      
-      Object[] ids = retrieve(NEXT, lockType, iterating);
+
+      Object[] ids = retrieve(NEXT, lockType, iterating).getRow();

       if (isScrolling() && !isPresorted())
       {
          cursor.addResultNext(ids);
       }
-      
+
       return ids != null;
    }

• There are some javadoc to change that still state Array of record IDs reflecting the first query result
• Small misalignment on CompoundQuery::processComponent invoking. Other places as well.
• implements should be on a separate line
• I am not a personal fan of having overloaded constructors that are not related. For instance, calling hasRow makes sense only if using the RetrievePeekingResults(boolean hasRow) c'tor. Otherwise, it defaults to false ... is this right? The same goes for retrieveMode and query that are initialized with 0 and null ... which I think are not valid states always. Or if this is right, then setting them explicitly and making the props final is cleaner.
  • same applies to RetrieveCachedResults and RetrievePreselectResults. Please stick to one single c'tor that sets the final props accordingly. You can overload them as long as they are calling each other until a master c'tor and you pass default values on the way.
• there are some open braces on the same line as the statements

Addressed a few formatting issues and rebased on trunk rev 15866.

Rebased on trunk rev 15891. Alexandru, could you please look at the last commits, I would like to move on with testing 9683a, if the review is ok.

• implements should be aligned to public (at 3 spaces from the start of the line).
• Typo in RetrievePeekingResults.retrieveMode javadoc (mod instead of mode).
• please make all fields of such Retrievable implementations final. This will enforce setting them from the constructor and not miss the right initialization of any field.
  • please attempt to have only one constructor that sets fields. All of the other ctors should only call the "master" ctor with default values.
  • this applies to all Retrievable implementations
  • I want to avoid the high dependency between what API is called (hasRow, getRow) to the constructor overload (with hasRow or retrieveMode and query).
• New line is required after RetrieveCachedResults.cachedRow
• I think the combination of a ctor that provides a cachedRow and a setter for the cachedRow confusing. I might expect to either initialize the results with a cached row or set it along. This suggests the need to have the cachedRow field as final.
• Shouldn't the peeknext and peekprevious return Retrievable? Add the row to the cursor only by calling getRow inside of that. For non-scrolling queries, you won't need to call getRow.

• return new RetrievePeekingResults(true); is returning an instance of peeking results that have a row. However, a call to getRow on this results will yield NPE (because hasRow is true, but query is null).
• return new RetrievePeekingResults(navigation, query); is returning an instance of peeking results that can retrieve a row. However, a call to hasRow is false.
• retrieve(CURRENT, lockType, false) == null was changed to !retrieve(CURRENT, lockType, false).hasRow(). This is right, but according to current implementation, hasRow and getRow are sometimes out of sync.
• RetrievePreselectResults can return null with getRow in case it is non-scrolling and off-end. So hasRow can also be false in that case.

Rebased 9683a on trunk rev 16047. Addressed review points in rev 16053.

We need to finish the work in #6720.

Greg Shah wrote:

We need to finish the work in #6720.

Before finishing up #9683 or instead of #9683?

I assume we will finish this task first, but I am highlighting that we really need to finish the general solution to this problem, which is in task #6720.