Code Review Checklist

Earlier today on one of the forums where I hang out, I got into a discussion of code reviews. I mentioned that I recently put together a brief checklist for my company. One person asked about it, so here it is.

  • Are there things that can degrade performance
  • Do you find any infinite loops
  • Are Else and Switch Default conditions in place
  • Is there duplicate code or conditions
  • Has usability of the finished product been considered
  • Are calculations correct
  • Is the code readable
  • Does the code follow best practices and company guidelines
  • Are comments clear, meaningful, plentiful, and not overdone
  • Are complex algorithms and corner-case input clearly commented
  • Are there refactoring opportunities
  • Did you perform psychic or intuitive review (the code just feels wrong)
  • Are unit tests provided for areas where needed
  • Is the class design secure
    • Are variable scope and method visibility limited
    • Are non-base classes sealed
    • Are properties used to expose fields
    • Does the code use read-only properties
  • Is exception handling implemented correctly, including exception reporting and a proper page displayed to the user
  • Are protections against cross-site (XSS) scripting implemented
  • Are protections against SQL injection implemented
  • For public-facing web sites, have connection strings been encrypted

If you want to learn more about code reviews, look at these references

Best Kept Secrets of Peer Code Review”, Jason Cohen, 2011, published by SmartBear Software

Effective Code Reviews Without the Pain”, Robert Bogue, 2006, Developer.com

Running an Effective Code Review”, Esther Schindler, 2008, CIO.com


Why you should move to Windows 8

Microsoft has a long history of good and bad versions of Windows. Released in 2001, Windows XP is still used on millions of computers around the world. It was a great release. But there has been bad, remember Windows Me? Yeah, unfortunately, I do too. And while Vista was not well received, I didn’t find it as bad as people said it was.

Today, we’re close to availability of another version. Will it be good or bad? My guess is it will not be well received, primarily because it is designed for touch devices, apparently leaving the desktop and laptop behind. However, I’ve been using test versions of Win8 for several months and rarely see the interface formerly called Metro. (I will call it “tiles” in this post as it seems Microsoft hasn’t quite figured out what to call it.)

One of the biggest complaints seems to be that the start menu was removed from the desktop. This is not quite true. The start menu simply has a new look. It’s the tiles. In Win7, if you press the Windows key, the start menu pops up. Guess what it does in Win8…yeah, goes to the tiles. Search does work a bit differently. But it’s still easy. Press Windows Q (think Query), then start typing.

I can tell you, Windows 8 is pretty solid and easy to use, even on a non-touch device. But, why should you move to Windows 8? I can think of a few reasons.

  • When people ask you about Win8, you’ll be able to speak from experience, rather than heresay. This adds to your credibility.
  • You will have customers buying new machines that will be pre-loaded with Windows 8. You should be testing to ensure your applications work properly (hint, they probably will).
  • If provide any type of desktop support, you will need to know how Windows 8 works.
  • Windows 8 is a paradigm change. ARM-based tablets will not have a desktop mode, which means if you want your apps to run on those devices, you’ll have to target WinRT, a new Windows runtime. Windows 8 is required to develop WinRT apps.
  • You’ll want to be ahead of the game for Win9.
  • I can’t tell you the number of developer forum posts I see that say something like “We’re moving from XP to Win7 and my app doesn’t work. Is technology x incompatable?” These problems almost always come down to either a UAC, using Program Files folder for data, or a virtualized registry issue. If the dev would have moved to Vista years ago, at least on his machine, and done 15 minutes of research on changes and what they mean, he would have been aware of the issues. Windows 8 will undoubtedly have some issues and I expect to see “We’re moving from XP to Win8 and my app doesn’t work.” (Along the same lines I often see, “Will technology x work on 64 bit systems.”)
  • You are providing a disservice to your customers if you haven’t tried Win8 and the only way to really try it out is to use it day-in and day-out.

So, get prepared now. Start making a list of what you need to really give Win8 a spin. If you are an MSDN subscriber, you’ll be able to download Win8 next week. For everyone else, it will be available in October. The upgrade cost is cheap. You really have nothing to lose by not trying it except your customers and your reputation.


Integrating FxCop in TeamCity

Static code analysis is a valuable tool to find many errors in your code. What is static code analysis? It’s a process to analyze the code or assemblies themselves rather than testing the application by running the code. Hence, the analysis is static. There are a couple of free tools that allow you to do static code analysis on .Net code. The first, FxCop analyzes the generated assemblies. It checks assemblies for correctness, library design, internationalization and localization, naming conventions, performance, and security issues. The second tool, StyleCop analyzes that actual source code. I’ll discuss it in a future post.

To get FxCop, you need to download and install the Windows SDK. Once that is done, run %ProgramFiles%\Microsoft SDKs\Windows\v7.1\Bin\FXCop\FxCopSetup.exe to install FxCop. You should do this on both your development machine and your build server. You’ll now have an FxCop entry on your start menu. Use it to launch the FxCop GUI.

I won’t go into how to use FxCop or interpret the results. This is will docmented in the FxCop help file and also here and here. You should create a new FxCop project for each Visual Studio solution that you analyze. You should save the FxCop project file with your solution and check it into version control. We put it in the same folder as the Visual Studio solution file.

We also set a few FxCop project options. To so this, right-click on the FxCop project and select Properties.

On the General page, you should enter the name of your project. This keeps things need and tidy. You may be tempted to check Apply style sheet when saving report. However, when you check this option, it will cause TeamCity to not include the analysis report in its HTML output. You can still get to the report, but it will be presented as raw XML.

The next settings we made are on the Spelling & Analysis page. We checked all the options.

Next you need to select the Targets to add. These are the assemblies generated when you build the solution. There is no need to include Microsoft and third-party assemblies. That will generate too much clutter in your results. Just select the assemblies generated as part of your code. Remember that the location of the assemblies may be different on the build server than on your development machine. For example, our build process  does not generate a Debug version, only Release. If you create MVC applications, you can point to the assemblies in the Bin folder. Because this location is saved with the FxCop project, you need to save a location that’s avalailable on the build server. The location saved is relative to the FxCop project file.

When you have everything working, make sure to save your FxCop project file and then check it into your version control system. Now it’s time to setup Team City to run the analysis.

You probably don’t want to run code analysis with every build, especially if you are doing Continuous Integration (CI) with every checkin. Static code analysis is a good candidate for your nightly build. It should also run only after the build successfully completes and unit tests pass.

Modify the TeamCity project and add a build step. Here’s how we have things setup.

The Installation root is where FxCop was installed on the build server. We are using the FxCop project file because this makes it easier to manage. The developer is responsible for their Visual Studio project and by referencing the FxCop project, we don’t have to modify the TeamCity project when assemblies are added.

Now when you run the build in TeamCity, you’ll get FxCop results as part of the build. The TeamCity home page reports the results. When you drill-down into the Code Analysis, you’ll find a complete list of issues that were found.

And there you have it. The basics of running FxCop as part of your TeamCity CI process.


TLCTC and RMTT slides

Last week I spoke at two different events. Friday I was in Minneapolis for the ComponentOne Tech Connection Live! Then on Saturday, I was in Denver for Rocky Mountain Tech Trifecta. Slides from my presentations, Software Gardening and Branches and Merges are Bears, Oh My! are now available at http://www.craigberntson.com/presentations.html. Thanks for attended the events.


When Good Code Intententions Go Bad

What is good code? If it gives the correct result, but takes a convulted route to get there, is the code good? I’ve maintained for years that if the code yields the correct result, the code is right, but it must be performant and written following best practices and coding guidelines to be good.

I’ve recently been doing maintance on an old application. This is a C# WinForms application. It uses Click Once for deployment and all data access is via WCF services connecting to SQL Server on an internal server. The nature of the maintance is to yank out all the WCF and replace with direct SQL calls. (It’s not that WCF is bad, but it’s over kill for this app and it was poorly implemented.) Yesterday, I came across two examples of bad programming. So bad, that I had to share.

Here’s the first. I was looking through the data update code and found a method in a particular class. The first line in the method is:

if (true)

Um.. ok. If anyone can make a case that true can be false, I’ll put this line of code back in.

The second is much worse. It was an overloaded class constructor. One of the overloads had 144 parameters. Yes, you read that right. 144 parameters. It was a class that mapped to a table in the database, each parameter is a field in the database. What did this method do? Saved the value of each field to a property on the object. This code was written pre-Entity Framework, but what’s wrong with using a DataTable and pass that around? Here’s some advice for you. Anytime you get more than four or five parameters, it’s time to rethink the entire structure of the method and maybe even the class itself.

I’m back working on the app today, but I’m almost afraid to go back in. I have no idea what else is lurking in the depths of this code.


Software Gardening Part 5: Seeds

This is part 5 in a series on Software Gardening. If you’re just jumping in, you can find Part 1 here.

Plants start with seeds. We need good seeds to create great softwaseedsre. Our seeds are good OOP practices. Each of the concepts I will talk about could be a single post on their own. If you want to drill into the concepts more (something I encourage), there is lots of information available on the interwebs.

Let’s do a quick review of some basics, starting with inheritance. We can create a class, then subclass that. The first class is the parent and the subclass is the child. The child will inherit attributes (properties) and behavior (methods) from the parent. There are actually two types of inheritance. The first is implementation inheritance. This is where you put functionality into the parent class and the child inherits it. If a method is not defined in the child class, the parent class method is used. You can also override the behavior in the parent so that what you define in the child is what gets run.

The second type of inheritance is interface inheritance, where you simply define some some public members and the signature of each method. The parent has no functionality. The child class must inherit each method in the parent, but is not required to provide functionality in each.

The second basic concept is polymorphism. This is where you can have two things named the same that do different things. For example, the Click method of a command button does something different than the Click method of a radio button.

Next is encapsulation. This means that a class hides it’s internal data from the outside world. It also means that a class data and the methods it uses are defined together.

Now for what I call intermediate OOP concepts. The first of those is loose coupling. What this means is that classes don’t necessarily know about each other. Do you instantiate a class every time you need it or do you pass in a reference to a class or create a static class to make it easier to get to? The less we instantiate one class from another, the more loosely the classes are coupled. And this is good because it makes it easier to update or even completely replace loosely coupled classes.

This brings us to loose coupling’s cousin, tight cohesion, which is a measure of how functionally related the different parts of a class are. You want to strive for everything in a class to be closely related, thus tight cohesion. This is desirable because it results in a class being changed less often because it does fewer things and that leads to fewer bugs.

The last five concepts are considered advanced by many people. But, I think they are more basic than than, but are very important. So important in fact, that they have been called SOLID because the first letter of each principle spells SOLID. They’ve been around for some time, but were compiled together by Robert “Uncle Bob” Martin in his book Agile Principles, Patterns, and Practices in C# (a book that I highly recommend).

The first SOLID principle is the Shgle Responsibility principle. The responsibility of the class is what it does. We’re talking about the behavior of the class. A class should have only one responsibility, it should do one thing. Sometimes it’s difficult to determine what that one thing is. If in doubt, split the class in two. When a class does one thing, there is less chance to introduce bugs because you will modify the class less often.

Next is the Open-Closed principle. This principle says that a class should be open for extension, but closed for modification. Wait! What? How can a class be both open and closed? What this principle says is that you should be able to add new functionality to a class without modifying the source or even the the binary of that class. In .Net, you can do this with partial classes, extension methods, and inheritance.

The L of SOLID is the Liskov Substitution Principle, which tells us that you should be able to substitute a child class for its base class without any of the classes that use it knowing that the substitution was made.

The Interface Segregation principle is next. Over time, an interface will grow as new functionality is added. At some point, the class will become bloated. This principle says that when this happens, you should split the interface into smaller pieces so that the client does not have to rely on methods it doesn’t use. Following this principle makes your code decoupled and easier to maintain, enhance, and extend.

Finally, we’re at the Dependency Inversion principle. This is basically inversion of control (IoC) or dependency injection (DI). When following this principle, you pass in a reference to a class rather than new-ing. It makes it easy to change the class that is used. Just have two classes that inherit from the same interface and pass in the one that  you need. Again, this decouples the classes.

So, in a nutshell, that’s SOLID. By following these principles, your application will be better designed and better able to withstand change.


New Job, New Adventures

On Thursday, November 3rd, I arrived at work at 3M and found a 1/2 hour meeting scheduled with my boss. The subject, “1 on 1 Job Priorities”. I got a bad feeling about it and immediately started looking at her calendar. She had several other 1/2 meetings that morning, all marked “private”. I knew the ax was going to fall. We’d been having layoffs every quarter for a couple of years. Sure enough, when I walked into her office at 10:30, HR was sitting there too. After 10 years at 3M, I was going to be unemployed. I did get a great separation package. Between that and savings, I could keep myself going for several months if need be, but my plan was to be working sooner rather than later.

Fast forward to today. I have accepted an offer from Black Diamond Equipment. I’m very excited about this prospect as they quickly moved to the top of my list as I went through the interview process there. I will be developing C# applications for internal use. My first day will be December 5.

So, what about Mojo Sofware Worx, the consulting company that I started? I’ll keep that for side work. I do have one project with it that I should soon be starting on. I’m just waiting for all the details on it.

So, from layoff to employed, one month and two days. There will be new challenges at the new job, but I’m excited about them. Sometimes we need to get out of our comfort zone to grow. After 10 years of comfort at 3M, it’s time for new growth.


Software Gardening Part 4: Light

This is part 4 in a series on Software Gardening. If you’re just jumping in, you can find Part 1 here.

Light is a critical part of growing any garden. For us as developers, light helps us see things in a new way or with newer technologies or methodologies. At the end of my last post, I said that we neelightd to take the responsibility to do our own learning. So, as developers, our light is Personal Development.

There are two types of skills we need to develop as professional developers. The first is technical. Since you are reading this post, you are already taking a step toward your technical development. Reading blogs is an excellent way to learn about new techniques and skills. Pick a few blogs to follow, set them up in an aggregator, and check them daily. Make a list of topics to follow up on later.

But there is other types of reading in the form of magazines and books. Some will say that hard copy is outdated in this internet age, but I find them invaluable learning tools. Some books give you a quick overview of a new technology while others dig deep into a single topic. Magazines have an advantage over a book in being able to quickly get information about new stuff out to you, but they lack the depth needed for deep understanding.

Podcasts are another excellent form of learning. There are dozens of podcasts that give you broad overviews. One advantage of a podcast is that you can listen to them while on your commute. A few minutes each day is all that you need to learn about something new.

The last place to get technical knowledge is from conferences, user groups, code camps, and training events. Do not overlook the importance of in-person events. The networks you establish will give you contacts when you are looking for your next gig or to find an expert to help solve a particularly nasty issue. Even a Code Camp, that is run by your local community will bring you great topics, friendships, and learning.

The second area of personal development is soft skills. As technologists, we often get caught up in the technology and overlook things like business acumen, public speaking, technical writing, and even things as simple as how to write a good email. It’s these skills that many employers value over our technical ability. But as technologists, we often don’t look to these skills. Whether it’s doing a presentation to our development team or writing an email to the boss’s boss’s boss about our current project, these skills are highly important.

If you don’t have one now, put together a list of topics for your personal study. Set aside some time every week or every day to learn something new. Oh, and remember, this is personal time. It’s your responsibility to keep your skills up to date, not your employer.


Software Gardening Part 3: Water

This is part 3 in a series on Software Gardening. If you’re just jumping in, you can find Part 1 here.

Unless you are growing a cactus, chances are you’ll need lots owaterf water to keep your garden green and growing. In Part 2, I discussed the importance of soil in your garden and said our software soil is Agile methodologies. Now, we add our water, Software Craftsmanship.

The Manifesto for Software Craftsmanship came about when developers felt that Agile was creating applications that the customer wanted, but the quality of the applications was lacking. If you carefully read the Agile Manifesto, you’ll see that is really about project management, not about how to create quality software. So, the Manifesto for Software Craftsmanship was created to support both Agile and quality code. Like its cousin, it has four points of emphasis:

  • Not only working software, but also well-crafted software: Thinking about this, what does it mean to have well-crafted software? It could really be a huge undertaking. The bottom line is to write code in a way that it’s easily testable, extensible, and easy to enhance.
  • Not only responding to change, but also steadily adding value: The Boy Scouts have a rule to always leave your camp cleaner than how you found it. If you apply this to your code, every time you go into it, you’ll leave it cleaner and in better condition than how you found it. After all, how many times will you write a line of code? How many times will you read it? Make code easily readable.
  • Not only individuals and interactions, but also a community of professionals: We can all learn from each other, no matter what technology stack you are using. Share your knowledge with others and hear what they have to say too.
  • Not only customer collaboration, but also productive partnerships: This says it isn’t enough to keep your customer informed. You need to do it in a way that is beneficial and productive for you and them.

It’s interesting that many of the original signers of the Agile Manifesto also signed the Manifesto for Software Craftsmanship. That tell me that they too saw areas that the Agile Manifesto didn’t cover.

So, how do you learn more about Software Craftsmanship? You can join the Google group or find a group in your area. Here in Salt Lake City, we have one. You can also read books that discuss general software development processes and methodologies. I blogged a list of books earlier this year.

I have one more thought about this. Take a look at any Computer Science curriculum or development requirements of many companies. Go ahead, I’ll wait. Did you find anything about the quality of the code? I didn’t find any. This means that many of these quality skills are self-taught. So, get busy. Learn them and put them into practice.

1 Comment more...

Software Gardening Part 2: Soil

This is part 2 in a series on Software Gardening. If you’re just jumping in, you can find Part 1 here.

Perhaps the most important thing you need for your garden to thrive is good soil. Go to Home Depot every spring and you’ll find huge bags of soil for your garden. So, if we’re going to grow software, what kind of soil should we use? If you read Part 1 of this series, you’ll know that one of the drawbacks of comparing software development to construction is that construction uses waterfall project management. So, the answer to our question, of course, is to use the best soil we can. For software, our soil is Agile methodologies.soil

Agile methodologies have been around for a long time, but it was in 2001 that the leading experts pushing these development processes got together to sign the Agile Manifesto. Those leaders agreed on four principles that summed up their different processes:

  • Individuals and interactions over processes and tools: People are the most important part of developing software. It isn’t process or tools. Think about it this way. How many times has a process or tool caused pain on your project? How many times has people issues caused a problem? Keep people first and foremost in mind. Most Agile processes have some sort of period meeting, generally daily, to ascertain where you are on your tasks. It’s a way to keep the people working together and completing the next point.
  • Working software over comprehensive documentation: Aren’t we paid to deliver working software? A stack of documentation does nothing to help the customer, but do not misunderstand the Manifesto, which states, “comprehensive documentation”. This means some documentation is acceptable. Most Agile processes deliver working software to the customer every two to four weeks. And every build should be considered a potential release candidate.
  • Customer collaboration over contract negotiation: Don’t drag on negotiations with the customer. Make the deal, then get to work. Keep the customer informed at each step. If that means having the customer at the daily stand-up, so be it.
  • Responding to change over following a plan: Change happens. Get over it. You should be prepared for it and embrace it. Don’t needlessly put stuff into the project. Every piece of code your write should be tied back to a defined user story and should add value to the software. The nice thing about Agile is that if change to the business environment causes you to change the code, you won’t be months down some path that ends up being a dead end.

Now that we’ve covered Agile at a high level, which flavor of Agile should you choose? I’ve seen studies that indicate that 80-90% of Agile projects today are using Scrum. But other methodologies are effective. I know of a large web-based retailer that uses Extreme Programming (XP) and it is effective for them. The important thing is not which Agile methodology you choose, but that you pick one and use it properly. Many companies say they are Agile, but really aren’t. Get some Agile training and maybe bring in an expert to show you the way.

There you go, you have your soil. You’ve taken the first step to begin software gardening.

1 Comment more...

Copyright © 1996-2010 Developer.Blog();. All rights reserved.
iDream theme by Templates Next | Powered by WordPress