Upgrade Your Drupal Skills

We trained 1,000+ Drupal Developers over the last decade.

See Advanced Courses NAH, I know Enough

Code that makes Programmers Perform

Parent Feed: 

Code that performs well should be an assumption, not a requirement. I've never had a client ask me, “will your code make my site run slower?" Clients just assume that I'm going to deliver a codebase that does not hold things up.

However, over the years, I've discovered that code can actually have a major impact not just on how well the website runs, but also on how  much work a coder can get done. And here is the big surprise: a lot of the code I review for others, or sometimes code that I wrote years ago myself, doesn't allow me to work as quickly as I (or the client) would like. It actually sometimes slows me down so much that, by the end of the day, it would have been quicker for me to just rewrite it from scratch, using efficient code and accepted coding standards.

The background to this post

I was recently in contact with a client that had a website which was performing poorly - a pretty standard Drupal 7 website (which somehow took over a year to develop). When the site was eventually delivered, it was so full of bugs that the original development company couldn't (or wouldn't) fix it. The task was clear: make the site run faster (a normal content page would easily take over 10 seconds to load) and fix as many bugs as you can within three days.

Before meeting on site with the client, I got a copy of the codebase and began to make myself familiar with it. Oh man. This already looked monumental. I got scared. Scared because I wasn't sure that I could touch anything on the site without making it fall apart, or making things even worse, or conflicting with another piece of code somewhere else. Or ...?

I'm going to give you a couple of examples of what the client and I decided to do, along with some advice I'd like to give to the original developers if I had them sitting next to me. There will be more in future parts of this series.

Files and folders

There was basically no file/folder structure to speak of. Almost every module was placed in a flat hierarchy, except for some custom modules and the odd (unused) feature. So there was approximately 150 folders in sites/all/modules. But I had no way of knowing if they were patched contrib modules, a custom module, or a sandbox module. They could have been anything. Updating modules was a matter of convincing ourselves that the site would probably break somewhere, and we could take it from there.

I suggested that we left the structure as it was for now, but that they should consider creating a structure that looked similar to this (basic Drupal good practice):

drupal-root-folder
- sites
- - all
- - - libraries
- - - modules
- - - - contrib
- - - - contrib-dev
- - - - contrib-patched
- - - - custom
- - - - custom/features
- - - themes

If a module is patched, put the patch in the module folder, so it can be re-applied if needed when next updating that module. This will also make any new developers that come to the project aware that they should pay extra attention to this module. It's also good practice to contribute the patch back to the community if you think other projects might benefit from it.

Warnings and notices

I hate warning and notices. On this website, they were everywhere. Hundreds of warnings and notices on every page. The former programmer/site builder's solution to this issue appears to have been: turn off message logging. I know that makes sense in a production environment, so you don't expose a weakness to a potential hacker of the site, but it does not solve the underlying problem. To me, it just looked like a developer trying to fool a client, and ensure that, though the site had major issues, the client wouldn't be aware of them.

At one point it was so bad that the customer had to turn off the logging system to help the database that was being overloaded with error logs. However, this meant that if there was a problem - such as the site getting hacked - they couldn't do any meaningful debugging because there was no error log to check.

The solution was pretty simple: start from the homepage and fix all the code that was throwing warnings and errors. Next move on to a sub page, and so on. I spent long, long hours on this, but the work paid off in the end: we got the amount of warnings down to a level where it was possible to have the logging system running again so we could get some real debugging done.

But I still couldn't touch the logic in the code, because I still didn't have a clue what most of it was doing.

Indentations and variables

It is obvious to many programmers that coding standards matter. In this case the developers didn't care about correct indentations, or any other coding standards for that matter. This is an example of what a typical piece of code on that site looked like:

        case 'tasks_node_form':
          $key_value = array_keys($form['field_task_listing_ref']['und']['#options']);
          $ff = array_shift($key_value);
          $new_arr['_none'] = '- None -';
          foreach($key_value as $val)
          {

              $inode = node_load($val);

              //$new_arr[$val] = $inode->title.' -> '.second_mlid_title($val);
              // calling functtion to get complete listing (added by Bob the Builder)
      if(get_complete_listing($val) != ''){
              $new_arr[$val] = $inode->title.' -> '.get_complete_listing($val);
      }
          }
          $form['field_task_listing_ref']['und']['#options'] = $new_arr;
          $form['#after_build'][] = 'custom_after_build';
          $form['#validate'][1] = 'lets_compare_date';

      foreach ($form['field_task_elements']['und']['#options'] as &$option) {
            $option = decode_entities($option);
        }
      foreach ($form['field_section']['und']['#options'] as &$option) {
            $option = decode_entities($option);
        }


     break;

No, there's nothing wrong with your screen, this is how the code was presented when I saw it for the first time. It's almost impossible to read and understand when you have 2000 lines of code looking like that.

But, look carefully and you'll also see a XSS security issue.

You can read more about how you should indent your code (and other coding standards) here.

Comments

The code contained comments. Really, it did. But there was no point to them. They didn't help me to understand what was going on at all. Comments like, "Loop through array", "Call function that cleans up" and "Returning the array". And one that really scared me: "If the alias already exists, generate a new, hopefully unique, variant". We are hoping for a new variant? Why not just make sure it's unique?

The rest of the comments were primarily just bodies lying around (dead, unreadable or commented code). No tombstones (comments, telling why the code is commented), so I could't tell if the code had a reason to stay there or not.

Here are some general rules when commenting code:

  • Don't ever leave commented code in your code base, unless you write a comment above it, telling why this code should stay in there.

    • A "we might need this later" comment isn't good enough. That's what your SCM (Source Control Management, like git or SVN) is for. You can always go back in your scm's history, and resurrect your code from there.

  • Don't state the obvious.

    • I know a loop when I see it. I can read the return statement, there is no need to comment it. I want to know why you are doing something, not what is being done. Don't tell me that you called the clean-up function. Tell me why we need to clean something up.

  • In general, don't comment variables.

    • It's way better to give the variable a descriptive name. If you can't tell from the name of the variable what it contains, you should consider renaming it.

    • Of course, there can be situations where a variable can hold an unspecified data type or unspecified content, and here it is important to make sure that you write a comment saying that.

  • The most important comments in the code are the DocBlock (documentation block) comments.

    • These are the comments that are before EVERY function throughout your code. Don't EVER create a function, without creating a good DocBlock. They are not hard to write. Read more about them here.

    • A good DocBlock doesn't take long too write. If it does, you probably did something else wrong, like your function is doing too much. Try to split it into smaller more specific functions.

    • The first line of the DocBlock is an especially good indicator of whether your function is trying to do too much. The first line must contain a summary of what your function is doing. And it's a one-liner only (80 charecters max). If it takes more than that to write a good summary, consider splitting the function.

    • After the first line, you leave an empty line, and after that again, you can add descriptive, in-depth explanations.

    • Lastly remember to describe all the parameters and the return value as well, using @param and @return.

  • Keep in mind that the DocBlock design that I describe above isn't necessary when you are writing hook functions.

Drupal Coding Standards

Drupal's Coding Standards are there for a reason. They weren't created for fun, or to make text look pretty.

When you return to a broken or unfinished module, or when you read unfamiliar code, and the code is strictly written to comply to the standards, you know how to read it straight away. You know what to search for, because the variables are in the same format. You know what every single function is doing, because it's explained in the comment above the functions. You know what data type a function's parameters expects, and what every function returns.

That's what allows the developer to perform - to get work done quicker. In the long-term, it's a money saver.

Conclusion

The site was FUBAR BUNDY.

What I suggested to the client was something like "Lets get this patched up, so it can last at least a while longer, and then get something new built."

I suggested that they should build the new version themselves as they were now familiar with Drupal and how to code Drupal modules. They could then just get help from consultants like Annertech to help with any tricky aspects that might crop up.

I'd like some help from Annertech

Original Post: 

About Drupal Sun

Drupal Sun is an Evolving Web project. It allows you to:

  • Do full-text search on all the articles in Drupal Planet (thanks to Apache Solr)
  • Facet based on tags, author, or feed
  • Flip through articles quickly (with j/k or arrow keys) to find what you're interested in
  • View the entire article text inline, or in the context of the site where it was created

See the blog post at Evolving Web

Evolving Web