Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: Trunk
    • Fix Version/s: Upcoming Release
    • Component/s: framework
    • Labels:
      None

      Description

      The EntityListIterator contains duplicate code and has need for refactoring in general.

      1. OFBIZ-9549.patch
        35 kB
        Tobias Laufkötter
      2. OFBIZ-9549.patch
        34 kB
        Tobias Laufkötter

        Activity

        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Hi Tobias,

        I reviewed your patch, I globally like it. I have few questions and opinions.

        1. Why do you prefer a return to an else in close() (I see why in absolute() and hasNext()) ?
        2. Please explain
          if (rowNum == 0 && resultSet.getType() != ResultSet.TYPE_FORWARD_ONLY) {
          
        3. While at it I'd prefer this formatting for this note
               * PLEASE NOTE: Because of the nature of the JDBC ResultSet interface this method can be very inefficient
               *  It is much better to just use next() until it returns null
               *  For example, you could use the following to iterate through the results in an EntityListIterator:
          

          Same in hasPrevious()

        4. I'd keep the comment
                          // do a quick game to see if the resultSet is empty:
                          // if we are not in the last or afterLast positions and we haven't made any values yet, the result set is empty so return false
          

          in hasPrevious()

        5. Why did you prefer an ArrayList vs a LinkedList in getCompleteList() and getPartialList(). Did you make some performance reviews using a profiler?

        Thanks!

        Show
        jacques.le.roux Jacques Le Roux added a comment - Hi Tobias, I reviewed your patch, I globally like it. I have few questions and opinions. Why do you prefer a return to an else in close() (I see why in absolute() and hasNext()) ? Please explain if (rowNum == 0 && resultSet.getType() != ResultSet.TYPE_FORWARD_ONLY) { While at it I'd prefer this formatting for this note * PLEASE NOTE: Because of the nature of the JDBC ResultSet interface this method can be very inefficient * It is much better to just use next() until it returns null * For example, you could use the following to iterate through the results in an EntityListIterator: Same in hasPrevious() I'd keep the comment // do a quick game to see if the resultSet is empty: // if we are not in the last or afterLast positions and we haven't made any values yet, the result set is empty so return false in hasPrevious() Why did you prefer an ArrayList vs a LinkedList in getCompleteList() and getPartialList(). Did you make some performance reviews using a profiler? Thanks!
        Hide
        tlaufkoetter Tobias Laufkötter added a comment -

        Hi Jacques,

        thank you for the review.

        1. I opted for the return instead of else to reduce the code complexity. There is nothing to do after the else, so there is no reason to introduce a new block and an additional level of indentation, which makes the code harder to read. Testing for the "easy" cases at the beginning and returning right after is a popular and (in my opinion) preferable code style. Now that I think about it again, I'd also like to return from the second if-block.
        2. I included this, because the called function doesn't like a forward only result set. I now realize, that the alternative doesn't like it either and that the thrown exception is handled properly. I'll just take it out.
        3. I'll apply your formatting.
        4. I'll take it back in.
        5. Actually, I'm not quite sure what would be preferrable in this case. The general use case in these methods is to get the results and filter or display them. In each case the resulting list would be iterated and not modified during the iteration. An ArrayList allocates less memory, since we know the size of the resulting list and don't need additions beyond the initial size. So performance wise, this should be the way to go. For an empty list, the LinkedList actually leaves a smaller memory footprint, so I would chose that one for the empty list use cases. I would leave this open to discussion, since I'm not an expert and need to trust what StackOverflow tells me.
        Show
        tlaufkoetter Tobias Laufkötter added a comment - Hi Jacques, thank you for the review. I opted for the return instead of else to reduce the code complexity. There is nothing to do after the else , so there is no reason to introduce a new block and an additional level of indentation, which makes the code harder to read. Testing for the "easy" cases at the beginning and returning right after is a popular and (in my opinion) preferable code style. Now that I think about it again, I'd also like to return from the second if -block. I included this, because the called function doesn't like a forward only result set. I now realize, that the alternative doesn't like it either and that the thrown exception is handled properly. I'll just take it out. I'll apply your formatting. I'll take it back in. Actually, I'm not quite sure what would be preferrable in this case. The general use case in these methods is to get the results and filter or display them. In each case the resulting list would be iterated and not modified during the iteration. An ArrayList allocates less memory, since we know the size of the resulting list and don't need additions beyond the initial size. So performance wise, this should be the way to go. For an empty list, the LinkedList actually leaves a smaller memory footprint, so I would chose that one for the empty list use cases. I would leave this open to discussion, since I'm not an expert and need to trust what StackOverflow tells me.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Thanks Tobias,

        1. OK, I see your point
        5. Right, I think we can let it as is. An emtpy ArrayList is not an issue.

        Waiting for your new patch to commit

        Show
        jacques.le.roux Jacques Le Roux added a comment - Thanks Tobias, 1. OK, I see your point 5. Right, I think we can let it as is. An emtpy ArrayList is not an issue. Waiting for your new patch to commit
        Hide
        tlaufkoetter Tobias Laufkötter added a comment -

        I fixed 2 - 4 and unified the used list implementation.

        Show
        tlaufkoetter Tobias Laufkötter added a comment - I fixed 2 - 4 and unified the used list implementation.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Thanks Tobias,

        Your slightly modified patch is in trunk at r1804328

        I simply cutted some long lines in comments and such.

        Show
        jacques.le.roux Jacques Le Roux added a comment - Thanks Tobias, Your slightly modified patch is in trunk at r1804328 I simply cutted some long lines in comments and such.

          People

          • Assignee:
            jacques.le.roux Jacques Le Roux
            Reporter:
            tlaufkoetter Tobias Laufkötter
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development