Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring XSSFRow #949

Merged
merged 5 commits into from
Nov 26, 2022
Merged

Conversation

artem-iron
Copy link
Contributor

Code clean-up, refactor into more of C# style code, use expression body style for properties and methods where applicable, use 'var' instead of explicit types where possible, add curly brackets for clarity in places where they were omitted, re-arrange members into regions.

Change javadoc to c# xml documentation.

No changes in code itself or logic, only refactoring.

@PBrunot
Copy link
Contributor

PBrunot commented Nov 5, 2022

I find your version much easier to read.
I had a doubt about the usage of => with older versions of the framework supported by NPOI ( .net framework / C# version 7.3 ).
This feature was added in C# 7.0 (source: https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/statements-expressions-operators/expression-bodied-members)
So there is no language compatibility issue.
I suppose the appveyor should have caught it anyways, but better safe than sorry.

@artem-iron
Copy link
Contributor Author

@PBrunot I'm a bit conflicted on the use of expression body myself, it makes code a little less cluttered, but does it help readability or hurts it? I'm not sure, to me it seems like neither. But since it does reduce clutter - decided to roll with it.

The language support, of course, is a legitimate issue, but I test the code on multiple targets including .net framework 4.6.2, and no language compatibility issues were raised while testing. Anyway, not married to the idea, let's see what @tonyqus (and/or others) think and if need be, I'll switch it back to full method bodies.

@tonyqus
Copy link
Member

tonyqus commented Nov 8, 2022

To be honest, I'm not a fans of expression body and var. NPOI used to support very old .NET framework (such as 2.0 and 3.x). I have a habit to avoid using new C# syntax feature since it may break the compilation.

I'm fine with the following changes:

  • Convert javadoc documentation to csharp xml documentation in XSSFRow
  • Re-arrange the members of XSSFRow.

Brings back block bodies for methods and properties;
Brings back use of explicit types instead of 'var'
@artem-iron
Copy link
Contributor Author

@tonyqus I've addressed your comments in the recent commit. Thanks for the input!

@tonyqus tonyqus added this to the NPOI 2.6.1 milestone Nov 15, 2022
@tonyqus
Copy link
Member

tonyqus commented Nov 26, 2022

LGTM

@tonyqus tonyqus merged commit b9b3f7c into nissl-lab:master Nov 26, 2022
@artem-iron artem-iron deleted the refactoring-xssfrow branch November 28, 2022 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants