Colleague's code. Am I allowed to pick holes in it?

[bhonda]

So - I'm in a bit of a moral pickle.

For the past few months, another developer here at work has been working on this big new project. I've just had a chance to have a quick peek through the code, and even though I'm sure it's been well thought out, there are a few little things that stand out as just not very good.

But we're not talking trivial little things, like variable naming or line indentation, but actual design decisions, and I think it could have been done better.

Things like using interfaces, and abstraction into methods. To me, they are obvious things to have done, but in this case, they've not been done.

What do I do?

I'm not involved in this project yet, but I will be in a few weeks.

I'm sure the application will work, but it's not 'excellent', it is just merely 'good enough'. And I like the excellence side of this work - keeps me from getting lazy.

Does that make sense? It's not that I want to get one over on my friend, but I just want to make sure everything we do here is the best that we can do.

So, do I have a friendly chat with my friend about it? Print out the code, circle the 'bad bits' with red pen and leave it pinned up on our manager's office door? Leave it be and hope for the best?

Just wondering...

[weeks]

It's obvious you have a lot of respect for this person's ability as well as their feelings. And it's clear you realize you're not perfect either. You say it well here. Why not send this person a link to this post?

I would add this: How much work would it take to fix these problems? How much time? You both are in business, ya know, not creating a work of art for all time.

So, with that said, consider which issues that need to be addressed. (Line indentations? Gimme a break.)

[grandpa]

At one outfit where I was a programmer we did a peer review on any major projects. This had two objectives - to familiarize the other programmers with the code and to sort out any glaring deficiencies.

As a programmer, I tended to be creative - but not always efficient. My stuff worked, but in a couple of cases is was made to work better with some suggestion. Talk to your friend. Tell him you looked over the code. Ask why he prefers this method over that method. Don't come off as judgmental but instead as interested in the approach to the solution.

[londrum]

well i'd be annoyed if you did that to me. if you're actually working on the project then fair enough, that is the time to speak up. tell him then. i've no problem with that. but trashing his code when you're not even working on it yet would get my hackles up, especially that bit about pining up his mistakes on the manager's door for the boss to see. was that supposed to be a joke?

[trillianjedi]

Talk to your friend. Tell him you looked over the code. Ask why he prefers this method over that method. Don't come off as judgmental but instead as interested in the approach to the solution.

Exactly right. And during that conversation be prepared also to learn something you haven't considered before... (eg, be open-minded)

[drongo]

There's more than one way to write solid, robust, working and bug-free code.

From a business point of view, if it fulfils the above criteria, it should be fine. You can introduce the developer to the higher concepts over a pint.

"The perfect is the enemy of the good." :-)

[Demaestro]

If it were me without saying why, I would try to implement some type of internal code review sessions. Do it under the premise of standardizing internal programming practices.

When I was at a dev house we would do them once a week. Boss would buy lunch and we would use our lunch hour.

We would take sections of code and review them as a group, pointing out good and bad programming practices, doing theoretical rewrites or optimizations.

Make sure you seperate people's egos from their work. It is not good for your health to become emotionally attached to your code anyways. Once egos are gone it is easy to criticize implementation without hurting people's feelings.

Start with some of your code and then work your way to his, when discussing it as a group you may find there were reasons you hadn't thought of for doing things the way he did. Maybe you learn something, maybe he does... hopefully you all do.

To answer your meta question though.

IMO yes you are allowed to pick holes in your colleges code. In fact if the code is going to produce bugs or security issues it is your duty to pick holes in it.

[rocknbil]

another developer here at work...

Which is it, friend or co-worker or both? How you approach it depends on the delineation.

... leave it pinned up on our manager's office door?

No! :-) I'll reserve my opinion as to what this would show, just don't. :-) Let's not forget most office printers have hard drives in them, if it came down to it you would be revealed. If you stand behind your convictions, you need to do just that.

As a programmer myself, I *beg* for input on improvement and excellence, anyone who loves what they do would do the same. But first, take a step back . . .

I think it could have been done better.

Ask yourself, does this thought come from your ego or solid facts that you can document and reference? This is a really hard one to face sometimes, if you can reference documentation your case will be much more acceptable than "I think it can be done better." Write them down if you need to. Some good examples might be

- these lines of inline execution code are repeated in several places throughout the application. They would be more useful and can be re-purposed if moved into a single function.
- you are doing a textual database query for *this* value when a numeric one on an indexed column would be more efficient.
- The redundant use of headers for 1000 pages is difficult to maintain, let's move those into an SSI or PHP include and do redirects to make them easier to modify.

Maybe some specific examples will help members here to provide input on whether you're on the mark or not. An example . . .

Things like using interfaces

This can mean a lot of things, for example, they hand coded a Javascript function instead of using a library and jquery. Maybe the task was so limited that the overhead of jquery would only bog down the page, I often make this decision myself. See where that's going? One of the most difficult - but can be, most rewarding - experiences is working with other programmers, learning their coding methods, getting into their heads. At first you may think you can do better, but after a time it rubs off on you, there may be reasons they didn't do what you think they should do. You wind up appreciating their logic and you grow. It's called eclecticism and works as well in programming as it does in forming rock bands.

When all is said and done, don't confront, call a meeting with everyone and discuss. "Here's what I'm thinking John, this area could use improvement, be more efficient. What does everyone think?" Be open to "no." You may be right, but you may not be.

[drongo]

Another example is database normalisation.

We're all taught to normalise to whatever normal form, but some of the most efficient web sites out here are thoroughly denormalized, breaking all the rules of "good" coding.

Us programmers should try to denormalize our brains in terms of our concept of what "right" is.

[wheel]

Are you this person's manager? I didn't think so. How about you let them look after their staff's code?

(being blunt here - stick to your own knitting. You start p*****ing in other people's work, offering unsolicited advice, and it won't end well for you).

[ergophobe]

Read the bits about feedback (and feedforward) in Marshall Goldsmith's book What Got You Here Won't Get You There

For a capsule version - watch his google talk (search on his name plus "leading at google").

Watch for the danger of "trying to add value".

That said, I have written brutal but really in-depth critiques of someone else's work with great fear and received effusive thanks and praise for my efforts. I've made what I thought were mild suggestions and eventually was told that the person couldn't sleep that night because she was so upset. None of these were people over whom I had any power at all. The first one was my boss and the other was a friend.

[ergophobe]

>>offering unsolicited advice

Why were you shown the code? Was it to review?

In an ideal world, we would all welcome and constructive comments, but not everyone does and even people who do, don't welcome them at all times.

Personally, I like to do so in phases and if I'm not in a "feedback phase" I will pretty much ignore your feedback - not intentionally per se, but I will have my own list of 1000 things to fix from the last feedback phase and when you bump that list up to 1010, it really isn't helping. When I get my list down to 20 things and you add 10, that becomes a lot more valuable for me.

[wheel]

I'm reminded of a Dilbert cartoon where the final caption is "this is where you learn that your my coworker, not my boss" :).

[Demaestro]

this is where you learn that your my coworker, not my boss

That is weak. You don't need to be someone's boss to comment on your co-workers attitude or poor work habits/practices.

It is almost universally true in programming that those who can't, manage those who can.... Meaning the boss is often more inept then the co-worker when it comes to writing code. So to assume management will catch it is silly. If you have no Q/A or source control then you may have to be the one to point this stuff out if there are no processes set up to catch them.

In this situation it sounds more like programming practice is at play then security or bug issues, but even bad programming practices can bring a system down if it scales big enough.

Like I said, if you notice this, it goes into production, and it causes crashes or introduces security flaw then I would call you negligent if you didn't bring it to the attention of everyone.

Not being the boss has NOTHING to do with this. It has to do with being a responsible employee.

If I was a cook and saw other cooks storing meat improperly, like raw chicken with raw beef, I wouldn't say... "I am not the boss, it isn't up to me to make sure people don't get sick"

That is a total cop out. You have some responsibility here, the question is how serious are the flaws in the code?

[londrum]

its not a cop out at all, it's about being polite.

you dont give your neighbours the benefit of your parenting advice every time you see their kids playing around, because its none of your business.

if he's not working on the project and he's not the boss, then the same thing goes. he shouldn't butt in and tell him his stuff is wrong, when by his own admission it works perfectly okay.

[johnblack]

I'm sorry I've not read through all the comments, so apologies if I'm repeating things someone has already said.

If I were in your shoes I'd wonder why he wrote the code the way he did. You can even ask him if you want, I don't think it would be rude especially if you are expected to pick it up soon. There may be valid reasons why he did it the way he did. Time constraints, design restrictions imposed by others, or he just did not think of it at the time.

I've sometimes looked at old code that I've been handed to debug and my first reaction is why on earth did the developer write it this way. Then on deeper inspection I've spotted my own name in the comments section of the code!

Do you have coding standards in place? Do you have peer code reviews? Both I think are good ideas and probably necessary in multi-developer environments. I'll be saying developers should write documentation next ;-)

Anyway tread carefully, but if you're expected to pick up this code, I don't see any problem in politely asking why it's been written the way it has.

[bhonda]

pinned up on our manager's office door

was that supposed to be a joke?

Yes. Yes that was a joke. Sorry - I always forget tone never comes through in text!

I'm appreciating all the discussions on this.

It is almost universally true in programming that those who can't, manage those who can

I'm saddened to say that this is exactly the case here. I'm conscious of the fact that there is nobody else above me who would even consider looking at code in this way - I suppose it comes with working in such a small team (there are only 4 main developers here).

Ask yourself, does this thought come from your ego or solid facts that you can document and reference?

Now I've had time to think about it, I have to admit it's a bit of both. If I'm honest, I think I'm just a bit jealous - I would have loved to work on a project like that. Even though there are some solid facts I could point out as 'not ideal', I think the reason for pointing them out in the first place was most likely due to that jealously, and maybe, subconsciously, I'm secretly hoping that management will turn around and say 'Yes, you're right. This whole thing won't work. Let's scrap it, and we'll give it to you to do properly. Now, go forth, and make us proud. And have a raise while you're at it. And your own office. And a title.' Hmm...maybe not!

Seriously though, all emotions aside, I've got every confidence that this project will actually work, and the vast majority of it looks brilliant. There are a handful of little things that do throw up a little warning flag for me, but they are not life-or-death issues.

Politely asking why it's been written the way it has

I think that's it - I'm well aware that I don't know everything. I guess I just need to have a chat about it. It could be the case that my friend just didn't think about it when they were writing it. Or it could be the result of weeks of research and testing to make sure it was the best solution. I have no idea. It would be good to find out!

So, once again, WebMasterWorld, I thank you.

[bwnbwn]

Nice ending bhonda took guts to see and understand the motives that sometimes drive us to make poor decisions. I am very proud of u in the way your looking at this.

[weeks]

Now I've had time to think about it, I have to admit it's a bit of both. If I'm honest, I think I'm just a bit jealous - I would have loved to work on a project like that. Even though there are some solid facts I could point out as 'not ideal', I think the reason for pointing them out in the first place was most likely due to that jealously, and maybe, subconsciously, I'm secretly hoping that management will turn around and say 'Yes, you're right. This whole thing won't work. Let's scrap it, and we'll give it to you to do properly. Now, go forth, and make us proud. And have a raise while you're at it. And your own office. And a title.' Hmm...maybe not!

Love it! I'm over 50 years old and I STILL cannot get over how life is unfair. But, then, sometimes I realize if life was, indeed, fair I'd probably be worse off than I am.

The personal traits that make for great coders (being tacky, I'd call it perfectionism) can also create serious problems in the work-a-day world.

Example of how this can go on and on:
[online.wsj.com...]

[kaled]

Things like using interfaces, and abstraction into methods. To me, they are obvious things to have done, but in this case, they've not been done.

Ok, sounds like you're talking about object-orientation and Windows in particular. If I'm right, listen up...

Keep your nose out it - it's not your project. Such decisions may be due to programming style but they may also be down to practical problems that you are simply unaware of. If the code works properly and fulfils the role it was designed for, that's good enough. Moving certain code into objects can be done easily at a later date if necessary.

I'll give you an example - I implemented zip-file inflate a few years ago. I then decided it would be nice to re-do it with object orientation but as well as adding to code bloat it slowed the code down horribly so I ditched it.

Kaled.

[ergophobe]

you dont give your neighbours the benefit of your parenting advice every time you see their kids playing around, because its none of your business.

Actually, I grew up in a fairly small town and that is *exactly* how things worked. More precisely, if you got out of line, it got back to your parents, not as "parenting advice" but as "useful information about your kid you might want to know".

And so it should be with commenting a colleague's work. If that person wants help/feedback, you should be honest, but also non-judgmental as much as possible. Give your reasons and stop there. But if that person doesn't want feedback, nothing good will come of it. The hard part is knowing where they are mentally, but unless asked, I usually shut up.

[Hoople]

My grandpa used to say - 'Never mess with another man's wallet'.

If it's not severely detrimental to the continuation of your employment/the company just let it go.....get a hobby outside of work to occupy your time.