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.

Disclaimer: I'm a newcomer (this is my third day of work), and most of my teammates are more experienced than me.

When I look at our code, I see some code smells and bad engineering practices, like the following:

  • Somewhat inconsistent naming guidelines
  • Properties not marked as readonly when possible
  • Large classes - I noticed a utility class that consisted of hundreds of extension methods (for many types). It was more than 2500 lines long!
  • Large methods - I'm trying to refactor a method which is 150 lines long.

The latter two seem to be a real problem. I want to convince my teammates to use smaller classes and methods. But should I do that? If yes, then how?

My team got a mentor from the main team (we're a satellite team). Should I go to him first?


UPDATE: Since some responses asked about the project, please know that it's a working project. And IMHO, huge classes/methods of that size are always bad.

Anyways, I never want to piss my team off. That's why I asked - Should I do that, and if yes, then how I do that gently?

UPDATE: I decided to do something based on accepted answer: because I'm newcomer, so I see everything in "fresh eyes" I will take note on all code smells that I found (position, why it bad, how can we do it better,...), but at the moment, I just try hard to gather respects from my team: write "better code", know people, know why did we do that... When the time is right, I will try to ask my team about some new code-policies (naming guidelines, smaller classes, smaller methods,...), and if possible, refactor some old code. It should work, IMHO.

Thank you.

share|improve this question

migrated from stackoverflow.com May 12 '11 at 11:39

This question came from our site for professional and enthusiast programmers.

11  
A recommendation I would make is to look at the source code they are checkin in now, not what is currently in the project. It is possible that most of the current code was not written by your coworkers, but rather by your now managers 10 years ago. –  earlNameless May 12 '11 at 11:45
14  
You're likely to just piss people off since you've only been on the job for 3 days. Get to know the team first and earn some respect. Bring things up in casual conversation to feel the waters out. You've got the right idea, but you may be a race horse in a stable of farm horses. –  kirk.burleson May 12 '11 at 12:34
4  
Welcome to the real world :) –  fretje May 12 '11 at 13:22
    
With smaller functions JIT compiler will be happier and code will be faster. Effective C# Second Edition Item 11. my.safaribooksonline.com/book/programming/csharp/9780321659149/… –  Job May 12 '11 at 15:12
3  
I couldn't help but chuckle when I saw how horrified you felt upon witnessing a utility class 2,500 lines long. I've seen more than one 25,000 line class in my career. Don't get me wrong, though, I think a class is getting too long after 500 lines. –  PeterAllenWebb May 12 '11 at 15:49
show 2 more comments

15 Answers

up vote 16 down vote accepted

You have the benefit of viewing the code with fresh eyes. Take notes to document what you discover of bad practices. Then, as you're getting settled with the team, pull out your notes at an opportune moment, like when it's time for refactoring.

share|improve this answer
2  
+1 for "fresh eyes". This encourages me a lot –  Vimvq1987 May 11 '11 at 3:10
    
Really good idea. Write things down now, propose some changes later. –  Roman Grazhdan May 12 '11 at 14:02
2  
This works in theory, but in practice they will just beat the bag out of him. –  Job May 12 '11 at 15:13
add comment

Code Complete, by Steve McConnell, has tons of good statistics on the very topic you're talking about. I don't recall all the numbers, but he talks about how numbers of bugs increase with longer methods/classes, how much longer it takes to debug, etc.

You could buy a copy of the book, and show your peer some of the studies... stats (although they lie all the time) tend to convince people.

share|improve this answer
1  
+1 for Code Complete. Also research the term "Technical Debt". I find it very useful in explaining to others why sometimes (but not always) it's worthwhile investing in making code simpler. The first rule, however, is to create tests. Unit tests, system tests, integration tests, etc. Before doing any refactoring, create tests. Test, test, test. Tests. –  Ben Hocking May 12 '11 at 12:25
    
@Ben no disrespect but I think the term "Technical Debt" is way over used. As soon as somebody starts using that as their reasoning behind a decision I tend to stop listening. Maybe its a flaw on my part but when I hear that my thoughts go towards "this person reads a lot of blogs but doesn't really understand the real costs of balancing reworking code vs other tasks" –  Gratzy May 12 '11 at 14:18
3  
@Gratzy: I'm sure it depends on your personal experience. I don't want to go into details, but when you see projects in "technical debt" up to their necks, the expression becomes very apt. Coders can spend 90% of their time "paying the interest" on the debt. In those cases, it's not surprising to find out that no one on the team has heard of the term. –  Ben Hocking May 12 '11 at 14:23
    
Clean Code has lots of information about this as well (though, not many statistics). –  Steve Evers May 12 '11 at 15:50
    
If you take a look at page 173, McConnell presents some statistical evidence in favor of routine sizes that would probably make most agile advocates balk. He puts 150 lines pretty clearly in the OK (but not ideal) column, but gives a strong personal recommendation against going much past 200. –  Dan Monego May 12 '11 at 16:31
show 1 more comment

Unless you were hired with the specific intention of overhauling how your team writes code, you might wish to moderate your enthusiasm for drastic overhauls. Most working code works for a reason :) no matter how rubbish it is, and sometimes drastic overhauls makes those annoying corner cases even uglier.

I think the easiest lever into writing smaller code would be asking developers to focus on unit testing. Nothing forces concise code like being asked to test it; it's amazing how developers suddenly have an aversion to global data structures, passing too many objects too many layers deep, when they know they've got to write tests for it all.

I'm not a huge fan of TDD but I do love the fact that it forces developers to consider how they'd write tests. And that often is why the code is better, not some magic about actually having the tests. (Though that sure comes in handy when you make changes later.)

Best of luck.

share|improve this answer
    
++ for "moderate your enthusiasm". IMHO, the vehemence with which these principles are promulgated is in inverse ratio to their justification. (Religions are like that.) –  Mike Dunlavey May 15 '11 at 1:51
add comment

Disclaimer: I'm a new comer (this is my third day of work), and most of my team are more experienced than me.

You may want to slow down a bit, listen, and learn from your team before you go suggesting too many changes. There may or may not be good reasons the code is structured as it is but either way taking the time to listen and learn first can only help.

After that any suggestions you may make will most certainly be viewed more positively and be met with less resistance.

You're chances of successfully introducing change is greatly improved if you earn the respect, or at least don't lose the respect, of your coworkers first.

How does it go? "Measure twice cut once.." Something like that.

share|improve this answer
1  
I agree. who's to say they know it's bad practice but where on a very tight deadline? Or maybe they had a bunch of people previous to themselves that did the some of the code, and have not had time for refactoring? As you are new, keep an open mind when letting them know –  Spooks May 12 '11 at 18:39
add comment

You should not convince your team. As a newcomer, you won't be taken seriously - hence wasting the time.

Instead, go ahead and write compact and clean code yourself. Then hopefully, after a while and some code reviews, some teammates might start mimicking your style.

If not, you'll still be more productive and your hard work will eventually get you to a higher position where you can start enforcing some of these rules.

And yes by all means show stuff from Code Complete to everyone.

share|improve this answer
3  
+1 for "Instead, go ahead and write compact and clean code yourself". It is often best to lead by example. And cleaning up an established code base is more like a marathon than a sprint; it takes patience and perseverance. –  JeremyDWill May 12 '11 at 16:30
add comment

Here's a couple tricks:

  • Learn the team's current state and history - it sounds like they have a mentor, how much influence does the mentor have? Also, how new is the mentor and was there a long time with no mentor? When does the problem code originate? Criticizing the current team's baby can be a lot different than criticizing some old code that no one actually remembers writing.

  • One thing at a time - don't drop the bomb on all your thoughts at a team meeting. Start with some tentative questions that come from your specific perspective. For example - "Hey, as the new guy, I noticed that some of the utility classes are really big, is there a reason for that?"

  • Suggest baby steps - it's almost never possible to do an immediate total overhaul, so figure out some starting steps to suggest in case everyone agrees that this is a good plan.

  • Suggest future prevention mechanisms - for example, the team could agree to a goal that it will never add to the top few largest classes, but will refactor when there's a need to grow them further.

  • Listen to concerns about risk. If this is really legacy code, there may be enough unknowns and dependencies that refactoring is extremely risky. That may not be a reason to avoid refactoring, but it may mean you need some better test strategies or some other way to reduce risk before you tackle the real rework.

  • Be aware of body language and go slow. You're bringing up a problem in a code base that you haven't had a lot of experience with. You have a new guy window right now, where you can ask some naive questions and get helpful answers, and you can use those questions to probe the team to consider their own design choices. But it goes both ways - as the new guy, you also don't have a ton of "cred" yet, so go slow and be aware of closed faces or postures. If people start shutting down, suggest a way to delay any decisions and look for ways to win them over.

I can say as a manager and team member, I've been glad for New Guy Insights. I didn't accept every single piece of constructive commentary that a new team member gave me, but I was generally willing to listen if the criticism was voiced as honest concern and curiosity and not delivered as a lecture. The mark of respect to the new guy goes when he can deliver the insight and then step back and handle whatever comes - it's easy to feel good when your decisions are heard and taken up, it's harder when the team tells you "no". You may still be right, the trick is to figure out what to do next... usually waiting a bit and looking for more information is a good next step in those cases.

share|improve this answer
add comment

This seems to be more of an management question than technical question. All you said is valid, what your team really need is an good architect who can make sure everyone adapts to a single design pattern and enforce it, the team need to be constantly and regularly refactoring the code.

However, there is another "you ain't gonna need it" principal, if what ever existed works for a fairly long time, no matter how ugly it is, changing it is always not a good idea. Instead if your team need to rebuild the whole thing or rebuild part of it, accumulate a document of bad practices and problems before carrying out the coding.

share|improve this answer
3  
And surely, you should approach the team mentor, but before doing that, you should consult and discuss with your colleagues politely, don't make them feel left out. It gonna make your life difficult in the future. –  Anonymous May 11 '11 at 2:35
add comment

Some teams don't do any kind of quality control for code because they don't know the right tools for it. There are a lot of tools that can help a team code better.

Visual Studio has "Code Analysis" that may help with naming conventions.

Also code metrics could be used, like cyclomatic complexity. This helps point out classes and methods that are too complex.

Keeping records is also a good idea. If team members just verbally express what needs to be done, then people are bound to forget. Humans have very frail memories! =)

I would not make a lot of noise about this... A programmer's development team is like his own family... If you point out mistakes, people can get angry with you. This kind of culture change not only requires a lot of coding, but it also requires a delicate touch with human beings.

share|improve this answer
add comment

How do I convince my team to use smaller classes/methods?

Don't.

Buy yourself a Resharper license and lead by example. [Lean heavily on the 'Extract Method' refactoring.]

Over time, others should come to appreciate your more readable code and be persuaded to do likewise.*


  • Yes - There's a chance that they won't be persuaded; but it's still your best bet.

IMO - It's not worth your syllables to try to persuade your teammates to become better programmers, read 'Code Complete', follow @Uncle Bob. 's SOLID principles, and become better programmers if they're not already persuaded.

Remember: You can't use logic to argue someone out of a position they didn't use logic to get into in the first place.

share|improve this answer
4  
+1 and agreement. Your second point is what I've found to be most true; good programmers either know that stuff already or are willing to immediately start learning and applying it, bad programmers either pretend to care or just outright don't understand why those things are good (if they understood, they would have done it already). Chances are likely you're fighting a losing battle. It's sad how many "programmers" don't understand a lick about proper software development. –  Wayne M May 13 '11 at 11:01
add comment

As a manager I just want to add, that I want my team to write good code the first time. Code reviews, TDD and all that. But once it's in production and it works, you'd have to make a strong case to have us go back at it.

I do follow Uncle Bob's advice to always leave code better than you found it. So if we have bugs to fix or small enhancements, I'd hope we were doing some of the cleanup then.

But as it stands, the business really watches it's money. I'd have to make a case to them that refactoring benefits them enough for them to give my team the time and resources. Just not liking the way the code looks isn't enough.

So, if it works, as much as you might hate it, you might have to leave it alone.

Now new code, that's different. That should be good code.

share|improve this answer
add comment

Methods that are 150 lines... I've seen methods with 10.000 lines of code.

Two of your problems can be solved with external tools:

  • Somewhat inconsistent naming guidelines
  • Properties are not marked as readonly when possible

In C# Resharper can check both issues. Names which do not follow your guidelines are marked as errors. Properties not marked as readonly show as errors too. FxCop might also be a help.

These tools can also help to split up huge methods into multiple smaller ones thanks to its refactoring.

share|improve this answer
add comment

I don't know that large classes are always all that bad if they are structured well with well named methods. I use Eclipse as my IDE so it has something called the "outline" view, which is something all IDE's have just with a different name, most likely, that provides the name and link to each method within the class, you can sort it alphabetically, etc. When using this its easy to navigate a large class, its more that having really long methods would be bad, I think because its harder to navigate intelligently within that method unless you happen to be really familiar with it. I'm not advocating long classes but I think they are manageable in some cases and don't necessarily need to be broken down into multiple classes.

share|improve this answer
add comment

Bring the subject up with some of your team members and get their opinion on the size of methods. You may be surprised to find they agree with you. What you're seeing could be the result of bad previous practices, former developers no longer with the company, or that part was a rush job and now they've hired someone with time to refactor it ;)

share|improve this answer
add comment

You're still the new guy. Build up some reputation by taking on challenging assignments and completing them quickly and with no bugs. If you try to start changing things before you earn the respect of your peers, you may have a much harder time getting buy in (and possibly alienate your co-workers).

If you can find ways to introduce the better coding habits in your own work that effectively demonstrate how they reduce development time and result in more robust solutions, you might even have them coming to you to see how you achieved this.

share|improve this answer
add comment

Further to all the other great answers, maybe you can kill two birds with one stone and do some code cleanup as a project aimed at getting some understanding of the code-base. You can sell it to your team/manager as a learning opportunity for you, and you'll get feedback from your peers when they look over your changes which will guide you in your best approach for tackling the issue of poor design.

share|improve this answer
    
how does this answer the question asked? –  gnat Sep 17 '13 at 6:30
add comment

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.