[jira] [Commented] (OFBIZ-9549) Refactor EntityListIterator
[ https://issues.apache.org/jira/browse/OFBIZ-9549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16116317#comment-16116317 ] Jacques Le Roux commented on OFBIZ-9549: 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 > Refactor EntityListIterator > --- > > Key: OFBIZ-9549 > URL: https://issues.apache.org/jira/browse/OFBIZ-9549 > Project: OFBiz > Issue Type: Improvement > Components: framework >Affects Versions: Trunk >Reporter: Tobias Laufkötter >Assignee: Jacques Le Roux >Priority: Minor > Attachments: OFBIZ-9549.patch > > > The EntityListIterator contains duplicate code and has need for refactoring > in general. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (OFBIZ-9549) Refactor EntityListIterator
[
https://issues.apache.org/jira/browse/OFBIZ-9549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16116266#comment-16116266
]
Tobias Laufkötter commented on OFBIZ-9549:
--
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.
> Refactor EntityListIterator
> ---
>
> Key: OFBIZ-9549
> URL: https://issues.apache.org/jira/browse/OFBIZ-9549
> Project: OFBiz
> Issue Type: Improvement
> Components: framework
>Affects Versions: Trunk
>Reporter: Tobias Laufkötter
>Assignee: Jacques Le Roux
>Priority: Minor
> Attachments: OFBIZ-9549.patch
>
>
> The EntityListIterator contains duplicate code and has need for refactoring
> in general.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (OFBIZ-9549) Refactor EntityListIterator
[
https://issues.apache.org/jira/browse/OFBIZ-9549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16114845#comment-16114845
]
Jacques Le Roux commented on OFBIZ-9549:
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
{code}
if (rowNum == 0 && resultSet.getType() != ResultSet.TYPE_FORWARD_ONLY) {
{code}
# While at it I'd prefer this formatting for this note
{code}
* 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:
{code}
Same in hasPrevious()
# I'd keep the comment
{code}
// 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
{code}
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!
> Refactor EntityListIterator
> ---
>
> Key: OFBIZ-9549
> URL: https://issues.apache.org/jira/browse/OFBIZ-9549
> Project: OFBiz
> Issue Type: Improvement
> Components: framework
>Affects Versions: Trunk
>Reporter: Tobias Laufkötter
>Assignee: Jacques Le Roux
>Priority: Minor
> Attachments: OFBIZ-9549.patch
>
>
> The EntityListIterator contains duplicate code and has need for refactoring
> in general.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
