8
\$\begingroup\$

I need to write a service to return objects in pages of size 20. The caching of these objects is done in pages of size 60.

To abstract the logic of streaming differently-sized pages, I wrote this PagingAdapter class:

public abstract class PagingAdapter<T> implements Iterable<List<T>> {

    private final int callerPageSize;
    private final int sourcePageSize;

    private final LoadingCache<Integer, List<T>> localCache =
            CacheBuilder.newBuilder().build(new CacheLoader<Integer, List<T>>() {
                @Override
                public List<T> load(Integer sourcePageNumber) throws Exception {
                    return fetchPageFromSource(sourcePageNumber);
                }
            });

    public PagingAdapter(int callerPageSize, int sourcePageSize) {
        Preconditions.checkArgument(callerPageSize > 0, "callerPageSize must be greater than 0.");
        Preconditions.checkArgument(sourcePageSize > 0, "sourcePageSize must be greater than 0.");
        this.callerPageSize = callerPageSize;
        this.sourcePageSize = sourcePageSize;
    }

    /**
     * Returns a List of objects from the source for the specified page.
     * 
     * <p>If the returned List is smaller in size than the source page size, that
     * means there are no further source pages.
     * 
     * @param sourcePageNumber The requested page.
     * @return The List of objects.
     * @throws Exception 
     */
    protected abstract List<T> fetchPageFromSource(int sourcePageNumber) throws Exception;

    /**
     * Returns a List of objects for the specified page, as defined by the caller
     * page size.
     * 
     * <p>If the returned List is smaller in size than the caller page size, that
     * means there are no further pages available.
     * 
     * @param callerPageNumber The requested page.
     * @return The List of objects.
     */
    public List<T> getPage(int callerPageNumber) {

        Preconditions.checkArgument(callerPageNumber > 0, "callerPageNumber must be greater than 0.");

        List<T> callerPage = Lists.newArrayListWithCapacity(callerPageSize);

        int numLeftToFetch;
        while ((numLeftToFetch = callerPageSize - callerPage.size()) > 0) {
            int fetchFromAbsoluteIndex = (callerPageNumber * callerPageSize) - numLeftToFetch;
            int sourcePageNumber = IntMath.divide(fetchFromAbsoluteIndex + 1, sourcePageSize, RoundingMode.CEILING);
            List<T> sourcePage;
            try {
                sourcePage = localCache.get(sourcePageNumber);
            }
            catch (ExecutionException exception) {
                throw Throwables.propagate(exception.getCause());
            }
            int sourcePageStartAbsoluteIndex = (sourcePageNumber - 1) * sourcePageSize;
            int addFromIndex = fetchFromAbsoluteIndex - sourcePageStartAbsoluteIndex;
            int addToIndex = Math.min(addFromIndex + numLeftToFetch, sourcePage.size());
            if (addFromIndex >= addToIndex) {
                break;
            }
            callerPage.addAll(sourcePage.subList(addFromIndex, addToIndex));
            if (sourcePageSize - sourcePage.size() > 0) {
                break; //this optimization assumes a partial page is the last one
            }
        }

        return callerPage;
    }

    @Override
    public Iterator<List<T>> iterator() {
        return new Itr();
    }

    private class Itr implements Iterator<List<T>> {

        private int currentPageNumber = 0;
        private List<T> currentPage = null;

        @Override
        public boolean hasNext() {
            initIfNeeded();
            return !currentPage.isEmpty();
        }

        @Override
        public List<T> next() {
            initIfNeeded();
            List<T> temp = currentPage;
            currentPage =
                    (currentPage.size() < callerPageSize)
                    ? Collections.<T>emptyList() //this optimization assumes a partial page is the last one
                    : getPage(++currentPageNumber);
            return temp;
        }

        @Override
        public void remove() {
            throw new UnsupportedOperationException();
        }

        private void initIfNeeded() {
            if (currentPage == null) {
                currentPage = getPage(++currentPageNumber);
            }
        }
    }

    /*** TESTING ***/

    public static void main(String[] args) {
        testHelper(10);
        testHelper(5);
        testHelper(3);
        testHelper(13);
        testHelper(58);
    }

    private static void testHelper(int callerPageSize) {
        final TestSource ts = TestSource.INSTANCE;
        PagingAdapter<String> pg = new PagingAdapter<String>(callerPageSize, ts.getPageSize()) {
            @Override
            protected List<String> fetchPageFromSource(int sourcePageNumber) {
                return ts.get(sourcePageNumber);
            }
        };
        System.out.println("caller page size = " + callerPageSize);
        int pageNum = 0;
        for (List<String> lst : pg) {
            System.out.println("page " + ++pageNum + " (" + lst.size() + "): " + lst);
        }
        System.out.println();
    }

    private static class TestSource {

        public static final TestSource INSTANCE = new TestSource();

        private final String[][] store = {
            { "1",  "2",  "3",  "4",  "5",  "6",  "7",  "8",  "9", "10"},
            {"11", "12", "13", "14", "15", "16", "17", "18", "19", "20"},
            {"21", "22", "23", "24", "25", "26", "27", "28", "29", "30"},
            {"31", "32", "33", "34", "35", "36", "37", "38", "39", "40"},
            {"41", "42", "43", "44", "45", "46", "47",                 },
        };

        private TestSource() { }

        public List<String> get(int page) {
            try {
                return Arrays.asList(store[page - 1]);
            }
            catch (ArrayIndexOutOfBoundsException aioobe) {
                return Collections.emptyList();
            }
        }

        public int getPageSize() {
            return store[0].length;
        }

        public int getNumPages() {
            return store.length;
        }
    }
}

I mainly want to make sure the getPage method is correct and optimal. The Iterator is less important. I included the "testing" section at the bottom for your convenience and as a usage example.

Note that the code is using Guava 12.

Edit: I added memoization of pages fetched from source using Guava's LoadingCache. I also declared fetchPageFromSource as throwing Exception so implementations can just let all exceptions propagate - getPage will unwrap these from ExecutionException and rethrow them using Throwables.propagate (or I might consider wrapping them in my own checked PagingException).

\$\endgroup\$
  • \$\begingroup\$ This is my first time posting on code review - let me know if/how I can improve the question at all. \$\endgroup\$ – Paul Bellora Jun 10 '12 at 6:48
  • \$\begingroup\$ I guess you should specify what you'd like to know in an answer. You tagged it with "optimization" - what do you want opimized? Readability, speed, memory consumtion? What don't you like about your own code in that regard? Right now, I have a hard time to find the right angle for an answer. \$\endgroup\$ – Arne Aug 30 '12 at 12:14
  • 1
    \$\begingroup\$ If you have unit tests (with assertions), could you include them, please? \$\endgroup\$ – palacsint Nov 19 '12 at 14:20
  • \$\begingroup\$ @palacsint Unfortunately I don't - just the cobbled psuedo-testing toward the bottom. I'll try to write unit tests when I get the chance. \$\endgroup\$ – Paul Bellora Dec 3 '12 at 2:53
  • 1
    \$\begingroup\$ @Arne Sorry for the late reply. I was mainly concerned with identifying any clever optimizations that could be done within getPage that I may have missed. I'm satisfied with the readability and memory consumption is a lesser concern. \$\endgroup\$ – Paul Bellora Dec 3 '12 at 2:58
2
\$\begingroup\$

I like your usage of Guava, and overall your code seems really good. I might actually use it myself somewhere.

The code could use some more Javadoc though, especially on the class itself and the constructor. Explaining those callerPageSize and sourcePageSize parameters is very important.

Possible extension

Currently, you leave some work for the user of your code to divide it into some fixed page size. What if that programmer is very lazy and don't want to do that? An optional constructor could be helpful to indicate that technically, you support that feature.

public PagingAdapter(int callerPageSize) {
    this(callerPageSize, 1);

However, if you add that optional constructor; the users will have to send your code a List of size one. That's not optimal. Perhaps a class to create pagination without some previously already existing pagination would be useful? (This is actually what I expected from your code when I first saw the class name of it)

A possible bug

Your variable sourcePageSize confused me a bit at first, even though it sounds like it should be equal to sourcePage.size() there is absolutely nothing that forces it to be so.

What if fetchPageFromSource returns a list that does not correspond to the sourcePageSize that the class got? I can tell you what happens:

Adding one extra string to the second page of your example causes it to be completely ignored for some callerPageSizes, and for other values of callerPageSize the first absolute index directly after is ignored.

Also, if fetchPageFromSource accidentally returns a list of size less that sourcePageSize before it actually is over, then that effectively cuts the list so that the last items does not get seen. By removing the item "20" in your example, the list always stopped at 19 for values of callerPageSize.

Possible Solution: Throw an exception if the fetchPageFromSource method returns a list of a bigger size than what was expected. Solving the problem of an incorrect smaller list is more difficult because of your optimization of smaller lists == no more items.

Even though this might be by design, it is very important to document this behavior.

Another possible problems might encounter when using your class is that if fetchPageFromSource returns null; this causes InvalidCacheLoadException (caused by Guava). This should either be fixed or documented. Returning null might be a way for a programmer to try and tell your code: "This page does not exist".

Remember that when writing code for other people to use, they might not use it in the way that is intended.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the good points. A variation of the code I posted is being used in production and has been successful - I'll make sure to review it to see if what you suggest can be applied. \$\endgroup\$ – Paul Bellora Jan 28 '14 at 1:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.