When writing software, we often have to write comments to explain why we did something. I try to write my code such that I don’t have to comment it. In fact, anything other than a temporary (less than a few minutes) comment I write, I consider to mean that I made a mistake.
We could get into endless discussions of when we should comment, what we should comment, and how much we should comment. I’ve had plenty of discussions about comments with people with various opinions on the subject.
An interesting feature of Visual Studio is that it lets you create a list of “tasks” by leaving yourself comments that begin with “TODO” or “HACK”. In some ways I like those comments, because they build into the code reasons for why the code is the way it is.
If you write “TODO”, it tells the reader that the code is incomplete and that wasn’t intentionally left the way it is. There just wasn’t enough time. If you write “HACK”, it admits that you wrote the code badly and it needs to be rewritten.
These types of comments fit in very well with what I tell people about comments. Each one I write and leave in the codebase means that I didn’t write the code as well as I wanted to. Each one is my mistake. The more comments I write, the worse the code is.
The Bad
One codebase that I was working on had a lot of these comments. It seemed they were all over the codebase. There was the other problem that copy-paste coding was rampant, so I am sure that many of these were copied and pasted from previous locations. Heck some of the even said to remove the duplication. The irony? Those comments about removing the duplication were copied with each new instance of the code that was copied.
I checked Visual Studio’s task list of these comments just to see how bad it was.
1131. 1131 tasks that were indicated with “TODO” and “HACK” comments. That takes more than just writing a lot of these comments. That requires writing them and never going back to fix them or complete them.
The Better
Check your codebase right now. Do you have any of these comments lying around? Is it only a handful? If so, you should go try to clean them up. That’s easy and will help you out down the road.
Anytime that you’re going to write a “TODO” comment, think about why you’re putting it there? Is it something that’s really needed? If so, make yourself a note somewhere else. Add a story to go back and make the change later. Get it in the system where you really track your work.
If you know that you’re not going to get to it any time soon, don’t bother with the comment. It will only clutter up your system. If it’s not important enough for you to come back and get it soon, why are you even recording it? If it’s important later, you will be working in that part of the codebase again and you can go back later and fix the code as part of your other work.
Certainly once you get that many of the comments, you should have long earlier realized that there is something that you need to work on. That’s just a ridiculous number of TODO and HACK comments for one codebase.
Has anyone ever see more than that? Anyone think I am crazy or wrong? Tell me about it in the comments!
More Code Audit Nuggets
Keep watching for more interesting nuggets of stuff that I’ve seen in codebases.
In software development, it is common to have users authenticate using usernames and passwords. In order to handle the authentication process, we must store this information in some way. Obvious, passwords should never be stored in clear text. Toward that end, when we store them, we encrypt, hash, salt, etc. to keep them safe.
While these steps cannot protect against all forms of attack, they’re a good base of protection to have in place. We don’t want to just give this information away to anyone who wants it. Yes, some people could take the non-clear-text passwords and obtain the original password from it, but we’ve certainly set the barrier higher.
Security is all about setting more and larger hurdles in place for people to jump over. The only secure information is information that doesn’t exist, and that becomes inherently insecure as soon as it does exist. If information is accessible to someone, it means that it’s possible for someone else to access that information also.
We just try to make things more difficult and limit that damage. Or at least some of us do.
I’ve made plenty of my own mistakes in the past. Some of the applications I wrote in college have security holes, which seem glaring to me now, but I’d not even noticed at the time I wrote the code. I’m sure that most people reading this would say the same thing about their own code.
The Bad
One of the systems I had the pleasure of seeing the code for, at its core, was a customer portal. It even had its own custom ecommerce solution. (These details while talking about security issues will have some of you facepalming already.)
The developers knew that they needed to encrypt the passwords, so they wrote some sql, dbo.encrypt, to encrypt passwords before storing them in the database. Insert a password and out comes a jumbled mess that no one could read.
Users will often forget passwords, and will need some way of recovering from this situation. I recommend resetting passwords when this happens. This application took a different approach.
Also stored in the database was some other useful sql, dbo.decrypt, which would take any of the encrypted passwords and decrypt them. Well that’s obviously a security issue, but the worst thing about this isn’t just that it’s reversible. It’s that this is also in the database. That means that we have the lock and the key together. If someone gets read-only database access, but not access to the application’s source code, they still have everything.
It’s like have a lock and keeping the key inside the lock for convenience. I wouldn’t want to have to call a locksmith if I ever lost the key, so I just keep it here. It makes opening the lock faster too!
The Better
Don’t allow reversing of stored passwords. Use 1-way only when dealing with passwords. This way brute force techniques are required to figure out a password.
If the system can reverse the password back to the original, anyone who has obtained access to the system can retrieve all of the passwords.
This isn’t a new idea. You’re not the first developer to set up authentication in an application. There are plenty of existing authentication systems out there. Don’t reinvent the wheel if you don’t have to. Some systems are safer than others. Ask around. Talk with security experts (I am not one of them) about these if your application requires really high hurdles.
More Code Audit Nuggets
Keep watching for more interesting nuggets of stuff that I’ve seen in codebases.
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.
Enjoy the rest of July! Don’t forget that you should also be avoiding Feature Creep.
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 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.