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

Code improvement in - PlatformDetector.java #19

Open
akshayvenkatesan opened this issue Apr 15, 2024 · 2 comments
Open

Code improvement in - PlatformDetector.java #19

akshayvenkatesan opened this issue Apr 15, 2024 · 2 comments

Comments

@akshayvenkatesan
Copy link

Is your feature request related to a problem? Please describe.
This particular utility code is not causing any problems but from a readability standpoint, it is much better to maintain a mapping rather than so many if-else conditions in normalizeOs

Describe the solution you'd like
A mapping can be maintained instead

Describe alternatives you've considered
Switch case is another alternative but mapping is better

@spensonshih
Copy link
Contributor

@akshayvenkatesan , agree 100%. This will be a nice refactoring to make the code more readable.

I’m thinking you could create some kind of List of Predicates with corresponding os value, iterate and test each predicate to find the first match.

Same can be applied to normalizeArch method.

Please feel free to contribute and raise a pull-request. Thank you in advance.

@ee08b397
Copy link

@akshayvenkatesan Quick question, how do you plan to refactor String.startsWith with switch? I thought Java 14+ could do case String s when s.startsWith("aix") -> { but it would elevate the minimum required Java version.

https://github.com/blackrock/protoc-jar-maven-plugin/blob/main/protoc-jar/src/main/java/io/github/blackrock/protocjar/PlatformDetector.java#L143

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

No branches or pull requests

3 participants