Codeigniter models optimization

Question

I have two tables:

tableA

aid name age sex email password

tableB

bid idno aid

The steps should be

• Enter, verify email and password

• If email and password are correct, insert random number into tableB as idno

• Select name, age, sex, email, idno from both tables and return an array as result.

• If email and password are wrong, do nothing.

I wrote the following code in Codeigniter model

function login($data) {
    $email = $data["email"];
    $password = $data["password"];
    $sql = "select aid from tableA where email='".$email."' and password='".$password."'";
    $query = $this->db->query($sql);
    if ($query->num_rows() > 0) {
        $ret = $query->row();
        $idno = random();
        $aid = $ret->aid;
        $sql = "insert into tableB (idno, aid) values('".$idno."','".$aid."')";
        $query = $this->db->query($sql);
        if ($query) {
            $sql = "select '".$idno."' as idno, name, age, sex, email from tableA where aid='".$aid."'";
            $query = $this->db->query($sql);
            if ($query->num_rows() > 0) {
                return $query->result_array();
            }
            return $query->num_rows();
         } else {
            return false;
         }
         return false;
    }
    return $query->num_rows();
}

The code works, but I think the code is so ugly and not efficient. How can I optimize the code?

Answer 1

The biggest thing I see are the numerous SQL injection vulnerabilities enabled by concatenating variables with SQL together. Use Prepared Statements instead.

You can reduce nesting in code by returning early from the function.

$query = $this->db->query($sql);

if ($query->num_rows() == 0)
    return false;

$sql = "insert into tableB (idno, aid) values('".$idno."','".$aid."')";
$query = $this->db->query($sql);
...

if (!isset($query))
    return false;

$sql = "select '".$idno."' as idno, name, age, sex, email from tableA where aid='".$aid."'";
$query = $this->db->query($sql);

if ($query->num_rows() == 0)
    return false;

return $query->result_array();

Pass the e-mail and password values as explicit parameters, instead of an array. You only need two values from this array. That's not too many arguments for a method:

public function login($email, $password) {
    ...
}

Some code style improvements:

• Put a blank line above the if statements
• Put a blank line above the return statements

Whitespace is not just a useful design element for graphic designers. It is useful for code authors as a means for grouping related statements together, or calling out a statement as something more important than the others. A return statement is pretty important in knowing what this method does. Furthermore blank lines above and below code elements give natural "breaks" in the look of the code, which guides your eye and mind to realize they are related — like paragraphs of text.