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.
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.
No 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.
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.
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
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.
