search

graphhopper / geocoder-converter

Converts arbitrary geocoding responses to a GraphHopper response
https://graphhopper.com/api/1/docs/geocoding/#external-providers
Apache License 2.0
9 stars 11 forks source link

refactored by concatenating String to be appended to StringBuilder #68

Open Shivangip285 opened 2 years ago

Shivangip285 commented 2 years ago

Refactor

karussell commented 2 years ago

Thanks for your contribution. Unfortunately I do not understand the intend of the refactoring. What do you think has improved with it? Why e.g. is append replaced with String concatenation?

PS: I fixed the build in the master branch

Shivangip285 commented 2 years ago

Sure, actually I made 3 commits with following reasons.

1.

refactored by concatenating String to be appended to StringBuilder https://github.com/graphhopper/geocoder-converter/commit/ac9141fef41c43b59218f7b4388c94e5d45454a6- I had preferred concatenation of static String with dynamic String followed by append, over multiple append for code optimization purpose, if you observe those multiple appends were appending static or constant String value like ", "using append . 2.

Refactored by removing redundant variable rsp https://github.com/graphhopper/geocoder-converter/commit/5fe5127076f03162cf94a5f707e514cd42d82c67-here variable was defined and assigned and then returned in next line by function, to optimize this I had combined returned the value directly instead of assigning to a redundant variable 3.

Refactor by replacing if block with optimized Math.min() https://github.com/graphhopper/geocoder-converter/commit/76e9f6758e89d03c488f2821b155751cc0e7fc22- this is optimized replacement of 3 line of if block with Math.min().

I had started with small refactoring for code optimization and clean coding, these individual changes might not have a very high effect on performance but, collectively they will help improving performance and keeping the code clean. On Thu, Nov 3, 2022 at 10:32 PM Peter @.***> wrote:

Thanks for your contribution. Unfortunately I do not understand the intend of the refactoring. What do you think has improved with it? Why e.g. is append replaced with String concatenation?

PS: I fixed the build in the master branch

— Reply to this email directly, view it on GitHub https://github.com/graphhopper/geocoder-converter/pull/68#issuecomment-1302409880, or unsubscribe https://github.com/notifications/unsubscribe-auth/AZPVVCINMJKSGUUR4XOOBLDWGPVZPANCNFSM6AAAAAARR6NHIA . You are receiving this because you authored the thread.Message ID: @.***>

karussell commented 2 years ago

I had preferred concatenation of static String with dynamic String followed by append,

In non-performance critical code this is indeed better readable, but I would keep it as it is here. Also you shouldn't mix append and +.

collectively they will help improving performance and keeping the code clean

improving performance is only possible if you measure your results. You shouldn't change code only because you think it is faster (e.g. use JMH or apache bench). Also from my experience it does not matter if you use append or + although append was slightly faster in previous JVM versions, but I'm unsure why + should be faster. Also a variable like rsp that is immediately returned is not a performance problem.

this is optimized replacement of 3 line of if block with Math.min()

Ok