General code style refactoring

[MariaEduardaDeAzevedo]

Hello!

We are computer science students from Federal University of Campina Grande (Brazil) and, as part of the Software Engineering course, we were in charge to choosing a real open source application project to exercise what we learned in the course. My group and I choose the Retrofit knowing its extensive use in the area of development and API integration.

As result, after all the activities, we did some refactoring and added some automated tests to the repository and decided to open a PR to contribute with the Retrofit code style. All refactoring done was based on code style, readability and comprehension improvements, applying some known refactoring in the refactoring.com catalog.

[JakeWharton]

Hello. Thank you for sending the PR. I cannot accept it as-is. Frankly, almost every single commit in this PR is the exact opposite of what you should do in any project, is the opposite of what I value in the codebase, and is the opposite of how you should approach a project as a contributor. I realize this sounds quite harsh, but there's a huge teachable moment here since, after all, you are in school!

First, the generated test cases are nonsensical. Some explicitly test implicit behavior such as the ability to load the classes in the library. Others are basically empty with a single line of setup and no assertions. The tests in this library are focused on one thing: exercising the public API codepaths that a caller would use to execute an API call. Sometimes we rely on internal APIs to make the test slightly shorter, but ultimately we are still testing a codepath in a way that can be done through the public API. There is no mocking–we use a real HTTP client and real HTTP server which let us validate the behavior of both. Additionally, by virtue of (mostly) sticking to the public API we can refactor large swaths of the codebase without touching the tests (and have in the past).

Second, simply put: nothing about removing comments makes code cleaner. Comments express why code is doing something and that context is important to other developers as well as your future self.

Third, most of the refactorings are superfluous and make the code readability worse. A few of them extract all strings to constants. You'll probably here people say this is a good idea and maybe they'll also tell you about avoiding magic constants and such. Single-use strings should never be extracted to a constant unless there is an extreme need to convey semantics through a variable name. Similarly, single-use numbers follow the same pattern. Numbers are only magic when you make them magic through how they are used but they are not intrinsically magic. Numbers sometimes are just numbers.

Finally, there are myriad changes to the codestyle as well as committing unrelated files to the repo such as Eclipse project files, outputs of the automated test generator, etc. The project enforces code style through a static tool called java-code-format from Google so all the changes will actually fail the build until you let the tool reformat the code. Maybe you don't agree with how it formats everything. I sure don't! But the value of a normalized format exceeds that of individual preference. And with the committing of unrelated files, you should always review your own PRs and make sure that you are sending the minimal amount of changed lines to accomplish a task and absolutely nothing more.

I'm sorry that you spent time on doing this only to get it rejected. I wish instead that you would have studied some of the PRs which were merged or rejected in the past as it would have likely been a much more insightful look into an open source project. And only then perhaps you could have sent a PR to fix a single thing which I would have been happy to help land. With something like this that tries to do everything in a single PR the chances of them every landing in any project is, basically, zero.

[MariaEduardaDeAzevedo]

Thank you very much for the feedback, we appreciate it!
When we submitted the PR we didn't think it would be accepted, we did it aiming more for feedback from a large open-source project and to gain some experience looking at real and widely used code.
We recognize that our refactorings and the automated tests were not useful, we even had a hard time with this generation since it was a requirement on the course to use some kind of automated code tool. Still, we realized that there was no compatibility with the tests that already existed in the project. The refactorings we did were more to demonstrate in the course that we knew how to identify possible perfective design improvements in a code so that we were only required to indicate and refactor a certain amount.
We thank you very much for the time you spent on this review, and for the tips and teachings, we will take them with us in our lives and our maturing as professionals. We hope one day to be able to contribute more effectively to projects like this one!