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?

• mickmackusa: Where are your prepared statements or Codeigniter query building methods?
• KIKO Software: 1. The function is called login(), but it doesn't seem do what it says on the label. 2. You could get rid of the third query if you get all the needed fields with the first query.
• mickmackusa: You're not saving unencrypted passwords are you? How can you be sure that you will never have an $idno collision?
• mickmackusa: Your question title seems to address your chief concern with your code instead of what your code does. You will need to edit your title.
• ArtisticPhoenix: $data["email"] = "' OR 1 --" There I just bypassed your query with SQLInjection, and passed your conditional check of if ($query->num_rows() > 0) For example select aid from tableA where email='' OR 1 --' and password='...' the -- is the start of a comment so the tail of the query is ignored. OR 1 is always true so this returns all the rows from that table.

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.