A couple pieces of code came across my desk yesterday and they’re definitely the kind of thing you read about… if nowhere else, then here for sure. This was code that a dev sent me that is to be deployed next week.
The first one could actually cause a problem so I’ll talk about it first.
I’ll write the pseudo-code because I don’t have the actual code in front of me right now. It goes like this:
1. Check if the Whale schema exists.
2. If it does exist then drop it.
3. Create the Whale schema.
Ok, so that’s exactly what the script does. Here, I’ll go ahead and attempt the code. I didn’t look it up so it may have a slight error in it, but you get the idea.
If exists (select name from sys.schemas where name = ‘Whale’)
BEGIN
drop schema Whale
END;
Create schema Whale with authorization = ‘dbo’;
So what’s wrong with this code you ask? It’s simple, here’s the logic.
First, if it exists and you drop it then why just turn around and recreate it in the next step? What’s the point of that? So if your goal is to create the schema then everything above the ‘create’ line is useless. And I know what you’re thinking… so what? What’s the big deal if a tiny query gets run during a deployment? It’s not like it’s going to drag the system down. Well, you’re right about that, but it could kill the deployment. If that schema did exist and it actually contained objects, then the drop statement would fail until you put those objects somewhere else. So with there being no code to check that objects exist inside, you could be dooming the deployment to an unnecessary error. You could also say that you know that the schema doesn’t exist so there’s nothing to worry about. That’s fine, then take out the check and the drop. If you know it’s not there, then take it out. It’s called having concise code and it’s something that we lack in this industry these days. Let me illustrate this with something completely ridiculous that also doesn’t really effect performance.
Create table #t1 (col1 int)
truncate table #t1
truncate table #t1
truncate table #t1
truncate table #t1
truncate table #t1
Insert #t1 Select 1
Ok, so look at that code above. I created a temp table and then truncated it 5x. That’s not going to effect performance at all because there’s no data in it since it was just created and there are no pages. Then I go ahead and insert a row. I can’t think of anyone who would let this kind of thing go on in an SP, or in deployment code, but we’re expected to let useless checks stay in our scripts.
This code was clearly scripted in the wizard so it’s not like the dev went out of his way to do this by hand, but it’s the mark of a fairly inexperience coder to let the wizard create scripts and not inspect what it’s giving you.
The other piece of code doesn’t really do any damage as much as it’s just annoying. In fact, it’s really 2 different scripts. They’re create view scripts and the first one reads something like this:
create view MyView
as
Select MyFavoriteTable.col1,
MyFavoriteTable.col2,
MyFavoriteTable.col3,
MyFavoriteTable.col4,
MyFavoriteTable.col5,
MyFavoriteTable.col6,
MyFavoriteTable.col7,
MyFavoriteTable.col8,
MyFavoriteTable.col9,
MyFavoriteTable.col10
from MyFavoriteTable as MyFavoriteTable
Ok, so even typing this in the blog pisses me off. And again, it’s just a style thing, but this just drives me crazy. What drives me crazy the most is that this dev doesn’t understand what aliases are for. To alias a table with the same name as the table itself defeats the purpose of having the alias in the first place. Here a much better alias would have been mft or mt or m. Hell, you could even go as far as MyFT or something like that. Here, I’ll play one of those out for you and you tell me which one you’d rather read.
Select mft.col1,
mft.col2,
mft.col3,
mft.col4,
mft.col5,
mft.col6,
mft.col7,
mft.col8,
mft.col9,
mft.col10
from MyFavoriteTable as mft
Forget reading, which one of those would you rather type? It’s just more concise and easier to read. I happen to know the dev who wrote this and his opinion is that the full table name makes it easier to read when you’ve got larger joins. Personally, I’ve never known anyone to complain about short aliases before, and again, I honestly think this boils down to inexperience. When I first started coding well over a decade ago, I used to need things to be presented to me in very specific ways too. It was the only way I could read the code. That kind of thing is much less important to me now that I have been doing it for a long time. Why? Because I know how to code.
So the second thing about this script that bugs me is the fact that he saw the need to alias a query with a single table in it. Now you’re just being obstinate for no reason. I know the argument though. He probably would say that he did it for consistency. The only problem with that is the other create view script he submitted didn’t have the same stupid aliasing structure, so where’s the argument now?
Ok, so this code was clearly brought to us as part of a code review so the next logical question is why don’t we just reject the code if it bothers us so badly? Well, the answer is simple. Like so many places, our code reviews are merely perfunctory process placeholders. Our lead dev has made it very clear that he’s going to do whatever he likes no matter what we say. We’ve rejected code plenty of times for really valid process or performance reasons and he always thumbs his nose up at us and pushes it into prod anyway. I really don’t understand how he’s gotten such a superior attitude towards the DBAs, but it’s completely unwarranted. He acts like we’re in this just to piss on his parade and make him look bad, but we’re really only trying to help. But we don’t even bother anymore. What’s the point? He knows more than the rest of us put together so we don’t even bring stuff up anymore.
So that’s my rant for tonight. Remember, use aliases wisely and be as consistent as you can. And for everyone’s sake listen to what your DBAs are telling you for a change. They’re really only trying to help.
