3
\$\begingroup\$
  • code feature: read a list of value display it depends on pagesize and current page
  • original code:
public static <T> List<T> pageBySubList(List<T> list, int pagesize, int currentPage) {
        int totalcount = list.size();
        if (totalcount == 0) {
            return new ArrayList<>();
        }
        int pagecount = 0;
        List<T> subList;
        int m = totalcount % pagesize;
        if (m > 0) {
            pagecount = totalcount / pagesize + 1;
        } else {
            pagecount = totalcount / pagesize;
        }
        if (m == 0) {
            subList = list.subList((currentPage - 1) * pagesize, pagesize * (currentPage));
        } else {
            if (currentPage == pagecount) {
                subList = list.subList((currentPage - 1) * pagesize, totalcount);
            } else {
                subList = list.subList((currentPage - 1) * pagesize, pagesize * (currentPage));
            }
        }
        return subList;
    }
  • first try:
  public static <T> List<T> pageBySubList(List<T> list, int pageSize, int currentPage) {
      if (list.isEmpty()) {
          return new ArrayList<>();
      }
      int pageCount = list.size() / pageSize + list.size() % pageSize > 0 ?  1 : 0;
      boolean isLastPage = list.size() % pageSize != 0 && currentPage == pageCount;
      return list.subList((currentPage - 1) * pageSize, isLastPage ? list.size() : pageSize * (currentPage));
  }
  • second try:
public static <T> List<T> pageBySubList(List<T> list, int pageSize, int currentPage) {
      if (list.isEmpty()) {
          return new ArrayList<>();
      }
      int pageCount = list.size() / pageSize + list.size() % pageSize > 0 ?  1 : 0;
      boolean isLastPage = list.size() % pageSize != 0 && currentPage == pageCount;
      int pageStartLine = (currentPage - 1) * pageSize;
      int pageLastLine = isLastPage ? list.size() : pageSize * (currentPage);
      return list.subList(pageStartLine, pageLastLine);
  }
  • one-line try, I think this one is the worst one which is worse than original one:
public static <T> List<T> pageBySubList(List<T> list, int pageSize, int currentPage) {
     return list.isEmpty() ? new ArrayList<>() : list.subList((currentPage - 1) * pageSize, (list.size() % pageSize != 0 && currentPage == (list.size() / pageSize + list.size() % pageSize > 0 ?  1 : 0)) ? list.size() : pageSize * (currentPage));
 }

question:

  • Which one is better in maintainability and readability? In short, Temporary variables amount vs Readability
  • Any better way to achieve it?
\$\endgroup\$
1
  • 2
    \$\begingroup\$ I have rolled back your last edit. Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Heslacher
    Jan 15 at 7:00

1 Answer 1

4
\$\begingroup\$

meaningful identifier

public ... pageBySubList(List<T> list, ...) {

That final identifier there, list, is just terrible. Tell me what's in the list, tell me about its content. From reading the signature I already know that list is a List, so the name isn't being helpful.

OTOH isLastPage is a terrific identifier, very helpful. The pageStartLine and pageLastLine names are nice, also.

documentation

This function promises to do something. But you don't spell out what that is, you don't tell us about the post-condition. There's no /** javadoc */, and the English phrase "page by sub-list" is not sufficiently descriptive.

Let's put it this way. Suppose I gave you specs for the three input parameters, and an hour to write some code, and I asked you to "please write a page by sub-list function", which I'm going to examine for correctness and run tests against. Would that be enough of a specification for you to code it up, and be confident it would pass my functional tests?

We're looking for a sentence of javadoc which would allow you to successfully accomplish that task.

parallel structure

  int pageStartLine =                           (currentPage - 1) * pageSize;
  int pageLastLine = isLastPage ? list.size() : pageSize * (currentPage);

nit: Please use the same order for factors in both assignments, with pageSize appearing first.

Yes, I know, multiplication commutes, and the machine doesn't care. But people do. You want to emphasize the similarity between what is being computed.

level of abstraction

I agree with you that first and second try are definitely better than the original, and that the one-liner is a non-starter.

It's a bit of a toss-up between the two, as either version would make it past PR code review and get merged down to main. I'm going to vote for the second try. It introduces a pair of temp vars, that is, it names intermediate expressions, and in doing that it reveals higher level business concepts, reveals what those expressions mean.

Ship it!

\$\endgroup\$
1
  • \$\begingroup\$ great answer! Sry I just miss copy-paste the javdoc and fix it a little by your advice. I'll accept your answer in a few days if no better answer. \$\endgroup\$ Jan 15 at 7:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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