Prefer ArrayList over LinkedList and native array#1334
Prefer ArrayList over LinkedList and native array#1334belugabehr wants to merge 2 commits intoapache:masterfrom
Conversation
ctubbsii
left a comment
ctubbsii
left a comment
There was a problem hiding this comment.
Nice changes. The LinkedList was probably preferable for the deepCopies, though.
|
|
||
| List<LocalityGroupReader> readers = (useSample) ? r.sampleReaders : r.readers; | ||
| for (LocalityGroupReader reader : readers) { | ||
| LocalityGroupReader newReader = new LocalityGroupReader(reader); |
There was a problem hiding this comment.
Good opportunity to use var if one were inclined.
| LocalityGroupReader newReader = new LocalityGroupReader(reader); | |
| var newReader = new LocalityGroupReader(reader); |
| } | ||
|
|
||
| if (deepCopies.size() != 0) | ||
| if (!deepCopies.isEmpty()) { |
There was a problem hiding this comment.
Unrelated changes, but nice changes to see anyway. I sometimes have my IDE automatically add missing braces for single-statement blocks, but the downside is that they can clutter the review (same with the unrelated change to convert this to isEmpty()). I don't think there' a problem here... the changes in this PR are simple enough to understand, and these are nice changes to see.
| } | ||
|
|
||
| @Deprecated | ||
| public LocalityGroupContext(LocalityGroup[] groups) { |
There was a problem hiding this comment.
This isn't public API. It's internal only, so if we're not using it internally, we don't need to keep it around deprecated for any user code. It can just be deleted outright.
| currentReaders = new ArrayList<>(size); | ||
|
|
||
| deepCopies = new LinkedList<>(); | ||
| deepCopies = new ArrayList<>(); |
There was a problem hiding this comment.
We only ever do add() on this, which can cause problems on certain system patterns which use a lot of deepCopies, if it results in array resizing. LinkedList is probably better here for those use cases, and not worse for other use cases. I'd leave this one the way it was.
| this.currentReaders[i].setInterruptFlag(r.interruptFlag); | ||
| } | ||
|
|
||
| List<LocalityGroupReader> readers = (useSample) ? r.sampleReaders : r.readers; |
There was a problem hiding this comment.
This is a nice improvement to simplify the following loop.
| public LocalityGroupIterator(LocalityGroup[] groups) { | ||
| super(groups.length); | ||
| this.lgContext = new LocalityGroupContext(groups); | ||
| this.lgContext = new LocalityGroupContext(Arrays.asList(groups)); |
There was a problem hiding this comment.
Since this system iterator is also not public API, I wonder if there's something we can do here to pass this in as a list initially, rather than as an array that we have to convert. This could also be done in subsequent work. What do you think, @belugabehr ? Is it worth going down the rabbit hole a bit further on this one, or doing in a future change?
|
There has been no response in almost 11 months since the last code review feedback, and the PR now has merge conflicts. I'm closing this for now. Please feel free to re-open or re-submit against the main branch if there is still an interest in making these changes. |
2 participants
No description provided.