Job Interviews : The Lassam Test
July 4th, 2009As a young developer, I’ve attended more than my share of job interviews. They seem always to end up the same way. You get the “HR Questions” – a series of questions about your future aspirations, who you are, basically just talking about yourself at whomever is interviewing you.
Inexperienced companies will hire you after the “HR Questions”, without any sort of verification of your technical abilities.
More experienced companies, however, will ask you the “Tech Questions.” The tech questions are a series of knowledge questions about various technologies. If they’re well chosen, they’re a great way to filter people who know what they’re doing from people who are utterly clueless.
Well selected questions can be a great boon when it comes to gauging the technical competency of workers when it comes to a specific technology. If you claim to have 3 years of experience with SQL, and you do not understand what a CREATE statement is, or what relational integrity is, you can be dismissed off-hand as a goon.
Technical knowledge in a specific domain, however, is not the be-all and end-all of programming. Many “domain experts” can write code that is downright ugly.
Thus, I demonstrate the Lassam Test — the point of which is to gauge ‘programming instinct’. It’s hard to describe what ‘programming instinct’ is, but it’s that part of every programmer that can successfully identify code smells and fix them.
The Lassam Test is as follows: Perform a code review. Pretend that you are a co-worker who will be responsible for maintaining the code, and you have been tasked with reviewing the following code snippet.
Highlight problems and suggest solutions.
function display_stuff()
{
print "<ul>"
$conn = mysql_connect( "mysql.foo.org:412", "skookum", "tinydancer" );
mysql_select_db( "skookum", $conn ); // selects a db
$query = " SELECT * FROM main WHERE id > " . $_GET["id"]. ";";
$results = mysql_query( $query, $conn);
while( $row = mysql_fetch_assoc($result) )
{
print "<li>".$row['description']."</li>";
}
print "</ul><br /><ul>";
$query = " SELECT * FROM main WHERE id < " . $_GET["id"]. ";";
$results = mysql_query( $query, $conn);
while( $row = mysql_fetch_assoc($result) )
{
print "<li>".$row['description']."</li>";
}
print "</ul>";
}
Now, the code I've presented is literally swimming with bad design decisions and code smells.
Let's begin.
-
display_stuffis the second worst possible name for this function. It doesn't tell us anything about what the function does, or why. - There's also no function-level comment to clarify what task this function performs. The only way to truly know what's going on is to peer into the function.
- This function combines display, logic, and database code. It's essentially a big wad of loose logic.
- The database connection details are included as hard-coded values. This suggests that the programmer has been doing this with all of his database code - which means that if said programmer wants to change the database server location, or password, or database name, he is going to need to change the connection details every time they're used in the code.
- Let's not forget that the database API is being directly used in the code, in such a way that switching from, say, mysql to postgresql throughout this codebase would be an utter nightmare.
- The mysql database connection is not checked. What happens if it fails?
-
//selects a dbas a comment right next to the mysql_select_db function is pretty darned useless. - The query allows data from the $_GET field, from the user, unfiltered, straight into the database query. This is quite possibly the most dangerous part of the entire script, as a single little Bobby Tables could take this coder's database down in a flash.
- The SQL query selects "*" from main, when all it seems to be using is the 'description' field. This is inefficient - and it's harder to tell what sort of data will be returned by a query when it is not made explicit as part of the request.
- The results from the SQL query are not checked. What happens if it fails?
There's more, much more, wrong with this code snippet, much of which is related to it's fundamental lack of architecture. It's brittle code, and it will break at the first sign of trouble or change. It's also a strong warning sign of a brittle system, one that will, once again, break at the first sign of trouble or change.
My ultimate recommendation here would be aggressive refactoring of the system to eliminate hard-coded variables, separate display logic from database logic, and include much more robust error-checking and management - with an eye for more descriptive function names & comments.
This is the underlying concept of the Lassam Test — can your potential hire identify bad code, when he sees it? Does he see deep structural problems, or does he dwell unnecessarily on trivialities such as brace-style?
It's not hard to construct additional examples — just take best-practices and violate them horribly. Program unsafely. Fail to anticipate errors or changes in the code. Nest 10 deep, and never use a loop when brute force copy-pasting will do. Construct massive object hierarchies with confusing and contradictory inheritance rules, and then break base-class functionality in child-classes. Use functional programming, but use it wrong. Be exceedingly clever, even when you don't have to and nobody wants you to. Name your variables after favourite childhood pets, letters, or incomprehensible acronyms. Resist modularization, construct god-objects, and keep large blocks of commented-out code around forever. Learn how to write unmaintainable code, and embrace every anti-pattern you can get your hands on. I can keep going, on and on, and on and on and on. There are as many ways to screw up code as there are stars in the sky.
If a potential hire can take a red pen, go over the code, and show you what needs to change, why, and how, you've got a keeper on your hands. I'll admit that calling this The Lassam Test demonstrates an awful lot of hubris, but — well, hell, I just love putting my name on everything. I call that The Lassam Nomenclature System. .
Breaking small/simple functions into pieces to fit into MVC (or whatever) can add unnecessary complication to simple code. There are legitimate reasons to do it, splitting off the DB query into its own function means you won’t have to duplicate error checking code throughout the codebase… but doing it just so it fits into someone’s programming ideal is kind of silly, especially when the code is trivial.
DB abstraction just for abstraction’s sake is a recipe for bloat and bugs. Most projects will only ever be used with a single database, and if you migrate there are likely bigger concerns. If you restrict yourself to the lowest common denominator in terms of DB features, you’re not going to have great results. Non-portable features like CALC_FOUND_ROWS in MySQL come to mind. Switching DBs (with few exceptions) is generally a very good indicator that you’re doing something fundamentally wrong.
One potential problem you didn’t mention is escaping data coming FROM the database. Based on the code there’s no way to know what could be in the description field but it’s possible it might contain characters that could be used maliciously in output. So, yeah, potential failure there too.
What I liked doing when giving interviews was giving open-ended questions: “You have this problem and these constraints, how would you solve it?” Whether their answer is right or wrong isn’t particularly important, what you’re looking at is their thought process. If it’s wrong, give them a prod in the right direction and see if they take the bait. Computer skills can be taught, critical thinking skills not so much… and the ability to think quickly on your feet is important.
We’re assuming that this code is part of a much larger application. As a one-off script – an application that fits in a single page, for example – going FULL ON MVC would be a silly idea. For a larger application, doing everything with copy-pasted database-connection, query, and error checking logic everywhere is a huge waste of everybody’s time.
Insulating the code from the DB logic hurts nobody. In any app, there are database calls that you will make frequently, and it is better, clearer, and more maintainable to toss in a db.getUser(‘classam’), then, say, 12 lines of database connection, query, input escape, output escape and retrieval code.
On top of that, thanks to the fact that ‘db.getUser’ just expresses what you want, instead of how you plan to get it, if you need to switch to a different DB, you can produce a database class with the same interface, but different connection & query code, and swap the two as necessary without worrying about having to use ‘lowest common denominator’ database functionality.
A lot of libraries, frameworks, and software packages can be used with multiple databases. If you completely control your roll-out environment, and you’re the only administrator of your software, it’s not a problem, but that’s not always in your hands.
Even if you are just using a single, controlled database, it’s convenient to have all of your DB code in the same place. One chunk of connection code, and a series of queries with data handling code.
Er.. yeah. I put that bug in on purpose, but then got so carried away with ‘not-escaping’ as a bug that I thought I’d put it in the input, too, and forgot about the output bug.
I’m not really arguing against splitting off common functions as much as I am needlessly separating logic and layout. I should also make it clear that most of my objection here is because we’re dealing with PHP, which was pretty much designed NOT to have logic and display split. Perl, Ruby, Java, whatever would be a different story.
Duplicate code should of course be broken into its own function.
>> On top of that, thanks to the fact that ‘db.getUser’ just expresses what you want, instead of how you plan to get it, if you need to switch to a different DB, you can produce a database class with the same interface, but different connection & query code, and swap the two as necessary without worrying about having to use ‘lowest common denominator’ database functionality.
Maybe. Sometimes you can get away with that (ie you just want to get a user), but what if you want to get more creative with joins on different conditions? What if you build your query differently depending on user input? A search page is the first thing that comes to mind.
You should have common validation code (class, functions, whatever floats your boat), and some common DB code (connect, a generic query class, etc) but I don’t think you should necessarily completely abstract it away.
$DB->Query(“SELECT hurf FROM durf WHERE …”, …, …);
Makes sense too. It gives you flexibility in the query. It gives you a common place for error checking (Are we connected? Are these parameters escaped? Parameter binding etc.) It gives you a convenient place to tie in other functions… say some day down the road you realize you’re thrashing the DB and want to tie in memcached or something…
Abstraction usually comes at a significant performance hit too. Adodb is dreadful, some are even worse… Pear DB I’m looking at you… You can get some of the benefits with what I mentioned above (common connect/generic query code would at least let you replace mysql_connect with pgsql_connect or whatever.) The performance hit won’t matter for all applications, but I’ve participated on some where it did add unacceptable load.
That’s true, but the phrase “jack of all trades, master of none” comes to mind. Gallery2 is going to be my whipping boy here because it does a pretty great example of making my point. Well, two of them really.
One, the code was overengineered to the point that while you could consider it elegant in some ways, it was dog slow and nobody actually understood it. Bringing people up to speed was a nightmare.
Two, for Gallery3 they’re moving away from “Let’s support every database”, targetting specifically MySQL and… I think MSSQL… since those are the most common target DBs, and because they had nobody experienced enough with the others to make sure their code was correct and not generally awful.
Most apps are already going to make arbitrary decisions like not supporting PHP4, or not supporting anything before PHP 5.2 … at a certain point it’s okay to list requirements. You must be this tall to ride.
I don’t think separating logic and layout is ever needless, unless either one or the other is trivial. Logic is ugly, layout is ugly, and when you mix them together, what you get is doubly ugly.
Now, I can understand maligning templates – PHP makes a fine template language on it’s own, and much faster – but the underlying concept of ‘hey, let’s keep the logic away from the shiny bits’ seems sound enough to me.
$DB->Query(”SELECT hurf FROM durf WHERE …”, …, …);
Makes sense too. It gives you flexibility in the query. It gives you a common place for error checking (Are we connected? Are these parameters escaped? Parameter binding etc.) It gives you a convenient place to tie in other functions… say some day down the road you realize you’re thrashing the DB and want to tie in memcached or something…
So, once you’ve got a tidy DB object that performs escaping, error checking, error logging, possible caching, blah blah, how does it hurt to go …
class Queries
{
function getHurf( )
{
return $DB->Query(”SELECT hurf FROM durf WHERE …”, …, …);
}
}
You get a tidy place to keep all of your queries, a little bit of self-documentation, and you’re prevented from having the same query appear more than once in the same code. If you need to change a query – or check all of your queries, you know where to look.
I hate to break the train of criticism/serious discussion about the Lassam Test, but I have to do some self-toting and brag that I got a pretty good score on this test. I found most things that were pointed out, plus one or two others. Yay me.
Also, I will be a little more serious and agree to the early point about the flawed hiring process used by many companies who don’t actually test the abilities of the applicants. Of course, this extends far past the tech industry. It’s also not just small town companies that err on this level. Too many times I’ve seen resumes of friends, co-workers, employees, and myself that claim skills that are limited, if not fabricated completely.
To put myself in the seat of the employer, I understand that interviews are stressful and cause anxiety among most applicants, and as such I can’t completely rule out somebody’s abilities completely based on their ability to spot somewhat trivial code flaws. An applicant’s inability to perform well in interview should however, throw up a red flag to signal that further evaluation may be necessary.
“Wow Mr. Smith, you know 12 different languages. I assume you know how to traverse a BST in all of them?
>Sorry, what’s a BST?
…A binary search tree…
>Uh…sorry, I’m not familiar with that, and actually, what do you mean by ‘traverse’?”
At the very least, a situation like this should be played out in interview, because more often than I like to imagine, this is the type of person that gets hired.
A more personal experience is the current situation of two friends looking for co-op jobs. They both applied with a popular co-op employer, one who offers connections and looks great on a resume. One has a rather humble resume that greatly exaggerates his abilities, while the other hosts a more modest resume that still exaggerates some points, but to a much lesser degree. The former was hired almost immediately, without any test of actual knowledge, while the other has likely been rejected because he didn’t claim to know Python, solely on the grounds of being able to program Hello World with simple inputs and outputs. As far as the job goes, the latter would be a much better option, even if it deals entirely with Python.
I just drop trow and ask them if they think they’re a bigger man than I.
Ok, really, I’ve discovered that once you’ve worked a bit and kicked ass without chewing gum you never have to do a real interview again. When you do, they’re funny ’cause you know they’re next to meaningless.
Dave:
The ability to traverse a BST is very important for a computing scientist. However, for a coder, I would venture a guess that it is basically useless knowledge.
And this is why a lot of BCIT grads get coding jobs more easily than SFU grads.
(see my upcoming as-of-yet-unwritten-but-fermenting-in-my-brain article title What is Computing Science in the as-of-yet-non-existing CSSS Newsletter!
Phil
Antipode – Article – Feeds worth reading says:
[...] blessed with many friends who write about technology. Forced to pick a top two, I’d name Curtis Lassam and Bruce Alderson. Both write well on both web technology and creative topics, and I think of [...]