Mar 11, 2013

Bad coding

Never code like this:

function hasContentElements($pageId) {
    $rows = $GLOBALS['TYPO3_DB']->exec_SELECTgetRows('*',
       'pages', 'uid=' . $pageId ...);
    return $rows;
}

function processContentElements($contentElements) {
    foreach ($contentElements as $contentElement) {
        ...
    } 
}

$hasContentElements = hasContentElements($pageId);
if ($hasContentElements) {
    processContentElements($hasContentElements);
}

Why is this bad? Because:
  • Function name hasContentElements suggests that it only checks that the page has content elements. It should not retrieve them. Imagine if there are 100 elements on the page. How much memory would it take just for a simple check like that?
  • The variable is named improperly too. "has" means a boolean value!
  • Using the variable with such name is misleading here. Unless you see and remember what the function does, you can make wrong assumptions about the logic of the processContentElements function.
When coding, please, choose proper names for all your code elements. If you have to check for existence of database records, do not fetch them, use COUNT(*) instead. That makes a great difference in terms of performance and memory usage.

No comments:

Post a Comment