davideairaghi / php

repository of php code
Apache License 2.0
12 stars 9 forks source link

reducing cyclomatic complexity #3

Open noodlesegg opened 6 years ago

davideairaghi commented 6 years ago

1. library/Airaghi/PhalconPHP/MSSQL/Adapter/Mssql.php

Changing code from:

if (is_string($wildcard)) {
      $parameter = $wildcard;
} else {
     throw new \Phalcon\Db\Exception("Invalid bind parameter (#1)");
}

to:

if (is_string($wildcard)) {
    $parameter = $wildcard;
}
throw new \Phalcon\Db\Exception("Invalid bind parameter (#1)");

will throw an exception regardless of $wildcard being a string or not. Why are you suggesting this change ?

2. library/Airaghi/PhalconPHP/MSSQL/Dialect/Mssql.php

Changing code from:

$sqlWhere = '';
if (isset($definition['where'])) {
    $whereConditions = $definition['where'];
    if (is_array($whereConditions)) {
          $sqlWhere .= " WHERE " . $this->getSqlExpression($whereConditions, $escapeChar);
    } else {
        $sqlWhere .= " WHERE " . $whereConditions;
    }
}

to:

$sqlWhere = '';
if (isset($definition['where'])) {
    $whereConditions = $definition['where'];
    $sqlWhere .= " WHERE " . $whereConditions;
    if (is_array($whereConditions)) {
          $sqlWhere .= " WHERE " . $this->getSqlExpression($whereConditions, $escapeChar);
    }
}

could lead to an invalid value in $sqlWhere, you could end with a "double" "WHERE ..." condition inside the string and/or with an array concatenated to the string... am i missing something in your proposal ?

noodlesegg commented 6 years ago

many thanks for the review @davideairaghi , good spot! necessary changes has been committed. thanks

davideairaghi commented 6 years ago

FILE : library/Airaghi/PhalconPHP/MSSQL/Dialect/Mssql.php

Changing code from:

$sqlWhere = '';
if (isset($definition['where'])) {
    $whereConditions = $definition['where'];
    if (is_array($whereConditions)) {
        $sqlWhere .= " WHERE " . $this->getSqlExpression($whereConditions, $escapeChar);
    } else {
        $sqlWhere .= " WHERE " . $whereConditions;
    }
}

to:

if (isset($definition['where'])) {
    $whereConditions = $definition['where'];
    $sqlWhere = " WHERE " . $whereConditions;
    if (is_array($whereConditions)) {
        $sqlWhere = " WHERE " . $this->getSqlExpression($whereConditions, $escapeChar);
    }
}

Still presents chances of falling into an "array to string" conversion. Maybe the correct code should be:

if (isset($definition['where'])) {
    $whereConditions = $definition['where'];
    $sqlWhere = " WHERE " . (is_array($whereConditions) ? $this->getSqlExpression($whereConditions, $escapeChar) : $whereConditions);
}

What do you think about it ?