Last minute geek

last minute tech news from around the net

Wednesday, Aug 15th

Last update01:00:00 AM

You are here: English WTF CodeSOD: An Incomparable Event

CodeSOD: An Incomparable Event

User Rating: / 0
PoorBest 

Sandra’s ongoing battles continue. She currently works for Initrovent, and is doing her best to clean Karl’s dirty fingerprints off their event-planning codebase.

Now, as it turns out, Karl wasn’t their only developer. Another previous developer was a physicist who knew their way around APL and Fortran, then decided to follow the early 2000s money and became a self-taught web developer.

This Einstein decided to solve some problems, common problems, utility problems, the kind of things you might want to put in your central library and reuse in every project.

For example, do you hate comparisons? Does writing if ($x == $y)… make your skin crawl? Don’t you just wish you could write something like, compareValues($x, $y, '==') instead?

Well, have I got news for you.

/**
 * Compares two operands with an operator without using eval. Throws exception
 * of operator is not found.
 * @param mixed  $op1      Operand 1
 * @param mixed  $op2      Operand 2
 * @param string $operator Operator (==, !=, <=, >=)
 * @return bool
 */
function compareValues($op1, $op2, $operator) {
    switch ($operator) {
        case '==': return $op1 == $op2;
        case '!=': return $op1 != $op2;
        case '>=': return $op1 >= $op2;
        case '<=': return $op1 <= $op2;
        default:
            throw new Exception('Operator is not handled: ' . $operator);
    }
}

Now, this particular snippet isn’t the worst thing on Earth, and it actually has a potential use- you might need to decide which operator to compare using, and store it in a variable.

What we really need is a way to route around PHP’s type system. This method does exactly that, but instead of being placed in a central library, it’s copy/pasted in five different places across multiple projects with slightly differing implementations.

    function compareValues($data1, $operator, $data2, $type) {
        if (!$operator)
            return false;
        if ($operator == '=')
            $operator = '==';
        switch ($type) {
            case 'date':
                if (is_array($data1)) {
                    $data1 = $data1['year'] . "-" .
                            $data1['month'] . "-" . $data1['day'];
                }
                if (is_array($data2)) {
                    $data2 = $data2['year'] . "-" .
                            $data2['month'] . "-" . $data2['day'];
                }

                $data1 = strtotime($data1);
                $data2 = strtotime($data2);
                if (empty($data1))
                    return 'empty';
                if (empty($data2))
                    return 'empty';
                return eval('return (' . $data1 . $operator . $data2 . ');');
                break;
            case 'time':
                if (empty($data1))
                    return 'empty';
                if (empty($data2))
                    return 'empty';
                $data1 = (int) str_replace(':', '', $data1); //removing ':'
                $data2 = (int) str_replace(':', '', $data2);
                return eval('return (' . $data1 . $operator . $data2 . ');');
                break;
            case 'money':
                if (empty($data2) || $data2 == 'null') {
                    return 'empty';
                }
                if ($this->options['Request_currency'] != $this->options['Proposal_currency']) {
                    return false;
                }
            default:
                if (empty($data1) && $data1 !== 0)
                    return 'empty';
                if (empty($data2) && $data2 !== 0)
                    return 'epmty';
                if (!is_numeric($data1))
                    $data1 = ''' . $data1 . ''';
                if (is_string($data2))
                    $data2 = ''' . $data2 . ''';
                return eval('return (' . $data1 . $operator . $data2 . ');');
        }
    }

What a nice bit of defensive programming, ensuring that there’s a valid operator. And what a lovely pile of confusing code that may return true, false or FILE_NOT_FOUND empty. Or, sometimes, “epmty”. Bonus points for doing some of the comparisons using eval, thus allowing a code-injection attack. Einstein must have recognized this was bad, because some of the copy/pasted versions of this method instead called the first version of compareValues to avoid using eval.

But I know what you’re thinking. You’re thinking, “This is great stuff, but what good is having in PHP? This is 2018, and we’re writing operating systems in JavaScript now.” Well, don’t worry. This same method has a JavaScript version.

/**
 * compares two values
 *
 *  @fieldId
 *  @operator
 *  @compareData
 *
 */

function compareValues(data,operator,compareData,type,fieldId){
    if(!operator) return;
    if((data == "" || data == null) && (compareData != "" && compareData != null))
        good = false; //when the data is im
    else{
        switch(type){
            case 'date':
                //changing the values to numeric strings
                if(data.constructor == Array){
                    date1 = String(data[0])+String(data[1])+String(data[2]);
                }else{
                    date_arr = String(data).split("-");
                    if(date_arr[0].length == 2){ //swapping the order of the date-string (in case it's wrong)
                        date1 = String(date_arr[2]+date_arr[1]+date_arr[0]);
                    }else
                        date1 = String(data).replace(/-/g,"");
                }
                if(compareData.constructor == Array){
                    date2 = String(compareData[0])+String(compareData[1])+String(compareData[2]);
                }else{
                    date_arr = String(compareData).split("-");
                    if(date_arr[0].length == 2){ //swapping the order of the date-string (in case it's wrong)
                        date2 = String(date_arr[2]+date_arr[1]+date_arr[0]);
                    }else
                        date2 = String(compareData).replace(/-/g,"");
                }
                if(date1.length != 8 || date2.length != 8) //8 is the length of the number 'yyyymmdd'
                    return;
                good = eval(date1+operator+date2);

                break;
            case 'money':
                if (window.Request_currency != window.Proposal_currency)  {
                    good = false;
                    break;
                }
            default:
                if(IsNumeric(data)){
                    good = eval(data+operator+compareData);
                }else {
                    // Compare as strings
                    good = eval('''+data+'' '+operator+ ' ''+
                        compareData+''');
                }
        }
    }
    if(good){
        $('#'+fieldId+'_ico').addClass('icons__check');
        $('#'+fieldId+'_ico').removeClass('icons__caution_sign');
    }else{
        $('#'+fieldId+'_ico').addClass('icons__caution_sign');
        $('#'+fieldId+'_ico').removeClass('icons__check');
    }
}

Like all good spaghetti code, this one is topped with a sauce of global(window) variables, and mixes logic with DOM/UI manipulations. And thank goodness for all those regexes.

The JavaScript version is attempting to create good from eval, which never works, and it breaks the D&D alignment chart.

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!

Read all
Comment Policy:
We pre-moderate any comments and welcome all kinds of thoughts, supportive, dissenting, critical or otherwise. We delete or censor comments that are:

* abusive
* off-topic
* contain personal attacks, or against any company or organization
* promote hate of any kind
* use excessively foul language
* is blatantly spam or advertising

We do not discriminate based on the person who is posting, and we never censor comments for political or ideological reasons. We never delete an appropriate comment because we disagree with its viewpoint or ideology, and we never publish an inappropriate comment because we agree with or support its viewpoint or ideology.


Attention spammers: we manually approve all comments. Spamming and blatant advertising will NOT be published on this site and is deleted immediately, you've been warned, do not waste your time here.

Add comment

Security code
Refresh