Yes, but when the guy pointing ugly code is a good coder, "ugly" is just a shortcut for something that usually can be explicitly stated and is really harmful. (A bad coder will say "ugly" for any kind of code he did not write.) Here are some of the reasons behind "ugliness":
- Wrong logic giving correct results (this is awful)
- Hidden dependencies making remote changes dangerous (bèè)
- Misleading variable or function name
- Unnecessary use of magic (eg programmatically building function names), this breaks a lot of code analyser, and makes your code base ungreppable, which is very bad.
- Code duplication and configuration factorization.
- Nasty hack breaking the model (eg changing request parameters on the fly)
All in all, I agree with the OP premices, but not with his solution. Adding comments all over the code is not going to save you, and making sure you understand all corners of the module you work on before starting to modify a comma is not that helpful either. In front of a montain of legacy code that need some ordering, if it is a short cycles mode with versionning, I would:
- First make sure my tools are accurate and fast. It mostly means tweaking my grep alias, so I grep all the code and nothing more. I may mean to find a way to run the modified code in less that 3 secs (unit tests can help for that part).
- Second, don't try to understand all the thing, start low, maybe rename a few variables that are obviously wrong, for example marking them as private (adding _ prefix in python).
- You won't know were are the painful points in the begining, so you should continue doing safe & simple refactorings, one step at a time, in order to slowly "wake up" the code. A bit of temporay commenting can help, like "this func returns a x-blob used by that module".
- Then when you become familiar with the code you'll be in position to decide if and where to do some real changes. Some people here seem to believe that legacy code should not be changed except in case of earthquake. I don't agree. Adding a pile of hacky crap over a pile of hacky crap to make the thing temporary work is not a good bet on the future: do it only if you plan to leave your company soon enough.
- Doing a bigger change might require some iterations and lacemaker work. If possible, you can bridge the legacy code, ie write a func_new and compare it's result with func_old during one iteration, to kinda prove that they are the same.
- I noticed that it is sometime useful to take a big class, a big god-like object doing too much, and cut it in much smaller pieces. Sometime you may have to re-glue together some pieces because you went too far, sometime you may even kind-of rollback your changes, but in any case these operations will shaken the code base a bit, which is good.
These operations require courage regarding of the code base, because you will feel lost sometime, you will fear regressions, you may even fix a functionality that should not be working and, consequentially, break something. Here, be careful, you should not be fixing broken parts and refactor in the same operations, except if you know enough to be sure. One purpose of refactoring is to allow you to fix broken (or slow) part later. I insist on "later".
It is also requiring patience and tenacity in front of the operational team: they will think you are doing nothing (they are right, from their point of view, per the very definition of refactoring).