When you're the new guy and you keep seeing dumb things - do you refactor them?

Question

Do you refactor when you see things like this? Or do you just plug your nose and move on?

    public Collection<DataValidationRuleBase> GetFieldValidationRules(String key)
    {
        Collection<DataValidationRuleBase> found = null;
        try
        {
            this.mRules.TryGetValue(key, out found);
        }
        catch (ArgumentException ex)
        {
            //log the error
            Log.Error(ExceptionHandling.BuildExceptionMessage(ex));
            return null;
        }
        return found;
    }

Answer 1

If you're the new guy, you move on.

It is unlikely to be looked on favorably if you start being a cowboy and refactoring stuff all over the place, especially if it's unrelated to what you're working on. Even if you "know" it will improve the codebase, and you might feel you're "taking initiative", the first time you screw it up and introduce a bug in code that was working before, it will look very bad for you.

I would make note of all the things that you see that need refactoring, and when you've built your reputation and trust at the company, you'll have significantly more leeway to go back and improve things.

Answer 2

if it's working then leave it like this, you could never know what might happen if you will change stuff

Answer 3

Strip out anything that identifies your company (or the previous coder) and send them to The Daily WTF :)

Answer 4

ya, if your the new guy, then you don't refactor stuff, you just do what the team leader is telling you

Answer 5

what does TryGetValue do?

do you need to always track error? (the logging thing)

there is many thing that could be valid with this thing

Answer 6

When you are new, watch out who you tell about "crap" code. It is highly possible that they wrote it, or that they write code the same way. This is a good way to get off on the wrong foot with a new job.

Answer 7

Unless you work in Dilbert-world, you have a talk with your manager. "Hey, I think I found a problem and this is how I'd like to fix it." Discussion ensues. You might move on to talk to the original developer and some human contact will take place.

How is this a complicated problem?

Answer 8

Don't change working code without good reason.

If you think there is something wrong with some code, then point it out to your team leader, and let him decide what to do with it.

Reasons:

• You don't know why the original programmer did something. It might be idiocy, or they might have a sound reason for what they did. It's just possible that you are the idiot and have not grasped some subtlety of the code (although the other programmer should have commented it clearly if that is the case!)

• Any change to the code is something that needs re-testing, and potentially could introduce new bugs in otherwise working code. That shouldn't stop us refactoring to improve code quality, but we shouldn't just change everything we think is wrong without considering it carefully.

• If you "correct" other peoples' code, you stand a high chance of generating resentment and coding "battles" with your team mates.

• Your team leader can deal with it approriately (take the original programmer to task, lecture the team, or get it quietly fixed, etc) without anyone knowing that you "pointed the finger" at them. And you'll get brownie points with your boss for pointing out the flaw... Unless it's his code :-)

(You can also check in Source Control to see who wrote the code)

Answer 9

You are a little vague on your definition of idiocy in the code.

If it's because TryGetValue doesn't throw an ArgumentException but instead throws an ArgumentNullException, you should be safe in fixing it.

MSDN Dictionary.TryGetValue Method

Answer 10

Rule of thumb: If it ain't broke, don't fix it!

If you're the new guy, and you just happen to come across some smelly code, I wouldn't recommend changing the code unless doing so is part of whatever enhancement you are currently working on.

If it appears to be a major issue, a more acceptable alternative would be to try to write a unit test for the case that fails, and then, depending on how your company normally deals with these issues, either go back and fix it, or let the testers come across it and assign it to the appropriate developer.

This approach is likely to earn you some brownie points, as it shows that you're paying attention and being proactive, without introducing potential side effects in the code.

Answer 11

I would usually write some unit tests (hopefully this isn't a foreign idea in your company) based on my assumptions and leave it at that.

Occasionally there are (valid?) reasons why the code is strangely written that can only be seen from a failed unit test or when a change is made by a new hire.

Later, when I'm not the new guy and have a better understanding of the code base I would perform the refactoring.

This also has the added benefit of learning how the code works without being a cowboy -No matter how tempting it can be :)

Answer 12

(Almost) Every company that has a smaller headcount than its workload warrants will have technical debt or messy code. Unfortunately, (almost) every company is like that.

Unless you're in an agile organization or in a project that started out with low technical debt, and kept it that way, fighting this is almost hopeless. Under time pressure, we all write code like that. In fact, even Uncle Bob writes ugly code before he has the time to refactor the bejesus out of it prior to posting it in a book.

If you start refactoring things, you're running the risk of breaking something. If your organization does not have comprehensive unit tests, that's not a risk you should be taking.

Cleaning up technical debt is an organization-wide decision or at least project-side. It doesn't start with the new guy, unfortunately.

Answer 13

Bad Programmers:

Refactor it, check it in, and move on without saying a word.

Good Programmers:

"Hey Neil, I came across this method and was wondering why it was written this way.. this return null here seems redundant, mind if I drop it to clean up the code a little bit? Or is there a specific reason you wrote it like that?"