M
M
Max Ba2018-03-02 14:00:02
PHP
Max Ba, 2018-03-02 14:00:02

Am I using the class correctly?

Guys, I created a report class. Did I correctly create the logs.php file itself , where my class is used, and are there any gross errors in the class itself?

class Logs
{
  protected $db;
  public $id;
  public $text;

  public function __construct($db){
    $this->db = $db;
  }
  
  public function create($text){
    $query = "INSERT INTO `logs` (`date`, `text`) VALUE (NOW(), ?)";
    $stmt = $db->prepare($query);
    if($stmt->execute(array($text))){
      return true;
    }else{
      return false;
    }
  }
  
  public function delete($id){
    $stmt = $db->prepare("DELETE FROM `logs` WHERE `id` = ?");
    if($stmt->execute(array($id))){
      return true;
    }else{
      return false;
    }
  }
  
  public function getAll($id = 0){
    if(!empty($id)){
      $query ="SELECT * FROM `logs` WHERE `id` = '".intval($id)."'";
    }else{
      $query ="SELECT * FROM `logs`";
    }
    $stmt = $db->query($query);
    return $stmt->fetchAll(PDO::FETCH_ASSOC);
  }
}

//logs.php ВОПРОС БОЛЬШЕ К ЭТОЙ ЧАСТИ КОДА

$logs = new Logs(DB:instance());

if(isset($_POST['add'], $_POST['text'])){
  if($logs->create($_POST['text'])){
    echo 'Запись добавлена';
  }else{
    echo 'Ошибка добавления';
  }
}

if(isset($_GET['del_id'])){
  if($logs->delete($_GET['del_id'])){
    echo 'Запись удалена';
  }
}

echo '<h2>Все отчеты</h2>';
echo '<ul>';
foreach($logs->getAll as $v){
  echo '<li>'.$v['date'].': '.$v['text'].'</li>';
}
echo '</ul>';

Answer the question

In order to leave comments, you need to log in

2 answer(s)
M
Maxim Fedorov, 2018-03-02
@qonand

A quick look:
1. Why are the public attributes id and text declared in the class if they are not used anywhere?
2. You use $db->prepare everywhere, but the local variable $db is not initialized. In this code, you must refer to a class attribute, i.e. $this->db->prepare
3. Why check this check in each method

if($stmt->execute(array($text))){
   return true;
}else{
   return false;
}

but what's the point of it if $stmt->execute already returns boolean, it's easier to just write
4. And in general, in case of an error, it's better that the method does not return false but throws an exception, and class clients already somehow react to it
5 In the getAll method, it is better to set the $id parameter to null by default

A
Anton Mashletov, 2018-03-02
@mashletov

1) the name of the class in the plural no one does. just Log.
2) use the PSR-3 Logger Interface and don't go big.
3) design type

if($stmt->execute(array($text))){
      return true;
    }else{
      return false;
    }
can always be shortened to
return (bool)$stmt->execute(array($text));

Didn't find what you were looking for?

Ask your question

Ask a Question

731 491 924 answers to any question