bengbengbalabalabeng commented on code in PR #942:
URL: https://github.com/apache/fesod/pull/942#discussion_r3603524964
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/handlers/CellTagHandler.java:
##########
@@ -130,10 +149,8 @@ public void endElement(XlsxReadContext xlsxReadContext,
String name) {
tempCellData.setStringValue(tempCellData.getStringValue().trim());
}
}
-
tempCellData.checkEmpty();
tempCellData.setRowIndex(xlsxReadSheetHolder.getRowIndex());
- tempCellData.setColumnIndex(xlsxReadSheetHolder.getColumnIndex());
Review Comment:
> Considering the earlier logic, assigning
`tempCellData.setColumnIndex(targetColumnIndex)` here might make the behavior
more consistent.
Sorry for the confusion, my previous comment might not have been clear
enough. Here is what I actually meant:
```java
// ...
if (includeColumnIndexes == null) {
targetColumnIndex = xlsxReadSheetHolder.getColumnIndex();
} else {
// If it's a target column, rewrite the cell's internal index and pack
it sequentially
targetColumnIndex =
includeColumnIndexes.indexOf(xlsxReadSheetHolder.getColumnIndex());
if (targetColumnIndex < 0) {
return;
}
}
// ...
// line: 136 (old 154)
tempCellData.setColumnIndex(targetColumnIndex);
```
Since we guarantee `targetColumnIndex` is resolved in both branches, placing
`setColumnIndex()` outside the block maintains the exact same logic while
avoiding code duplication. What do you think :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
