0
\$\begingroup\$

Here I have a class where I'm checking if model_id exists in the table models_web_tbl.

Everything works like a charm but I just want to know if I'm on the right path.

class test extends dbh{

public function modelExist($model_id) {

$sql = "SELECT * FROM models_web_tbl WHERE model_id = ?";

    $stmt = $this->connect()->prepare($sql);
    $stmt->execute([$model_id]);
    $match = $stmt->fetch(PDO::FETCH_OBJ);

        if ($match > 0){

            echo "found";

        }else {

            $model_web_twitter = "twitter";

            $sql = "INSERT INTO models_web_tbl (model_web_twitter, model_id) VALUES (?, ?)";
            $stmt = $this->connect()->prepare($sql);
            $stmt->execute([$model_web_twitter, $model_id]);

            }

}
}
$model_id = 66;
$exist = new test ();
$exist = $exist->modelExist($model_id);
\$\endgroup\$
2
\$\begingroup\$

Function modelExist should check if model exists and return boolean. Your function does something completely different. It only needs that check for its job.

public function modelExists(int $modelId): bool;

To check existence of a row, you don't need all its columns. Select just 1 (literally) and check if you have received a row from the query or not.

SELECT 1 FROM models_web_tbl WHERE model_id = ?

Create another function for the insert

public function insertModel(int $modelId, string $modelWeb): void

Maybe you wanted to combine it into another function.

public function tryInsert(int $modelId, string $modelWeb): bool
{
  if ($this->modelExists($modelId)) {
    return false;
  }
  $this->insert($modelId, $modelWeb);
  return true;
}

Don't connect to database again for every query

class test
{
  private PDO $connection;
  function __construct(PDO $connection) {
    $this->connection = $connection;
  }
  function modelExists(int $modelId): bool
  {
    // ...
    $this->connection->prepare($sql);
    // ...
  }

  function insert(int $modelId, $modelWeb): void
  {    
    // ...
    $this->connection->prepare($sql);
    // ...
  }
}
$connection = connect();
$test = new test($connection);
  // I don't know why it was important to assign 'twitter' to a variable first :)
if (!$test->tryInsert($model_id, 'twitter')) {
  // echo definitely dont belong into exists-check function
  echo "found";
}
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Is there a criterion to choose between SELECT 1 or SELECT COUNT(*) for the query? (Obviously makes no difference if model_id is a key, but we can't know that here) \$\endgroup\$ – Toby Speight Feb 14 at 11:14
  • \$\begingroup\$ @TobySpeight well if model_id is unique then you at least save the dummy aggregation (it really is not needed so why adding, its like adding extra noops for no reason), but the difference will be miniscule. So it is just a convention or preference I guess. However if it was not unique, it would even make sense to add LIMIT 1 and definitely avoid COUNT, unless you really need to know the exact count. But in this case only "none vs. any" is requested. \$\endgroup\$ – slepic Feb 14 at 11:27
  • \$\begingroup\$ Thanks; I see that LIMIT 1 is a better choice for ensuring we're not sent a huge result. \$\endgroup\$ – Toby Speight Feb 14 at 12:24
1
\$\begingroup\$

The code you posted doesn't seem to be doing what it says. From the way it is called and from the method's name one could tell it is intended to check if the id exists or not. But it doesn't return any answer to this question. instead it inserts some info into a database, which is extremely confusing.

Also this code does some strange things like echoing the word "found" or overwriting the $exist variable with null value.

Therefore, the first and foremost thing you need to do is to make this code consistent. After that it will be possible to review it.

|improve this answer|||||
\$\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.