Dealing with some grey code style issues

Question

We have an intermediate developer who is really good at what he does, but there is one rough edge to this diamond. He's really insistent that every method have only one entry and one exit point.

The approach that I'm taking is to not make too big a deal out of it for code that he writes (except when there is a serious clarity issue). What bothers me is that he starts to refactor other's code so that it only has one entry and exit point. This is code that has already been tested (but not always with automated tests), so there is risk involved.

I am a senior developer in the team, so I have the authority to define the rules as far as the code base goes. But what would be the correct path to follow here? Should I let him continue to refactor other's code like this? If not, what is the best way of approaching the situation?

Answer 1

If you are the man that define the rules and you believe it doesn't make sense, just tell him that he is not paid to refactor the code in HIS way but to complete a project.

Answer 2

Allowing people to go through others code just to reformat when real performance is not an issue is a good way to ruin team relationships. Only change other's code when there is a compelling reason to do so, or if the group dynamic is very open to it.

Answer 3

In short, no, unless you have explicit rules against that in your coding standard (you have one, right?). Changing code for the sake of it is just asking for trouble (and tensions in the team).

First of all, I'd discuss this with the developer, and bring out examples of such changes that caused major problems in the past.

Also, this is one case where code reviews would probably help. If that developer needs to actually post a code review and make this change explicit to the rest of the team, chances are he won't bother, or the code will be rejected by his peers.

If code reviews are not a possibility, you could decide to enforce code ownership, in the sense that you need to consult with a module's main developer if you're to change things in the code base.

Answer 4

Ask if he allows for others to have a difference in approach. If he makes that concession you can simply point out for every line he refactors someone may come along and revert the code back exactly the way it was because their opinion differed. Thus ensuring a great big infinite loop which we all know is bad! :)

Answer 5

Tell him that a mandate is to keep code changes to a minimum. You shouldn't be changing code that's not pertinent to the task at hand. The top reasons being:

1. To reduce risk -- "Hey, it's you who's accountable if anything breaks."
2. Reviewing the code will be quicker since it allows the reviewers just to focus on the code that is actually being added/changed.

Answer 6

Make him roll through a code review for everything he changes and everything it touches. I do not like the idea of using code review as punishment but he has to understand the depth of what he is doing.

Then I would make him sit with QA and explain to them how he changed far more functionality than he needed to. After they are done beating on him for a while. Then take him to the project manager and have that person explain how he is effecting the schedule.

If that doesn't work, beat him until he cries.

Answer 7

I'm a bit confused. Does that mean he puts a try {} catch (...){} (or your language's equivalent) around all the code in every method? If not, then exceptions would count as an alternate path out of the method.

And frankly, that's just what they are designed to do. Otherwise, a simple exit code would serve the same purpose.

If it is really bugging people, it might be worth routinely backing out any changes he makes that don't have any functional impact. If you aren't into passive-agressive behavior, put it on his performance review. It may sound a bit ticky-tack, but I've had sillier stuff put on mine.

Answer 8

other's code

If you believe in community code ownership, there is no such thing.

grey code style

In the absence of a community coding rule, I will/must develop my own coding rule. I don't expect others to come to the same decision. I will refactor code I'm working on to comply with my rule, if I notice it and if the refactoring is cheap to perform.

---

There's two costs to my personal rule.

"My time"

I could be making progress on some other programming task. If I don't have another programming task, then my time was free and this is just good work.

"Code base oscillation"

If others are refactoring my refactorings, then there is a clear need for a community coding rule. The rule could be as simple as: both ways are accepted - don't refactor this scenario. This statement must be made explicitly, or it will be un-enforceable.

---

This is code that has already been tested (but not always with automated tests).

Aha, a programming task. This should get assigned.

Answer 9

I've been in that particular situation before, although I think your intermediate programmer is probably the exception, rather than the norm- the problem is most programmers won't touch other code unless they have to.

Chances are, if your intermediate guy is doing this, it's bitten him before, and he knows that its probably not the best idea to change code on principle. I would suggest a firm but open discussion about why he's making those changes in the first place. Chances are, if you present your arguments clearly but un-forcefully, he'll realize the detriment he may cause.

The one thing that I would be careful of is stifling this guy's enthusiasm for writing good code. He's obviously doing this because he thinks it will improve your code base. Like I said, I think his attitude is the exception rather than the norm, and its refreshing to hear of others who want to pro-actively improve their software. (I do understand the risk associated with it, but it sounds like something that can be greatly reduced with some more automated testing)

Hope that helps!

Answer 10

Since it bothers you (and you're in charge), you should at least talk to him about it. Since you have a solid reason to be bothered by it (it could break things), you probably want to tell him not to do that. Bonus points if you can get him to actually understand why other people don't mind multiple entry/exit points, and might actually like them.

Answer 11

Who is driving the boat in your shop? Somebody better be. If it is you, then shoot straight with this fine fellow and explain to him how things work in your shop. Likely there is no real benefit to what is essentially amounting to busy work. One can argue about multiple exits from a method versus a single exit, but the point is that if he's allowed to change what he wants, you've got a serious process issue there.

Definitely a 'people' issue that can get solved quickly by telling him how things work. No need to be nasty, just honest.

Besides, who's got time to mess with other people's code? He obviously isn't being fully tasked. :)

Answer 12

You may want to point it out, but be careful how you phrase it. Every time you pull the "I'm right because I'm the senior person," you lose credability and you're going to kill morale. It doesn't matter if your right, it won't be perceived that way.

He probably shouldn't refactor/reformat code that isn't being actively tested, but at the same point in time, if you treat him like he can't do anything right, then most likely he'll just end up quitting. In the end, that will most likely cost the company a lot more than a small amount of extra qa time (especially if he really is cleaning up bad code). I'm not saying that you shouldn't mention it to him just try and see if from his point of view and get him to meet you half way in what you're saying.

Answer 13

I've been in similar situations to this twice before.

In the first case, we had one guy who had a different sense of the One True Brace Style than the rest of the team and he changed it as he was reading code. Most of us either ignored it or put it back. In his case, he was too good to give him flack over something so minor. Not a huge deal, just annoying.

In the second case, we had one engineer who worked remotely in a different time zone and he was literally like the shoemaker's elves. Things that were half implemented with a pile of FIXMEs were magically done and beautiful. In his case, he measurably made things better and also only worked on the things that he was assigned to, so it was fine with us.

In your case, this is not the work that he's assigned so you can

• ask him not to do it: it's not his assignment
• tell him not to refactor code that's not his (ie, leave other people's code the same)
• require him to schedule the tasks including creating baseline unit tests for every routine/method that he wants to change across substantial input ranges and verify the output on the refactored code. You can tell him that you're trying to make it unpalatable because the cost of fixing the bugs that he's introducing will surely be more than the time to refactor, and wouldn't he rather be working on core features?

Answer 14

Something like this is unacceptable. If there is no set coding standard/style/layout then the programmer should mimic the existing code and style and refrain from refactoring anything.

If this programmer has the time to refactor everything then clearly s/he needs more work assigned.