Brendan Enrick

Daily Software Development

Boy Scout Rule

When and how much time to spend refactoring out code is one of the best questions that a budding software craftsman will ask. It’s one I’ve asked and answered many times. There isn’t one specific answer that is better than all of the others. It always depends on your personal preference, the restrictions of you client, company, or team, your code base, your language, your version control, and many other issues.

One rule, which is the one that I follow and encourage others to follow, is the Boy Scout Rule. It’s based on the “leave the campsite better than you found it” idea. It’s often said to be a line related to the scouts, but it seems to work well for code also.

If you have some old code that needs refactoring, I would guess it’s not tested. Since it’s not tested, you run the risk of creating bugs even if you’re testing it while refactoring. Some small changes will need to be made in order to test the code. You could miss something. That makes it dangerous. This is why you want to keep your refactoring to small pieces at a time. When and where you refactor are the next questions.

The Boy Scout Rule answers these questions nicely. You want to refactor the code you’re working on now. It’s fresh in your mind. You know how it’s supposed to work since you’re in there making changes now. It obviously is a volatile place, which should be cleaned up. You’re in there changing it now aren’t you?

Plus your current change might make things worse if you don’t do a bit of refactoring first!

The 2011 NimblePros Software Craftsmanship Calendar featured the Boy Scout Rule in the month of July, so for the rest of July, try to do small pieces of refactoring as you go into parts of a legacy codebase. Rewrite some code, write a test if you can, update an outdated comment (if you don’t just remove it), or even just write a better variable name.

It’s all about incremental improvements. The agile community should be loving this rule, since it is all about incremental changes and improvements.

Boy Scout Rule

Enjoy the rest of July! Don’t forget that you should also be avoiding Feature Creep.

Security Policies - Code Audits #7

I think we’ve all heard about the great security policies that companies implement.

Companies want you changing your password every other day, without repeating any passwords you’ve ever used, without using any characters from your first name or last name, without having any numbers associated with your address, phone number, birth date, the current year, or your age, and they want your password to be exactly 12 characters.

All of these rules are really funny. I don’t think I need to tell too many people that there are better ways of creating passwords.

All of that aside, those rules are kind of interesting. I use a lot of different passwords. I change the important ones. I don’t change the unimportant ones.

The Bad

When companies implement these policies, they like to have software in place to enforce the policies. Now is a great time to discuss how not to manage these policies.

In this instance the company requires that passwords be changed every 2 months. The passwords cannot contain more than 2 consecutive characters from the person’s first and last name (challenging to avoid as a user creating a password). You cannot use any of your last 3 passwords.

I was going to be rewriting this logic in a new system. I asked a few questions about this. One important questions is, if the password needs to be updated if the person’s name changes. For example they could have gotten married and may not be violating the password not containing consecutive name characters issue. Since I can’t find out what their password is in code, I have to just make them change the password.

This company obviously cares about securing this information since it’s implementing this password policy.

The application is using salted, hashed passwords when storing them in the database. That’s good.

I later noticed how the existing system handled password history. Clear text passwords stored with the username all in a table in the database. Every single password every user had ever used on the site. This is a scary pile of passwords that likely get used all over the Internet by these users. It even included the current password being used by that user.

The Better

These should not be stored this way! If you have to keep passwords around, keep the hash only. Also, get a new salt with each new password. Treat these as passwords (they are!) Don’t be foolishly insecure with this stuff. If someone accessed that database, they would have usernames and passwords for people to try all over the Internet. That would be a huge disaster.

If you have to keep that kind of information around, at least don’t have it in clear text. It would also be a good idea to limit the information that is there by removing the old passwords that are no longer relevant. Since they only care about the last 3 in the history, there is no reason to keep any more than that. Remove the old ones.

To be honest, I also don’t think that those types of password policies are all that secure anyway. I am sure that there is a weak link somewhere in the human element that would render the policy useless anyway.

More Code Audit Nuggets

Keep watching for more interesting nuggets of stuff that I’ve seen in codebases.

Static Overload - Code Audits #6

Static methods are very useful and powerful tools when writing code. As I am sure many people have pointed out, these tools should be used sparingly and cautiously.

Statics are dangerous if they have dependencies. This is because they’re hard to test. Or so I thought. Keep in mind, that just because you can mock statics does not mean that you should be using a lot of statics.

The Bad

I saw a great case of this misuse of statics in some cross-cutting code. It would be a great place to use Aspect Oriented Programming, but was instead a mess of statics and dependencies.

This cross-cutting code was being used throughout the application. There was barely a section of the site that did not use this code. Not once in the code had anyone unit tested any of the code that used these methods. It wasn’t being mocked, so there wasn’t really a way to unit test anything using that code.

Starting things off is a class. As you can probably have guessed from this post’s title, it has a static constructor. In the constructor, it accesses a database and creates an instance of the class. Each of the methods in this static class take in parameters and then write to the file system.

I was trying to write code in this code base, but I could not make any changes yet. I needed to get that monster abstracted so that I could work around it.

The Better

For cross-cutting concerns like this, it’s common to use statics. That’s OK. Not ideal, but OK. You just need to be careful. Try to limit the damage.

If done nicely, the Singleton isn’t horrible. Just keep in mind that tests will need to mock out that singleton when you’re using it.

For this class, I wrote a wrapper around it that implemented an interface. My code depended solely on that interface, and I mocked the interface in my tests. This let me minimize the impact of the change. In the long run, It will be important to make sure that the wrapper gets used throughout the codebase. Anywhere the static was used, it should not depend on the wrapper. This is a quick, easy solution that slowly improves the code.

In my opinion, that’s the best way to clean up any code base. Just fix a piece at a time. Make sure that when you touch any other part of the code that you use that same wrapper and eventually things will start looking a lot better.

More Code Audit Nuggets

Keep watching for more interesting nuggets of stuff that I’ve seen in codebases.

Massive Multiplied Methods - Code Audits #5

I am coining a new term and have just googled to make sure that it’s not written anywhere yet. “Massive Multiplied Methods” did not appear in a Google search at all. I establish that I am likely the first person to post those 3 words together in that order on the Internet.

I am sure that everyone reading my blog knows that you should not repeat yourself and that your code should be organized in small cohesive parts.

Sometimes developers don’t do that. Occasionally, we can find places where people completely ignored those two.

The Bad

Imagine a site whose content comes from editors uploading lists of content stored in spreadsheets. Each spreadsheet, for some reason, has to have only 1 type of content (even though the data all has the same fields).

Now imagine that when the system reads each row from the spreadsheet, it needs to map each value onto a specific record from the database. The data is normalized, so what was a string in the excel file needs to link to the correct other table in the database.

The user selects which type of content is being uploaded and then the system will call the method for uploading that content. Yes, it’s just one method. Any other methods called are part of the .NET Framework. Making things worse, the methods are long. Very long. Each one was about 1,500 lines of code.

There were 5 different types. Each of these types has its own 1,500 line method. That’s about 7,500 lines of code for these.

And the best part about those 5 methods? They’re all identical except for 1 string. Literally, the only difference between these methods is the magic string literal used for the type.

The Better

I am not sure what to say here other than. Go learn about parameters in methods and how to create methods to break an algorithm into logical parts. Wow. Just. Wow.

It’s bad enough that the string could have just been in the spreadsheet to begin with, but did it really require 5 separate methods?

To top it off, you could have just accessed the value from the drop down list the user selected from to indicate the type. This was all in a code behind file anyway, so it could have just grabbed that value. (It shouldn’t have been there, but this would at least be slightly better.)

I don’t know how people let copy and paste operations get that bad. To be honest, I am amazed that the methods still matched. I guess people just avoided changing the code because it was too ugly. (That’s a mistake, because we need to clean up the scary code.)

More Code Audit Nuggets

Keep watching for more interesting nuggets of stuff that I’ve seen in codebases.

Feature Creep

We’ve all been there before. Our project is moving along just fine. The release date is in sight. We’re on track to get everything pushed out right on schedule. Then everything is derailed by a last minute must have new feature. Maybe it’s the differentiator that will raise your product out of the crowd. Perhaps it’s just one small feature that will only push back the release by a day or two. When you’ve finished that one, however, will you have any additional must have features?

This is called Feature Creep. It’s when new features get added to the scope, and often, these features will cause delays in the project.

As a proponent of agile development, I approve of clients adding new features, however, it’s important to understand that adding in a feature should mean replacing a previous feature or you’ll have to push back the release date. It’s important to understand that with many (certainly not all) software projects, you can update the software at some point within the next few months with new features. It’s important to release something to the client. Adding on more and more new features right before a release will just mean a delayed release.

The NimblePros 2012 Software Craftsmanship Anti Patterns calendar for the month of July is about Feature Creep. So for the month of July, make sure that you watch to make sure that your scope is not ever increasing and pushing off release dates. Shipping is a feature, and your product should have that feature also. If you keep pushing off the release date, it will never ship.


Once the airbag, rocket boosters, seat belts, and parachute are added to this bike it will be done. Oh. And an ejector seat. We also need to add spokes, so I can put a baseball card in them. We need a bell too. And a horn. It won’t take long to paint it blue will it? Can we add padding to the seat? That’s the last feature. I promise. Oh I forgot about the mud flaps. Once that’s done we can ship it. As long as there are pegs on the back for passengers.

Hidden Inputs - Code Audits #4

ManBehindCurtainNo user could ever find your “hidden” inputs. You’re a master of stealth and the HTML hidden fields are totally safe and secure for whatever you want to put there. That’s why they’re called “hidden”. Obviously.

Hidden Inputs

In simple terms, these are HTML controls that sit on a page and have no display for users. They’re only “hidden” by not being in plain sight. It’s like hiding something behind a curtain. It makes it easily ignored, but anyone can decide they want to pay attention to that man behind the curtain.

When a user submits a form to your server, it will also post the values of the hidden fields. These are occasionally useful for developers, but are not a place for secure information. You might have an unimportant value you want to maintain.

Never put any information in a hidden input that shouldn’t be seen or modified by a user.

The Bad

One project that I had the pleasure of looking over for someone was a homebrew ecommerce application… Yes, this one is going to get scary.

When you buy something from a site, it will often show you some prices and a total for your order before you checkout. This application had a rather genius location to store the total price that you would be paying. It kept the total in a HiddenInput, so that when the user confirmed the order it would send that up to the server for processing.

I think this was a form of making sure that the same value shown to the user was what they paid. I can only assume that the developer was trying to avoid the values changing in the system and the customer paying a price different from what was shown on the preview.

The danger, however, is that if you want to pay $0.01 for anything on the site, you can just change the hidden input value to 0.01 and then submit the page. Sure it takes someone with some web knowledge to do that, but anymore that’s not a rare thing to know about.

The really cool part is that the system used that value to send to the credit card processor and actually recorded the full detailed list when it logged it. That means that the site logged everything correctly and when it processed the order with the card company it used the amount you specified in the hidden input. Yes, you could totally have ripped them off and their own sites records wouldn’t even show it!

If you’re thinking about trying this on that site, I am sorry to burst your bubble. I told them about that issue and they fixed it.

Less cool than the above example, I’ve seen AccountIds stored in hidden inputs as well as other values that shouldn’t be kept in those locations. I’ve yet to see passwords stored there, but I am sure it’s been done somewhere by someone.

The Better

Make sure to keep all information that requires any level of security or control on the server. Never ever put something like that on the page. Information that should be shown to a user in a read-only fashion should be treated as such. When sent to the client, it should be displayed only. Those values should be left with the client and only information from the server should be trusted.

If you need to store that information somewhere, put it in server memory in some kind of state or store it in the server’s permanent storage.

User sessions are great for storing user-specific information. In the case of the shopping cart, you could use some of that in-memory storage like session or you could store a temporary record in the database of that cart. I am not going to get in the specifics here, but make sure that you get it stored somewhere that users don’t have access to it.

More Code Audit Nuggets

Keep watching for more interesting nuggets of stuff that I’ve seen in codebases.

Massive Classes - Code Audits #3

Today we’re going to take a look at something that I’m sure most of you have seen before.

We’re talking about classes that have grown so large that they’ve earned the size category “massive”. These classes can sometimes house an entire layer of an application. Who would create these monstrosities? Usually it’s a team of malicious, evil developers who are trying to ruin your day. The devised a clever way to make your development experience terrible; they create a massive class and make sure that you end up having to maintain/repair/modify the code.

The real question everyone should be asking is “how big?”

The Bad

I could write all day long about different instances of these that I’ve seen, but instead of boring you, I will only talk about one of the worst offenders I’ve ever seen.

One fantastic code base we inherited was built using a “great” technique for organizing the code. It’s simple approach state that any time you needed to do any data access you could just go to the class named, “MasterDAL”. What made this so great? Well, you always knew where the method you needed would be. What is wrong with that? Well, it was quite common to have 2 or 3 methods that did the same thing in that massive class.

Sometimes you had a few methods that all did the same thing, but each one did it slightly different. The really fun part is that some of them worked and some of them didn’t. It was your job to make sure that you used one that had been updated and had its bugs fixed.

How big was MasterDAL you ask? That file alone was over 50,000 lines long. It was not auto-generated. It was written by developers.

OK, so something I told you so far about this wasn’t true. The code wasn’t really this nice. It gets better. I lied about there being 1 place for everything. MasterDAL was the master data access layer. There were two other data access layers.

Luckily those ones had much more specific names and were not as general or as large as the MasterDal. MiscellaneousDAL and OtherDAL were each only about 20,000 lines long. Obviously the miscellaneous data access and all the other data access go in those. That’s how they’re different from “Master”.

Oh and did I mention that not only did it have repeated code internally, but it also had some methods repeated in the MasterDAL. I know you’re surprised now.

The Better

Please always make sure that you’re organizing in smaller classes. Go read about the Single Responsibility Principle. It will explain that we need to organize our code. We also need to not repeat ourselves quite so much.

There are so many great ways of dealing with data access. Go get yourself an ORM or just use Entity Framework. I don’t care how you do it, but use one of those. Go read about some designs for organizing your data access code. There are so many good options out there. There’s no reason to create these big messes.

More Code Audit Nuggets

Keep watching for more interesting nuggets of stuff that I’ve seen in codebases.

Cookies - Code Audits #2

This is the second post in what I hope to make as a 30 post series of blogs about things I’ve seen in different code bases over my years as a software consultant. I will not be mentioning any client’s names or giving any easy way for people to identify the client whose code base has or did have any of these issues.

My team and I are often brought in on existing projects where the original development team failed to complete the code, created a big ball of mud that made bugs common and features slow to create, or simply where the previous team has completed the work and the customer just wants to know the quality of the code base.


Cookies are a great way for developers to keep track of which client is communicating with them. They can store information in the browser. That sounds great doesn’t it?

The cookie gets an expiration date and is accessible from ASP.NET when the user visits your site. This gives you a chance to look at the cookie and get the information that has been stored in the cookie.

The cookie is able to expire when you want it to and will allow the user to close their browser, come back later, and still have the cookie being identified by your site.

These are most often used a means of identifying a user. When you log in to a site, you will get a cookie, which has a unique identifier allowing the site to identify you during that session.

What should be stored in a cookie?

  • Temporary GUIDs
  • Unimportant information
  • Tracking information that in no way identifies the user

Interesting things found in cookies

You can find all kinds of things getting stored in cookies. Now might be a good time to go looking through your own browser’s cookies to see what things sites have stored there.

Full Address Information

One site had an interesting way of making sure it always had access to your relevant information. It kept it all stored in a cookie. Yes, it had your name, address, phone number, email address, etc. stored in a cookie. I bet you’re really happy to know that the information is being stored in a cookie where pretty much any interested party can access it.

This means that anyone sniffing your traffic would see these values being passed when you navigate around the site. Also, it keeps everything nicely packaged up for consumption by spyware or other malware.

Username and Password Information

Ever wonder how a site lets you “Stay logged in for 30 days”? Well I hope the site you’re using doesn’t do it the same way as this one. One application used a cookie to keep your username and password… Where anyone can get it… and it wasn’t encrypted or anything… Clear text…

I think this one explains itself.

You account number and access rights

When you log in to a site, you get access to your stuff and not other people’s stuff. One site whose codebase I’ve seen kept track of this information by storing the account number in the cookie. (At least it wasn’t in the address bar.) Yes, that’s correct. If you just change the account in your cookie, you can see someone else’s account.

One might expect at least a GUID, so it would be hard to guess the other account number, but one would be wrong. These account numbers were integers starting at the number… 1. Each was incremented sequentially by one.

More Code Audit Nuggets

Keep watching for more interesting nuggets of stuff that I’ve seen in codebases.

URL Parameters - Code Audits #1

This is the first post in a series that I am writing that focuses on some interesting finds from different code bases I’ve seen over my years looking at lots of different software projects. Code Audits are one place where lots of interesting bits of code can be found, since you're combing through an entire software system. I will be explaining what I’ve seen, why it’s wrong, and usually the better approach if it’s not obvious.

URL Parameters

URLs let you control the navigation as your user navigates around your site. Users may not know what URLs mean or how they work, but most people browsing the web know that URLs exist. A good number of them know that modifying that URL will take them to different pages. Even my mother knows that much about URLs.

It’s not just a tech savvy computer programmer or hacker who can figure out that changing the URL might get them to a page they’re trying to see. This is sometimes useful on site’s whose navigation is somewhat lacking. It’s also often possible to guess the page you’re looking for by adding “/login” or some other common word to find a page whose link you’re not seeing.

The Bad

On web sites that have user accounts associated with multiple accounts, it’s quite common for the current account that the user is viewing to be specified in the address bar using a query string parameter. Normally, this means that there will be something along the lines of “?AccountId=123456” on the end of the URL. It’s also very common for those to be built in as “subfolders” like this “/Account/123456/”. Either way, it’s easily visible, modifiable information presented directly to the user.

As I am sure most of you can predict, I am going to point out that I’ve seen some code that didn’t do the proper protection to make sure that only an allowed user can access any given AccountId. The site will only display the links to the allowed accounts, but the site itself does not protect against a user changing that AccountId.

If you want to get to another user’s account, you can just type in another AccountId into the URL and the site will take you there…

The circumstances where these mostly come up is when 1 user account is allowed to have access to more than one account. It’s on these sites that the user will need to be able to change accounts, and sometimes users keep multiple accounts open at the same time, so the URL is needed to keep both open at the same time. It’s important, however, that we maintain at least some level of security here.

The Better

When we build sites, it’s extremely important that we validate inputs given by users to make sure that they’re valid. It’s also important to make sure that users have access to the information they’re requesting.

The first thing you do when you receive user input is to validate that input. Obviously if you consider a URL to be user input, you would validate that immediately as well. This means that either directly or through some layer of abstraction, your code should be verifying a user has access to any information being requested.


There was a big fiasco last year related to Citigroup. ( The rumor is that they had a security breach based on this type of issue. I have no real, credible information about this, but it makes for a good example even if it’s only rumor.

From the New York Times article we get these:

Once inside, they leapfrogged between the accounts of different Citi customers by inserting vari-ous account numbers into a string of text located in the browser’s address bar.

That sure sounds to me like what I just described. If it is what I described, then this security expert stretched the truth quite a bit based on what is in that same article.

One security expert familiar with the investigation wondered how the hackers could have known to breach security by focusing on the vulnerability in the browser. “It would have been hard to prepare for this type of vulnerability,” he said. The security expert insisted on anonymity because the inquiry was at an early stage.

It’s not hard to prepare for that kind of vulnerability. I guess if you have it, it’s hard to track and log the issue. What’s easy is not having the issue to begin with. It’s a simple policy of making sure that all inputs are validated for permissions before being executed. Also, that is not a “vulnerability in the browser”.

More Code Audit Nuggets

Keep watching for more interesting nuggets of stuff that I’ve seen in codebases.

Single Responsibility Principle

Now that we’re in the second half of June, I think it’s time that those of us with one, take a look our 2011 Software Craftsmanship Calendar. In June 2011, the topic was the Single Responsibility Principle, which states that your code should only have one reason to change.

The Single Responsibility Principle is the first in a set of Principles called SOLID,


That looks dangerous…

For the rest of June, please try to follow Single Responsibility Principle and try to not be a Duct Tape Coder. Everyone who has to maintain your code in the future will thank you.