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

        Issue Links

          Activity

          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.
          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,

          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 -

          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 -

          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!

            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