Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I just joined a (relatively) small development team that's been working on a project for several months, if not a year. As with most developer joining a project, I spent my first couple of days reviewing the project's codebase.

The project (a medium- to large-sized ASP.NET WebForms internal line of business application) is, for lack of a more descriptive term, a disaster. There are three immediately noticeable problems with the coding standards:

  1. The standard is very loose. It describes more of what not to do (don't use Hungarian notation, etc..) than what to do.
  2. The standard isn't always followed. There are inconsistencies with the code formatting everywhere.
  3. The standard doesn't follow Microsoft's style guidelines. In my opinion, there's no value in deviating from the guidelines that were set forth by the developer of the framework and the largest contributor to the language specification.

As for point 3, perhaps it bothers me more because I've taken the time to get my MCPD with a focus on web applications (specifically, ASP.NET). I'm also the only Microsoft Certified Professional on the team. Because of what I learned in all of my schooling, self-teaching, and on-the-job learning (including my preparation for the certification exams) I've also spotted several instances in the project's code where things are simply not done in the best way.

I've only been on this team for a week, but I see so many issues with their codebase that I imagine I'll be spending more time fighting with what's already written to do things in "their way" than I would if I were working on a project that, for example, followed more widely accepted coding standards, architecture patterns, and best practices. This brings me to my question:

Should I (and if so, how do I) propose to my project manager and team lead that the project needs to be majorly renovated?

I don't want to walk into their office, waving my MCTS and MCPD certificates around, saying that their project's codebase is crap. But I also don't want to have to stay silent and have to write kludgey code atop their kludgey code, because I actually want to write quality software and I want the end product to be stable and easily maintainable.

share|improve this question
4  
How much experience do you have with ASP.NET? (besides your MCPD credential). –  Marcie Jan 28 '11 at 21:35
10  
Welcome to any successful product. The 'legacy' code is always crap. –  Steve Evers Jan 29 '11 at 0:11
    
en.wikipedia.org/wiki/C'est_la_vie –  Job Jan 29 '11 at 4:15
    
I'm pretty sure i saw a similar question on stackoverflow. There is no way i think one engineer can move a giant mountain of code poop with a small shovel. I guess you can start be teaching your coworkers refactoring. –  Reno Jan 29 '11 at 9:34
    
I left a job because of finding this situation, and I'm not even a programmer, I'm a system administrator but saw enough of the code, and coding principles to know it would KO the company, (which it did). The best I could do, which is helping them now, is explain that three weeks cleanup, and rewriting the core module will save months in future development - It took a application meltdown, before they began to consider it, by which time my CV had found its way online! Stand your ground, and prove your point if you can, then leave the decision to management it'll make your life easier. –  Mister IT Guru Apr 22 '11 at 11:37

14 Answers 14

You could spend your time arguing your case; or you could spend your time cleaning up as you go along.

Pick up Clean Code and Agile Software Development Principles, Patterns, and Practices and apply what you learn there as you work on the system. Eventually, people will notice (for better, or worse).

Edit Also check out Working Effectively with Legacy Code

share|improve this answer
    
The proof of the pudding is in the eating. If what you see actually does require repair, fix it - and those around you will soon notice. –  blueberryfields Jan 28 '11 at 21:16
1  
Well, I already intend to fix little things as I go along. But, for example... how the team does popup windows on their forms is just wrong. It's something that was custom-built, and used throughout the entire application. I don't think I can get away with rewriting it without anybody noticing, asking questions, or reverting my changes. –  Adam Maras Jan 28 '11 at 21:29
11  
I use a term called "opportunistic refactoring". (It's actually redundant because all refactoring is opportunistic). We've all had to work on eyesore code bases. Trying to change things with words just puts the team on the defensive. Change by doing (code is your currency) and when someone asks why, explain your reasoning to them (in better words than "the old way sucked"). –  Mike Brown Jan 28 '11 at 21:36
2  
If it's wrong, also add a test case to the test suite that fails if they revert the code. –  blueberryfields Jan 28 '11 at 23:54
4  
BTW, until you have seriously proved yourself with your code, you won't be taken seriously. Don't be the arrogant kid railing against the old guys. I'm not saying your perceptions of the code aren't correct, I'm strictly saying that your social position affects your message. Succeed with success. –  Hack Saw Jan 28 '11 at 23:59

I agree that you need to be cautious about this. You need to build reputation within the team before you can state criticism and propose deep changes in code or processes. I would just take notes about my findings & suggestions for the time being, and take opportunities to get acquainted with the team members and management, entering free discussions but not refusing if the talk turns towards quality issues, coding questions etc., sometimes even throwing some of my own questions into the pond (but in a general form, not too pointed towards any concrete issue I have seen within this codebase). This way I can get to know the team members' opinion and find potential allies.

In the best case, you may find that a significant portion of the team (and/or management) sees the same problems as you, just there has been no initiative to do something about them, or no resources granted to it by management. Together you have more say to convince management if needed.

As @Mike suggested, doing some of the cleanup yourself is surely needed, but again, better be patient with it. If you start too fast, you may alienate other team members who may take it as personal criticism. Also entering into extensive code cleanup / refactoring may be taken by your manager as a sign that you are not focused enough on the actual task. So you should get some mandate to refactor before embarking on it on a more serious level. Small local changes are probably OK though.

Also, to convince management, you need business rationale. Management is rarely moved by descriptions of how inconsistent coding style is. You need to show them what value the proposed changes can bring to the company - the bottom line is the cash. If you can put together a convincing looking calculation about the cost vs long term benefits of various proposed changes, and show that the overall balance is clearly on the plus side, you have improved your chances noticeably.

share|improve this answer

You're currently not in a position to prevent the bad code, so you're going to have to figure out how to contain it.

The PM's and team leaders will only care it it doesn't work. Their next concern will be when they ask you to make changes and wonder why 'simple' things take so long. That's where you'll come in.

Start now with the consistency issue. Whatever the potentially standard method currently is, try to identify it and see if you can't get it enforced. If you start with methodology no one else is familiar with, you'll get a ton of push back.

Other team members may want to get certified and you'll be a great resource. Hopefully they'll at least want to improve and look to you to show them the way.

Complaining about Hungarian Notation will get you no sympathy and is probably one of the last battles on the priority list.

share|improve this answer

I'd probably go down the road of trying to figure out how to present the proposal with the intent that you may end up not being able to justify your position adequately. A few questions to ponder in creating that proposal:

  • How much business value is gained by making the kinds of changes you describe here?
  • Have you considered options such as re-writing the application from scratch, modifying the existing code to get it up to snuff, or just doing little changes over time?
  • Do you know what features and bugs are wanted now that would prevent you from making the changes you'd want?

While I can appreciate wanting to fix the poor code base, this has to be weighed so that it makes sense from a business view to do it.

share|improve this answer
1  
Trust me, I'd love to propose a rewrite. I'm almost tempted to go home and start a new codebase in ASP.NET MVC 3 just to show them how much better it is. I just don't think it would be received well, as I'm new on the team. –  Adam Maras Jan 28 '11 at 21:31

Definetly don't go waving your MCSD around, people will just laugh at you quite frankly, MS certs are nice to haves but they are in no way a professional qualification!

Step lightly joining a new team, look for opportunities to suggest changes as you go.

Wait until you get the lay of the land before slagging off a teams code.

share|improve this answer
    
The reason I mentioned my MCPD cert is because there's a strong emphasis on proven-best practices. Sometimes, in a framework like ASP.NET, there really is a right way and a wrong way to do things. –  Adam Maras Jan 28 '11 at 21:30
    
No doubt there is a right way! But some new guy coming in waving a cert is not the way to go about it. Quote experience, it's more credible! –  jmo21 Jan 28 '11 at 21:46
1  
@Adam: just a thought, but the vast majority of examples I have seen - very particularly with ASP.Net and even moreso with MVC - show flat out terrible ways to do things, while claiming they are "best" practices. A simple look at how many examples use EF or L2S classes within their ViewModels is illustrative. –  qes Jan 28 '11 at 22:00

These things are OK to worry about but if you're new on the team don't rock the boat just yet, until you've built up some credibility with them.

It seems you have picked three (relatively) minor things to fret over. There are other things I'd worry more about:

  • Is the code well-documented?
  • Does the code adhere to specs/design docs?
  • Does the code work?
  • Is it easy to understand what the code does?
  • Are you considering submitting large chunks of this codebase to TheDailyWTF?
share|improve this answer
1  
1) No. 2) Not really. 3) Most of the time? 4) Sometimes. 5) YES. –  Adam Maras Jan 28 '11 at 21:27

From what you describe, it seems problems are mostly about not following coding standards and naming issues. That is not a disaster, by far. For a comparison, this is a disaster, for real.

Maybe you are used to and requires high standards? In that case any deviation from perfection might be considered a failure. That is a trap that's is easy to fall into when your demands are high, but it's important to keep perspective on things.

I suggest you talk about this as an improvement, and help the team to update their guideline and coach them to start using it for real. I really like to use a Definition of Done as a quality benchmark and creating one is a great team exorcise by itself. But I don't think you should make much more of it than that, at least not as a new team member.

share|improve this answer

Do not go directly to management with this 'problem'.

First, talk to your co-workers and find out from them why things are the way they are. Then you can make a better assessment whether there is a problem or not. You may be surprised at what you learn. You may also find allies in wanting it cleaned.

If you don't have any idea how you got to where you are in the first place, it makes it much harder to get out.

share|improve this answer

You've been there for a week? I'm not sure that you know enough yet to be making any proposals to the project manager. Even if you have your suspicions that the code and practices of this team are "bad", the best way to influence these things over time is to gradually gain their trust and respect. Coming into a project and after one week telling the team that their code needs to be rewritten is not a good way to gain trust and respect.

For your first few months, focus on setting a better example, and asking questions about why they have done things certain ways. Be careful about your tone when you ask these questions. If you come across as the "know-it-all" new guy, no one will listen to your opinions later on when you really are in a position to influence the way things are done.

Over time, if your code really is "better", the other developers will see that. They will begin coming to you as a resource to help them fix theirs, and then to ask for your advice about how they ought to be doing things. At that point, you'll be in a great position to tell the team (and management) your opinions on coding standards and the like. How long this process takes will vary by organization. It could be just a few weeks in a very adaptable environment, or months to years in a more stoic culture. Build your influence one person at a time.

share|improve this answer

Do it yourself. If you value certain coding styles, then work on code that violates the coding styles. Checkout a piece of offending code from source control, reformat the code to the style's whitespaces requirement (as an example), and commit the updated code with the message "Conform to the style guide's rule on whitespace."

share|improve this answer
    
+1: Right - Ask for forgiveness, not permission. –  Jim G. Jan 29 '11 at 17:42
1  
Fine if you follow the style guide and you are not behind on assigned tasks, bad if you are doing this before assigned tasks or changing to a style the organization has not dictated. –  HLGEM Jan 31 '11 at 14:24

After a week the worst thing you can probably do is go directly to management with this. In all likelihood they've put a lot of time into the code and have some degree of pride in it. You would probably come off as a severe "know it all."

What I would do if I were you is to improve it as you go along. As you get more familiar with it and as you work on more of it you will have the chance to improve it. It would be at that point that I would start showing the benefits of what you are doing to other co-workers. After that, when the design meetings come up for new modules, present an argument for what you perceive as the best way.

And honestly, standards exist for a reason but, if it works and works well that is worth 100 times more in my book (especially after a week in). I would be more concerned if you opened the code and saw tons of logic errors or something of that nature.

share|improve this answer
    
+1 for severe know it all. I follow an ex-MS guy on twitter and he's doing the exact same thing in a new role & tweeting about it, big mistake! –  jmo21 Jan 29 '11 at 0:12

You will never go into a new job where you will think the code base is perfect and everything has been done the "right" way. Learn to accept this. All you can do with legacy code is refactor and move forward bit by bit. Some things you don't want to refactor until you have more understanding of why they did what they did. Also, no one in business is going to pay you to fix working code, so you would have to make a business case for why it needs work before stepping up and spending the time. YOu are better off waiting to refactor code when there is a change you need to make to it anyway. You may not have chosen the method they did for some things, but if it works, be very careful about changing it and introducing new bugs. Particularly when you haven't been asked to change it.

After one week, you have no credibility to be making major suggestions about how they do business. If you bring things up now, people will laugh at you and will never take you seriously. Prove yourself with some good code first and then people will be more inclined to listen.

And frankly no one cares that you are a Microsoft Certified Professional. Anyone with lots of experience has seen as many bad programmers who had that certification as good people. I'm not saying it makes you look bad to have a certification, I'm saying we don't put much credence in them because we have not seen where they are effective is showing us who is a good programmer.

share|improve this answer

You might consider that the problem is you. NONE of the three things you mentioned are worthy of a complete rework of an application. They are just nitpicky details.

Remember that the goal of the application isn't to have beautiful code, it is to solve a business problem. Sure it is nice to have code that is consistent and follows a good standard, but the lack thereof is no reason to trash the entire codebase. Just because the engine is oily doesn't mean it needs to be rebuilt.

Look, everyone hates every codebase they didn't write. In fact, if you wait three years and look at your own code you will hate it too and think it needs a complete rewrite. The truth is, it doesn't. Unless you can make the case that spending months on making the code aesthetically pleasing to you instead of building features to add business value you are probably barking up the wrong tree.

Nothing says you have to write kludgy code just because there might be some in the codebase. And nothing says the code IS kludgy just because it doesn't adhere to your pet style. Just refactor as you go along on parts you work on and write good code when you work on new stuff.

share|improve this answer

Lots of good suggestions here already, and I'm of the don't-burn-your-bridges-until-you've-proven-yourself-first opinion like the others. What I would add is discuss things over with your teammates well before alienating them by going to the project manager or team lead first. And certainly show them before that, that you should be taken seriously.

It sounds too that it's more of a stylistic issue you're having with the code. Taking over someone else's code and holding your nose while getting use to the way they wrote it is just part of the job, even if it is a trivial thing like the way they format it. Handing over one of your 'babies' to someone else for them to mangle it with their grubby hands is even worse...

If it is ultimately more than that - and the problem is more functional than just stylistic - if you're going to the team lead/pm it's best to pitch the need for rework in terms of where it would save money and effort in a future development project. Good old refactoring.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.