Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

This is not a complete review, just a couple of points I noticed. I didn't see any bugs, but I didn't test the code in-depth.

Security

You should use prepared statements even if the data comes from the database (like with $row['mail_queue_id'] and $row['to']. Otherwise, you might be vulnerable to second order SQL injection. See also here.

if (cond) return true; else return false;

if ($mailer->send()) {
 return true;
} else {
 return false;
} 

can be rewritten as:

return $mailer->send();

Duplicate Code

 $sql = 'UPDATE mail_queue
 SET mail_status_id = [id]
 WHERE mail_queue_id = ' . $row['mail_queue_id'] . ';';
 $sth5 = $dbh->prepare($sql);
 $sth5->execute();

You have this code 3 times, I would either extract it to it's own function (updateMailQeue(errorCode)), or just save the error code in a local variable and execute this code at the end.

Confusing Check

You have this check:

if (count($queue) > 0) {
 // prepare smtp server
}
// do stuff with the smtp server

For a second, it confused be why only part of the code is inside this check (the answer is obviously that the foreach is an implicit check itself). I would rewrite it like this:

if (count($queue) > 0) {
 return; // nothing to send
}
// prepare smtp server
// do stuff with the smtp server

If you at some point extend the code, your code could easier lead to bugs.

Length of first Code Block

Your first try block is too long and too deeply nested. You could extract the code for each individual row in a new function (for example sendMail(mail_id)).

This is not a complete review, just a couple of points I noticed. I didn't see any bugs, but I didn't test the code in-depth.

Security

You should use prepared statements even if the data comes from the database (like with $row['mail_queue_id'] and $row['to']. Otherwise, you might be vulnerable to second order SQL injection. See also here.

if (cond) return true; else return false;

if ($mailer->send()) {
 return true;
} else {
 return false;
} 

can be rewritten as:

return $mailer->send();

Duplicate Code

 $sql = 'UPDATE mail_queue
 SET mail_status_id = [id]
 WHERE mail_queue_id = ' . $row['mail_queue_id'] . ';';
 $sth5 = $dbh->prepare($sql);
 $sth5->execute();

You have this code 3 times, I would either extract it to it's own function (updateMailQeue(errorCode)), or just save the error code in a local variable and execute this code at the end.

Confusing Check

You have this check:

if (count($queue) > 0) {
 // prepare smtp server
}
// do stuff with the smtp server

For a second, it confused be why only part of the code is inside this check (the answer is obviously that the foreach is an implicit check itself). I would rewrite it like this:

if (count($queue) > 0) {
 return; // nothing to send
}
// prepare smtp server
// do stuff with the smtp server

If you at some point extend the code, your code could easier lead to bugs.

Length of first Code Block

Your first try block is too long and too deeply nested. You could extract the code for each individual row in a new function (for example sendMail(mail_id)).

This is not a complete review, just a couple of points I noticed. I didn't see any bugs, but I didn't test the code in-depth.

Security

You should use prepared statements even if the data comes from the database (like with $row['mail_queue_id'] and $row['to']. Otherwise, you might be vulnerable to second order SQL injection. See also here.

if (cond) return true; else return false;

if ($mailer->send()) {
 return true;
} else {
 return false;
} 

can be rewritten as:

return $mailer->send();

Duplicate Code

 $sql = 'UPDATE mail_queue
 SET mail_status_id = [id]
 WHERE mail_queue_id = ' . $row['mail_queue_id'] . ';';
 $sth5 = $dbh->prepare($sql);
 $sth5->execute();

You have this code 3 times, I would either extract it to it's own function (updateMailQeue(errorCode)), or just save the error code in a local variable and execute this code at the end.

Confusing Check

You have this check:

if (count($queue) > 0) {
 // prepare smtp server
}
// do stuff with the smtp server

For a second, it confused be why only part of the code is inside this check (the answer is obviously that the foreach is an implicit check itself). I would rewrite it like this:

if (count($queue) > 0) {
 return; // nothing to send
}
// prepare smtp server
// do stuff with the smtp server

If you at some point extend the code, your code could easier lead to bugs.

Length of first Code Block

Your first try block is too long and too deeply nested. You could extract the code for each individual row in a new function (for example sendMail(mail_id)).

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

This is not a complete review, just a couple of points I noticed. I didn't see any bugs, but I didn't test the code in-depth.

Security

You should use prepared statements even if the data comes from the database (like with $row['mail_queue_id'] and $row['to']. Otherwise, you might be vulnerable to second order SQL injection. See also here here.

if (cond) return true; else return false;

if ($mailer->send()) {
 return true;
} else {
 return false;
} 

can be rewritten as:

return $mailer->send();

Duplicate Code

 $sql = 'UPDATE mail_queue
 SET mail_status_id = [id]
 WHERE mail_queue_id = ' . $row['mail_queue_id'] . ';';
 $sth5 = $dbh->prepare($sql);
 $sth5->execute();

You have this code 3 times, I would either extract it to it's own function (updateMailQeue(errorCode)), or just save the error code in a local variable and execute this code at the end.

Confusing Check

You have this check:

if (count($queue) > 0) {
 // prepare smtp server
}
// do stuff with the smtp server

For a second, it confused be why only part of the code is inside this check (the answer is obviously that the foreach is an implicit check itself). I would rewrite it like this:

if (count($queue) > 0) {
 return; // nothing to send
}
// prepare smtp server
// do stuff with the smtp server

If you at some point extend the code, your code could easier lead to bugs.

Length of first Code Block

Your first try block is too long and too deeply nested. You could extract the code for each individual row in a new function (for example sendMail(mail_id)).

This is not a complete review, just a couple of points I noticed. I didn't see any bugs, but I didn't test the code in-depth.

Security

You should use prepared statements even if the data comes from the database (like with $row['mail_queue_id'] and $row['to']. Otherwise, you might be vulnerable to second order SQL injection. See also here.

if (cond) return true; else return false;

if ($mailer->send()) {
 return true;
} else {
 return false;
} 

can be rewritten as:

return $mailer->send();

Duplicate Code

 $sql = 'UPDATE mail_queue
 SET mail_status_id = [id]
 WHERE mail_queue_id = ' . $row['mail_queue_id'] . ';';
 $sth5 = $dbh->prepare($sql);
 $sth5->execute();

You have this code 3 times, I would either extract it to it's own function (updateMailQeue(errorCode)), or just save the error code in a local variable and execute this code at the end.

Confusing Check

You have this check:

if (count($queue) > 0) {
 // prepare smtp server
}
// do stuff with the smtp server

For a second, it confused be why only part of the code is inside this check (the answer is obviously that the foreach is an implicit check itself). I would rewrite it like this:

if (count($queue) > 0) {
 return; // nothing to send
}
// prepare smtp server
// do stuff with the smtp server

If you at some point extend the code, your code could easier lead to bugs.

Length of first Code Block

Your first try block is too long and too deeply nested. You could extract the code for each individual row in a new function (for example sendMail(mail_id)).

This is not a complete review, just a couple of points I noticed. I didn't see any bugs, but I didn't test the code in-depth.

Security

You should use prepared statements even if the data comes from the database (like with $row['mail_queue_id'] and $row['to']. Otherwise, you might be vulnerable to second order SQL injection. See also here.

if (cond) return true; else return false;

if ($mailer->send()) {
 return true;
} else {
 return false;
} 

can be rewritten as:

return $mailer->send();

Duplicate Code

 $sql = 'UPDATE mail_queue
 SET mail_status_id = [id]
 WHERE mail_queue_id = ' . $row['mail_queue_id'] . ';';
 $sth5 = $dbh->prepare($sql);
 $sth5->execute();

You have this code 3 times, I would either extract it to it's own function (updateMailQeue(errorCode)), or just save the error code in a local variable and execute this code at the end.

Confusing Check

You have this check:

if (count($queue) > 0) {
 // prepare smtp server
}
// do stuff with the smtp server

For a second, it confused be why only part of the code is inside this check (the answer is obviously that the foreach is an implicit check itself). I would rewrite it like this:

if (count($queue) > 0) {
 return; // nothing to send
}
// prepare smtp server
// do stuff with the smtp server

If you at some point extend the code, your code could easier lead to bugs.

Length of first Code Block

Your first try block is too long and too deeply nested. You could extract the code for each individual row in a new function (for example sendMail(mail_id)).

Source Link
tim
  • 25.3k
  • 3
  • 31
  • 76

This is not a complete review, just a couple of points I noticed. I didn't see any bugs, but I didn't test the code in-depth.

Security

You should use prepared statements even if the data comes from the database (like with $row['mail_queue_id'] and $row['to']. Otherwise, you might be vulnerable to second order SQL injection. See also here.

if (cond) return true; else return false;

if ($mailer->send()) {
 return true;
} else {
 return false;
} 

can be rewritten as:

return $mailer->send();

Duplicate Code

 $sql = 'UPDATE mail_queue
 SET mail_status_id = [id]
 WHERE mail_queue_id = ' . $row['mail_queue_id'] . ';';
 $sth5 = $dbh->prepare($sql);
 $sth5->execute();

You have this code 3 times, I would either extract it to it's own function (updateMailQeue(errorCode)), or just save the error code in a local variable and execute this code at the end.

Confusing Check

You have this check:

if (count($queue) > 0) {
 // prepare smtp server
}
// do stuff with the smtp server

For a second, it confused be why only part of the code is inside this check (the answer is obviously that the foreach is an implicit check itself). I would rewrite it like this:

if (count($queue) > 0) {
 return; // nothing to send
}
// prepare smtp server
// do stuff with the smtp server

If you at some point extend the code, your code could easier lead to bugs.

Length of first Code Block

Your first try block is too long and too deeply nested. You could extract the code for each individual row in a new function (for example sendMail(mail_id)).

lang-php

AltStyle によって変換されたページ (->オリジナル) /