search

blackrock / protoc-jar-maven-plugin

Maven plugin to compile protobuf proto files into java source code
Apache License 2.0
9 stars 3 forks source link

Code improvement in - PlatformDetector.java #19

Open akshayvenkatesan opened 5 months ago

akshayvenkatesan commented 5 months ago

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 commented 5 months ago

@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 commented 2 months ago

@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