"Cosmetic" cleaning of an old, unknown code. What steps, what order? How invasive?

Question

When I get code that I haven’t seen before to reorganize it into some kind of normal state, I usually fix cosmetic things (for example, converting StringTokenizers to String#split() , replacing the pre-1.2 collections with new collections, making the fields final , converting C-style arrays to Java-style arrays, ...) by reading the source code I need to familiarize myself with.

How many people use this strategy (maybe this is some kind of "best practice" that I don’t know?), Or is it considered too dangerous, and not touch the old code if it is not absolutely necessary, usually preferable? Or is the combination of "cosmetic cleaning" more common with the more invasive "general refactoring" step?

What are the common “low hanging fruits” when performing “cosmetic cleaning” (versus refactoring with more invasive changes)?

Answer 1

In my opinion, the general refactoring is "cosmetic cleaning" . You simply change the code to make it more understandable without changing its behavior.

I always refactor, first attacking minor changes. The more readable you can quickly make code, the easier it will be to make structural changes later - especially since it helps you look for duplicate code, etc.

I usually start by looking at the code that is often used, and it will need to be changed often. (This has the biggest impact in the least amount of time ...) Variable naming is perhaps the easiest and safest “low hanging fruit” to attack first, followed by frame updates (collection changes, updated methods, etc.). Once this is done, separating the larger methods is usually my next step, followed by other typical refactorings.

Answer 2

There is no right or wrong answer, since it largely depends on the circumstances.

If the code works live, works, is not documented, and does not contain a testing infrastructure, I would not touch it. If someone comes back in the future and wants new functions, I will try to use them in existing code, changing as little as possible.

If the code is faulty, problematic, missing functions, and was written by a programmer who no longer works with the company, I would probably redesign and rewrite all of this. I could always refer to this programmer code for a specific solution to a specific problem, but it would help me reorganize everything in my mind and in the source. In this situation, all this is probably poorly designed, and it can completely rethink.

For everything that was between them, I would stop on your approach. I would start by cleaning everything cosmetically so that I can see what is happening. Then I would start working on what code stood out, since it needed to work the most. I would add documentation as I understand how it works, so that I can help remember what is happening.

Ultimately, remember that if you are going to maintain code now, this should be in accordance with your standards. Where this is not the case, you should take the time to bring it to your standards - no matter what it costs. This will save you a lot of time, effort and frustration in the future.

Answer 3

The lowest hanging cosmetic fruit (in Eclipse, anyway) is shift-control-F. Auto formatting is your friend.

Answer 4

The first thing I do is hide most of the things from the outside world. If the code is crappy most of the time, the guy who implemented it did not know much about hiding data and the like.

So my advice, the first thing to do:

Turn as many members and methods as you can without breaking the compilation.

As a second step, I'm trying to define interfaces. I am replacing concrete classes through interfaces in all methods of related classes. This way you unravel the classes a bit.

Further refactoring can be done more safely and locally.

Answer 5

You can buy a copy of Refactoring: improving the design of existing code from Martin Fowler, you will find many things that you can do during a refactoring operation.

In addition, you can use the tools provided by your IDE and other code analyzers, such as Findbugs or PMD to detect problems in your code.

---

Resources:

• www.refactoring.com
• wikipedia - List of tools for analyzing static code in java

In the same topic:

• How do you reorganize a big dirty codebase?
• Code Analyzers: PMD and FindBugs

Answer 6

You are on the right track. Having made minor corrections, you will be more familiar with the code, and larger corrections will be easier to do with all detritus aside.

Run a tool, such as JDepend , CheckStyle, or PMD in the source. They can automatically perform many changes, which are cosmetological, but based on general refactoring rules.

Answer 7

Starting with “cosmetic cleaning,” you get a good overview of how messy the code is, and this, combined with better readability, is a good start.

I always (yes, right ... sometimes there is something called the deadline that is messing with me) start with this approach, and it still helped me a lot.

Answer 8

I do not change the old code, except to reformat it using the IDE. There is too much risk of introducing an error - or deleting an error, on which another code now depends! Or introduce a dependency that does not exist, for example, using a heap instead of a stack.

Besides IDE reform, I am not modifying the code that the boss did not ask me to change. If something is egregious, I ask the boss if I can make changes and indicate the case why this is good for the company.

If the boss asks me to correct the error in the code, I will make as few changes as possible. Let's say the error is in a simple loop. I would reorganize the loop into a new method. Then I will write a test case for this method to demonstrate that I found an error. Then I will fix the new method. Then I would make sure the test cases pass.

Yes, I am a contractor. The contract gives you a different perspective. I recommend it.

Answer 9

There is one thing you should be aware of. The code you started with was TESTED and approved, and your changes automatically mean that a re-check should happen, because you may have inadvertently violated any behavior elsewhere.

In addition, everyone makes mistakes. Every non-zero change you make (changing the StringTokenizer to split is not an automatic function, like Eclipse, so you write it yourself) is the possibility of creep errors. Do you get the exact right to conduct the conditional expression or do you forget by a simple mistake ! ?

Therefore, your changes involve re-testing. This work can be quite substantial and greatly suppress the small changes that you have made.

Answer 10

I usually don’t understand the old code that is looking for problems. However, if I read it, as you seem to be doing, and it makes my brain fail, I correct it.

Ordinary low-hanging fruits for me, as a rule, are more associated with renaming classes, methods, fields, etc. and writing behavioral examples (like unit tests) when I cannot be sure what the class does by checking - usually making the code more readable when I read it. None of this is what I would call “invasive,” but they are more than just cosmetic.

Answer 11

From experience, this depends on two factors: time and risk.

If you have a lot of time, you can do much more, if not then, the amount of changes that you make is accordingly reduced. As much as I hate doing this, I had to create some terrible shameful hacks, because I just did not have enough time to do it right ...

If the code you are working on has many dependencies or is crucial for the application, make as few changes as possible - you never know what your fix can fix ... :)

It looks like you have a clear idea of ​​what it should look like , therefore, I will not say what specific changes to make in which order, because it will vary from person to person. First make small localized changes, check, expand the scope of your changes, check. Expand Test. Expand Test. Until you run out of time or there is no longer room for improvement!

BTW When testing, you will most likely see where things often break - create test cases for them (JUnit or something else).

EXCEPTION: Two things that I always find are reformatting (CTRL + SHFT + F in Eclipse) and a comment that is not obvious. After that, I first hammer the most obvious nail ...