I hate taking over someone’s bad code. The projects always go on forever and you are forced to follow bad practices to try and string together fixes and features without it all crumbling down. There are some quick, not always 100% accurate, ways to know whether you’ve inherited a disaster. I’m strictly talking about enterprise scale projects here, some of these practices might be ok for smaller stuff. Some of these are hard to quantify (what’s too much?), it’s all very relative to the size and type of your project.
Hit ctrl-shift-f from the 2nd dropdown box, select to look in the “Entire Solution” and enter the following into the first box:
1 – ‘catch’
The results that get displayed will give you a good idea about what type of exception handling you’ll have to deal with. If there are many lines with just “catch”, “catch(Exception XYZ)” or “catch(XYZ)” you’ve got some bad exception swallowing happening. You should expect to see this no more than once or twice (in a global handler). If you are seeing specific exceptions being caught, all’s good.
2 – ‘throw;’ and ‘throw e’
If “throw;” returns a lot of hits, I’d say it’s a good sign – the previous developer understood not to mess with the stack trace when rethrowing exceptions (you should spend a bit of time figuring out what he/she was doing catching the exception in the first place). “throw e” implies that exceptions are being caught and poorly rethrown. I use “throw e” because most people will name their caught exception “e” or “ex”.
3 – ‘throw new’
Unless there are a lot, this is a good sign. The developer wasn’t afraid of throwing exceptions. A good way to tell if he or she might have been over zealous is to see if these are spread out amongst many classes. In my experience, throwing exceptions tends to be centralized in like-classes.
4 – ‘: Exception’ or ‘:Exception’ (VB.NET ‘Inherits Exception’) (also try ApplicationException)
It’s hard for me to imagine large projects without at least some custom exceptions.
5 – ‘using (‘ and ‘using(‘ or ‘finally’
Developers who can’t cleanup their resources aren’t likely to cleanup their code – sorry. Hope for a lot of these!
6 – ‘on error resume next’ (VB.NET only)
(FxCop does a lot of analysis around exception usage, so I’m going to recommend that you use it again. You can learn more about exception handling here.)
5 – ‘StringBuilder'”
Few applications get away without needing to do a good amount of string concatenation. There’ve been so many blog posts about the importance of the StringBuilder class, that if you don’t find any it not only means you likely have ugly string concatenation and performance issues, but the developer didn’t spend much time learning and reading.
6 – ‘.Fomrmat’ and ‘.AppendFormat’
I’ve expressed how important I feel string formatting is in the past, so no one should be surprised to see it in this list. Have fun maintaining impossible to read code if you aren’t seeing many of these. (if the line looks like an SQL string, then take a quick skip to #9)
7 – ‘.ToString(“‘
Somewhat related to the above item, .NET has great built-in functionality to help you format all types of data (dates, numbers, etc) into meaningful string. You’ll likely find a lot of ToString()’s in your code, but you should also find a healthy number of ToString(“XYZ”)’s
8 – ‘SELECT ‘ or ‘UDPATE ‘ or ‘DELETE ‘ or ‘INSERT ‘
In my experience, people who use inline SQL statements simply haven’t bothered to learn how to use stored procedures. Now you get to pay the price for their laziness.
9 – ‘+ ” AND’ (VB.NET should also do ‘& ” AND’)
Inline SQL statements aren’t the end of the world, but injecting parameters via string concatenation is horrible (even if done via a string.Format). There’s a good chance the code has security holes, sorry.
10 – ‘new DataSet’
While not everyone will agree with me that DataSets shouldn’t be the foundation of an enterprise scale project, it’s good to know that you’ll be dealing with them.
11 – ‘”<‘ (change the file type to *.vb;*.cs)
You’ve possibly just found HTML or XML manually being put together. Who needs ASP.NET or System.XML, not your predecessor!
12 – ‘Response.Write’
13 – ‘HttpContext.Items’
The HttpContext is a new introduction to ASP.NET and it’s quite powerful. Unfortunately it’s also quite underused. Seeing the variables stored and retrieved from the HttpContext.Items collection is an good sign
14 – ‘OutputCache’ and ‘Cache.Insert’
Despite the fact that server side programming exists to deliver dynamic content, there’s almost always something that can and should be cached. You’ll often see programmers coming from another language overlook this because it’s not something you’ll find in many frameworks.
15 – ‘configSection’ ‘httpModules’ ‘httpHandlers’ (in the web.config)
Seeing custom configuration, use of HttpHandlers and HttpModules should be a very encouraging sign. It’s a decent sign that the previous developer had a solid grasp of ASP.NET. I wouldn’t panic if I didn’t see these though.
16 – ‘SqlDataSource’
While there might be some debate about the properness of DataSets in enterprise-type application, hopefully we can all agree that you shouldn’t find any SqlDataSources in your code.
17 – ‘interface’
If you don’t find any interfaces in your project, you could be in trouble – especially if there are a lot of classes.
18 – ‘(I’ (dunno about VB.NET)
I’m not sure what it means if you find a lot of interfaces (above), but not much code that’s casting to them. I think I’d be happier a few hits on both.
19 – ‘abstract’ (VB.NET ‘MustInherit’)
Somewhat similar to interfaces, there’s no reason that you don’t find a couple abstract classes in your code.
20 – ‘internal’ (VB.NET ‘friend’)
I’m a big fan of minimizing the public API published by an layer. In many cases, a public class, class member, interface or enum should really be internal. I’ve seen surface areas reduced by as much as 50%.
21 – ‘sealed’ (VB.NET ‘NotInheritable’)
There’s no reason to panic if you don’t find a sealed class in your code, but if you do, it’s likely a good sign.
22 – ‘GC.’
People who mess with the garbage collector are either very desperate to cover up a deep architectural problem, or simply think they know better than everyone else. Both of which spell doom for you.
23 – ‘Int16′ or ‘short’
More amusing than anything, this often indicates a micro optimization that’s actually going to cause a performance hit.
24 – ‘(string)Request.Q’ or ‘Convert.ToString(Request.Q)’ (in VB.NET try ‘cstr(Request.Q’)
I wouldn’t have mentioned this obvious one had I not just seen it, but come on, my grandma doesnt’ unecessarily cast!
25 – ‘List<‘ and ‘Collection<‘ and ‘Dictionary<‘ (VB.NET ‘List(Of’ and ‘Collection(Of’ and ‘Dictionary(Of’)
If you’re working on an 2.0 project, I hope you’ll find that generics are being used throughout the code. They are by far the best new feature of 2.0 and it’s a shame if they aren’t being taken advantage of. (note, you could search for the System.Collection.Generics namespace, but I’m pretty sure the default C# template includes)
26 – ‘assert’
If you don’t get any hits, there’s little chance that the code in question was unit testing. Finding asserts doesn’t guarantee anything, but it’s a good start.