Are you compelled to eliminate syntactic cruft?

Question

When you encounter working code that is unpleasing to the eye, how likely are you to refactor it? I know code aesthetics is mostly subjective, but I'm bothered a lot by "ugly" code and find it difficult to resist the urge to clean it up.

I don't like:

Heavy function nesting

AndFinally(AndThenDoThis(DoThis(strVar, 1, DoThat("February"))), FetchValue(0))

Any more paranthesis, commas, and ampersands than necessary

CType(ctlGrid.DataSource, DataView).RowFilter = "HourlyRate > 100.00"

and would consider adding a method (or extension method) to get

ctlGrid.DataView.RowFilter = "HourlyRate > 100.00"

alternately, I would change:

rows = tblEmp.Select("AnnualEarnings > " & dEarnings.ToString & " AND Position = '" & strPosition & "'")

to this:

rows = tblEmp.Select("AnnualEarnings > ? AND Position = ?", dEarnings, strPosition)

Now this is a bit extreme, but I will even take a common idiom, like finding the first item in an array and provide a method for it:

row = tblEmps.Select("Position = 'Manager'")(0)

becomes (and, I think, is more readable):

row = tblEmps.Select("Position = 'Manager'").First

When I see this it drives me crazy

frmMain.grdEmployees.colName.Bold = True
frmMain.grdEmployees.colName.Font = "Arial"
frmMain.grdEmployees.colName.BackColor = "lightyellow"

and, with regard only to aesthetics (not performance), I end up with:

With frmMain.grdEmployees.colName
  .Bold = True
  .Font = "Arial"
  .BackColor = "lightyellow"
End With

I could go on and on with this sort of thing. I exercise great care so that refactoring only occasionally results in introducing a bug--usually a short-lived, low-impact one.

Not everyone in our company is agreeable to this kind of thing, but I believe its a boon to long-term maintenance. I read "Don't Make Me Think" by Steve Krug a few years back. I would argue that this book (although geared toward web design) favors code written in such a way that it minimizes the mental effort necessary to digest it. In most cases, to me, this mean expressing the same code in fewer lines, but never in a tricky, obfuscating way.

How compelled are you to enhance code aesthetics? When do you leave well enough alone and what sorts of "ugliness" compels you to step in? Do you consider this practice more beneficial or more detrimental?

Answer 1

Within most organizations, there is an established guideline for style.

Answer 2

Makes sense.

I will use this as an example.
firstManagerow = tblEmps.Select("Position = 'Manager'").First

It is better to define variables at places, where it will be better to use them in debugging.

This could become a method with an appropriate descriptive name.

rows = tblEmp.Select("AnnualEarnings > " & dEarnings.ToString & " AND Position = '" & strPosition & "'")

Answer 3

I try to get rid of those ugly things aswell, resharper helps me a lot when developing .NET applications to give me hints about object initializers for example.

You can also configure your IDE to setup proper source code formatting rules which you and your team should all use. This does not eliminate situations where you want to see string formatters or maybe some extra lines but I think it will help a lot. For the rest there is resharper :).

Answer 4

When I started programming, every line of code I saw compelled me to be edited. Looking back, I wasted a lot of time on that. These days, I still fix what I consider to be ugly code, but only when it's in a file that I'm changing for practical reasons.

Avoid the temptation to edit an entire code base for stylistic reasons.

Answer 5

I'm all for code aesthetics, but the urge to change existing code, especially if it's working code, should be resisted as much as possible. Guidelines for code style should be established and enforced, but existing code should be treated with care: it is very easy to actually change how the code behaves.

Answer 6

Although I've read in quite a few books about using descriptive variable names, when I write a short function which is working with only a few variables, I usually opt for short (even generic) names. Long, descriptive names can consume a lot of visual real estate and for basic functions, I consider it overkill. If basic functions grow to encompass additional complexity and more variables are introduced, I will expand the names to improve clarity.

For i As Integer = 0 To tblEmps.Rows.Count - 1
  Console.WriteLine(tblEmps.Rows(i)("Name"))
Next

I see no reason why i should be more descriptive.

Answer 7

There are many conflicting possible answers to this stemming from various considerations, but a lot of your examples are matters of style for which opinion might differ...

• Is there an established standard? If so, is there an established culture of refactoring existing code to it, and also making sure the developer who wrote it is aware? If not, you risk offending someone else's sense of style and getting into an edit war. If there's not a preventative (in the form of standards and reviews), then there are bigger questions than whether you should "fix" it.
• Is there a repetition that suits refactoring? Introducing a method to handle a simple single case isn't always appropriate
• Are you actually working on that bit of code, or just using it? If you don't need to get your hands dirty and the code works, consider leaving it alone.
• Are you potentially writing more code and introducing more complexity for the sake of aesthetics? Introducing too many new methods can be just as confusing as a lot of nested brackets.

The team I work with has regular discussions in matters of style, and sometimes we agree on a standard, but often in matters of less importance we recognise that it's better to agree to disagree and try to be polite in avoiding drastic edits where only a difference of opinion is involved.

Ultimately the time and risk cost of all these edits adds up and it needs to be weighed up against time more productively spent.

Answer 8

To me, the most important thing is not aesthetics but whether someone else can understand the code later.

Nested function calls are typically a no-no, because the cognitive burden of understanding the whole expression becomes high. And there could be side-effects in the nested calls, which make things much worse. Therefore it is good to break the nesting and introduce temporary variables, also because you can then write comments in the code and explain what the intermediate results are about.

Introducing new notation for trivialities (like your (0) versus .First example) is not good, because it does not improve understanding: you are creating a new abstraction or concept without added value. Why write i.addOne() if you can do it with i++? Everyone knows what "++" is about, but they could not know what addOne() actually does without reading its documentation. So it's just a way to add cognitive burden.

You refer to refactoring, but refactoring does not mean just changing code layout and aesthetics, but also structural changes to the whole program. Continuing with your example, nested function calls f(x(y(z(q)))) can signal a need to do refactoring that is beyond aesthetics: figuring out why the nested calls are needed, and then packing them into a new abstraction. Of course refactoring could be also about changing the complete object model, a major effort.

Answer 9

Only refactor if you need to assuming this is a commercial venture. Your Heavy Function Nesting example would be where when debugging breaking into multiple lines makes life a lot easier. Getting rid of parenthesis is a lot easier to make mistakes. Yeah it might look better but the good really isn't any more readable/better. It is just more in the style you are used to seeing.

Answer 10

I sense two separate questions here:

1. Should code be refactored to improve clarity?
2. Should code be refactored for no other reason than to improve clarity?

In my opinion, the answer to the first question is usually "yes". All code should, ideally, be more-or-less self-documenting. Less time spent trying to understand code means more time is available to spend fixing it when it goes wrong. When you're diving into code to fix something, it can absolutely be worth the effort to clarify anything which made you scratch your head.

But that leads us to that subtly-dangerous second question. "When you're diving into code to fix something" is by far the more significant part of that statement. If you have no reason to be changing something other than to make it easier to read, then for goodness' sake, close your editor!

Premature refactorization should be treated much like premature optimization, and for the same reasons. When it comes down to it, bugs come from code. The more coding you do, the more bugs you'll generate. If you have a piece of code which (to the best of your knowledge) doesn't contain any bugs, there's nowhere to go but downhill. You may not introduce any, but you certainly won't if you don't touch it! By refactoring, clarifying, rearranging, or otherwise fiddling with code that's currently working as intended, you're doing yourself (and your employer) a huge disservice by incurring a guaranteed risk for a theoretical payoff.

If you're in already in there for another reason, go ahead and make it easier for yourself or whoever wanders that path after you. But never refactor working code, even if it'll look prettier afterward.

Answer 11

You can make "heavy function nesting" much better just by reformatting, lining up the parts of same nesting level on the same column, e.g.:

AndFinally (AndThenDoThis
             (DoThis (strVar,
                      1,
                      DoThat("February"))),
            FetchValue(0))

That way, the "cleanup" cannot cause bugs (unless your language's syntax is whitespace sensitive), and the whole thing's flow really becomes clear.

Answer 12

Beauty is in the eye of the beholder.

I'm sure there are people that wwill refactor your code because they think it's not pretty. What will you do then? Refactor it again? You'll end up in a vicious circle.

So please resist the temptation to refactor code just because your code is prettier.

Answer 13

I had a co-worker (no longer with us) that continually 'fixed' ugly code. Problem was that invariably he broke it, and invariably he made it ugly in some other way. One day a class would have a hand full of methods 10 or 20 lines long, the next day it would have 30 methods each one or two lines long, and at least one of the classes behaviours would be bust.

If it aint broke don't fix it.