4
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ Where are your prepared statements or Codeigniter query building methods? \$\endgroup\$ – mickmackusa Jun 14 at 5:51
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – KIKO Software Jun 14 at 6:41
  • \$\begingroup\$ You're not saving unencrypted passwords are you? How can you be sure that you will never have an $idno collision? \$\endgroup\$ – mickmackusa Jun 14 at 7:25
  • 3
    \$\begingroup\$ 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. \$\endgroup\$ – mickmackusa Jun 14 at 7:30
  • \$\begingroup\$ @mickmackusa Thanks for the reply, and actually I think the so many nestings will result in lower execution efficiency. \$\endgroup\$ – Mark Jun 15 at 13:21
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.