[SKRYPT] Ocena skryptu do dodawania/usuwania/edytowania artykułów na blogu Opcje

[lewsky]

Witam, dopiero co uczę się programować obiektowo, jest te mój pierwszy skrypt napisany tym sposobem. Będę wdzięczny za konstruktywną krytykę, będę wrzucał poprawione fragmenty kodu właśnie tutaj.

Obsługa artykułów articleClass.php

[PHP] pobierz, plaintext 
<?php
//THIS CLASS VALIDATES AND INSERTS
//ARTICLES INTO DATABASE
class AddArticle {
 
  public $title;
  public $article;
  public $img;
  public $flag;
  public $alert;
 
  function __construct(){
    $this->title = $_POST['title'];
    $this->article = $_POST['article'];
    $this->img = $_POST['imgurl'];
    $this->flag = false;
    $this->alert = '';
  } // ./__construct
 
  // IF SOMETHING GOES WRONG THIS METHOD IS STARTING
  public function echoAlert(){
    echo $this->alert;
  }
 
  // VALIDATES LENGTH OF TEXTS AND IMG SIZES
  public function checkInputs(){
  //TEXTS VALIDATION
    $explode = explode(" ", $this->article);
    if(strlen($this->title) < 10){
      $this->flag = false;
      $this->alert ="Długość tytułu musi wynościć przynajmniej 10 znaków";
 
    } elseif (count($explode)<20) {
      $this->flag = false;
      $this->alert = "Treśc artykułu musi wynosić przynajmniej 20 słów";
 
    } else {
      $this->flag = true;
    }
 
  //IMG VALIDATION
  // Remove all illegal characters from a url
  $url = filter_var($this->img, FILTER_SANITIZE_URL);
 
  // Validate url
  if (!filter_var($url, FILTER_VALIDATE_URL) === false) {
    list($width, $height) = getimagesize($url);
    if($width<1900 || $height<800){
      $this->flag=false;
      $this->alert = "Minimalne wymiary to 1900x800";
    } else{
      $this->flag=true;
    }
  } else {
      $this->flag=false;
      $this->alert = "Podaj poprawny adres url ";
  }
}// ./checkInputs()
 
 
// THIS METHOD INSERTS ARTICLE INTO DB
// USING OBJECT DbAction INSIDE db.php FILE
  public function insertArticle(){
 
    if($this->flag==true){
      require_once 'db.php';
      $db = new DbAction();
      $db->connection();
      $db->getResult("INSERT INTO `articles`(`title`,`imgurl`, `article`, date) VALUES ('$this->title','$this->img','$this->article', CURRENT_DATE)");
      $this->alert = "Pomyślnie dodano post";
    }
  }// ./insertArticle
} // ./AddArticle
 
 
//THIS CLASS GETS ARTICLES FROM DB
//AND DISPLAYS THEM IN INDEX.PHP FILE
class DisplayArticles{
 
    public $articleId;
    public $articleTitle;
    public $articleDate;
    public $articleText;
    public $noArticles;
 
  function __construct(){
 
      $this->articleId = '';
      $this->articleTitle = '';
      $this->articleDate='';
      $this->articleText ='';
      $this->noArticles = '';
  } // ./__construct
 
//THIS METHOD GETS ARTICLES FROM DB
//AND CLASS ANOTHER METHOD THAT DISPLAYS THEM
  public function getArticles(){
 
      require_once 'db.php';
      $db = new DbAction();
      $db->connection();
      $db->getResult("SELECT * FROM articles ORDER BY id DESC");
      $numRow = $db->rowNumber();
 
      if($numRow==0){$this->noArticles="Brak postów";}
 
 
      //call echoArticles as much as number of articles in DB
      while($row = $db->result->fetch_assoc()){
          $this->echoAll($row['id'], $row['title'], $row['date'], $row['article']);
      }
  }// ./getArticles
 
 
//THIS METHOD DISPLAYS ALL OF ARTICLES INSIDE INDEX.PHP
  public function echoAll($id, $title, $date, $article){
 
      $this->articleId = $id;
      $this->articleTitle = $title;
      $this->articleDate = $date;
      $this->articleText = $article;
      $articleShort="";
 
      echo '<div class="post-preview">';
      echo '<a href="post.php?id='.$this->articleId .'"> ';
      echo '<h2 class="post-title">'.$this->articleTitle.'</h2>';
      echo '</a><i>Posted on '.$this->articleDate.'</i>';
      echo '</a></div>';
      echo '<p>';
      $articleExplode = explode(" ",$this->articleText);
      for ($i=0; $i<=19; $i++){
        echo  $articleExplode[$i];
        if($i!=19){
          echo " ";
        }
      }
      echo "...";
      echo'</p>';
      echo '<hr>';
  }// ./echoAll
} // ./DisplayArticle
 
 
//THIS CLASS DISPLAY SINGLE FULL ARTICLE
//INSIDE post.php FILE
class PrintArticle {
 
    public $articleId;
    public $articleTitle;
    public $articleDate;
    public $articleText;
 
  function __construct() {
      $this->articleId='';
      $this->articleTitle='';
      $this->articleDate='';
      $this->articleImage='';
      $this->articleIText='';
  }
 
  //THIS METHOD DISPLAYS LONG VERSION OF ARTICLE IN FILE POST.PHP
    public function getArticle(){
 
        require_once 'db.php';
        $this->articleId = $_GET['id'];
        $db = new DbAction();
        $db->connection();
        $db->getResult("SELECT title, date, article, imgurl FROM articles where id='$this->articleId'");
        $row = $db->result->fetch_assoc();
 
        $this->articleTitle = $row['title'];
        $this->articleDate = $row['date'];
        $this->articleImage = $row['imgurl'];
        $this->articleText = $row['article'];
  }
}
 
 
//THIS CLASS DISPLAYS OVERWIEV OF ARTICLES IN ADMIN PANEL
class PanelEditing{
 
      public $articleId;
      public $articleTitle;
      public $articleDate;
      public $articleText;
      public $noArticles;
      public $alert;
    function __construct() {
        $this->articleId='';
        $this->articleTitle='';
        $this->articleDate='';
        $this->articleIText='';
        $this->noArticles='';
        $this->alert='';
    }
 
//THIS METHOD GET ARTICLES FROM DB
//AND CALLS ANOTHER METHOD THAT DISPLAYS THEm
    public function getArticles(){
 
        require_once '../db.php';
        $db = new DbAction();
        $db->connection();
        $db->getResult("SELECT * FROM articles ORDER BY id DESC");
        $numRow = $db->rowNumber();
 
        if($numRow==0){$this->noArticles="Brak postów";}
 
        //call echoArticles as much as number of articles in DB
        while($row = $db->result->fetch_assoc()){
            $this->printOverwiev($row['id'], $row['title'], $row['date'], $row['article']);
        }
    }// ./getAricles
 
    public function printOverwiev($id, $title, $date, $article){
 
        $this->articleId = $id;
        $this->articleTitle = $title;
        $this->articleDate = $date;
        $this->articleText = $article;
 
        echo'<form method="post" action="delete.php">';
        echo'<input type="hidden" name="id" value="'.$this->articleId.'">';
        echo'<div class="row">';
        echo'<div class="col-md-9">';
        echo'<div class="panel panel-default">';
        echo'<div class="panel-heading">';
        echo'<strong>'.$this->articleTitle.'</strong></br>';
        echo'<i>Posted on: '.$this->articleDate.'</i>';
        echo'</div>';
        echo'<div class="panel-body">'.$this->articleText.'</div>';
        echo'<div class="panel-footer">';
        echo'<a class="btn btn-default"; href="editArticle.php?id='.$this->articleId.'" style="margin-right: 4px;">Edytuj</a>';
        echo'<input type="submit" value="Usuń" onClick="return confirm(\'Na pewno chcesz usunąć ten post?\')"
        class="btn btn-danger" />';
 
        echo'</div>
            </div>
          </div>
          </form>
        </div>';
 
    }
 
    public function deleteArticle($postId){
        if(isset($postId)){
          require_once '../db.php';
          $db = new DbAction();
          $db->connection();
          $db->getResult("DELETE FROM `articles` WHERE id ='$postId' ");
 
        }
      }
} // ./PanelEditing
 
 
//THIS CLASS UPDATES ARTICLES
class UpdateArticles{
 
  public $articleId;
  public $articleTitle;
  public $articleText;
 
  function __construct(){
    $this->articleId = $_GET['id'];
    $this->articleTitle = $_POST['title'];
    $this->articleText = $_POST['article'];
  }
 
  public function updateArticle(){
    require_once 'db.php';
    $db = new DbAction();
    $db->connection();
    $db->getResult("UPDATE `articles` SET `title`='$this->articleTitle',`article`='$this->articleText' WHERE `id`=$this->articleId");
  }// ./updateArticle
 
}// ./UpdateArticle
[PHP] pobierz, plaintext 

Obsługa bazy danych db.php

[PHP] pobierz, plaintext 
<?php
class DbAction{
  public $query;
  public $result;
  public $connection;
  public $numRows;
  public $row;
  private $host;
  private $db_user;
  private $db_password;
  private $db_name;
 
 
  //CONSTRUCTOR
  public function __construct(){
      $this->connection;
      $this->query = '';
      $this->result = '';
      $this->numRows = '';
      $this->row = '';
      $this->host = "localhost";
      $this->db_user = "root";
      $this->db_password = "";
      $this->db_name = "cms";
  } // ./__construct
 
 
  //STARTS CONNECTION
 
    public function connection(){
        $this->connection = new mysqli($this->host, $this->db_user, $this->db_password, $this->db_name) ;
        if($this->connection == true){
          return true;
        } else {
          return false;
        }
  }// ./connection
 
 
  //RETURNS RESULT
 
  public function getResult($query){
      if ($this->result = $this->connection->query($query)) {
          return $this->result;
      } else{
          return false;
      }
  }// ./getResult
 
 
  //RETURNS NUMBER OF ROW
  public function rowNumber(){
      if($this->numRows =  $this->result->num_rows){
        return $this->numRows;
      } else{
        return false;
      }
  }
 
  //RETURNS NUMBER OF ROW
  public function row(){
 
      if($this->row =  $this->result->fetch_assoc()){
          return $this->row;
      } else {
        return false;
      }
  }
 
}// ./DbAction
 
 
?>
[PHP] pobierz, plaintext 

[markuz]

Niewiele ma to wspólnego z OOP, poza tym, że użyłeś klas Dużo nauki przed Tobą.
DbAction całkowicie bez sensu, już lepiej korzystać z czystego mysqli/PDO.
Wyobraź sobie teraz, że artykuł posiada dodatkowe pole tekstowe o nazwie "poleTekstowe" - ilość kodu do zmiany jest bardzo bardzo duża w twoim "skrypcie".

[viking]

Zobacz przykładowo https://github.com/RalfEggert/zend-expressi...part6/src/Album
Różnica taka że wszystko jest podzielone według zadań jakie ma pełnić. A zastosować można do dowolnych danych w tym takiego artykułu.

[mrc]

[PHP] pobierz, plaintext 
//THIS CLASS VALIDATES AND INSERTS
//ARTICLES INTO DATABASE
[PHP] pobierz, plaintext 

Po 1. Komentarz zbędny.
Po 2. Jeżeli jedna klasa ma dwie odpowiedzialności (robi dwie rzeczy), to rozbij ją na dwie klasy, które zajmują się jedną rzeczą.
Po 3. Nie uzupełniaj kodu komentarzami, które opisują coś, co już zostało opisane kodem.
Po 4.

[PHP] pobierz, plaintext 
        require_once 'db.php';
[PHP] pobierz, plaintext 

Nie lepiej użyć autoloadera?

Po 5. Brzydko formatujesz kod.

[SHiP]

1. Pomijając to, że kod jest bez sensu mnie najbardziej bolą konstruktory, które są kompletnie zbędne. Wartość domyślną możesz określić przy tworzeniu zmiennej

[PHP] pobierz, plaintext 
    public $articleTitle = '';
[PHP] pobierz, plaintext 

2. Nie mieszałbym typów. Skoro masz articleId, to domyślam się, że będzie tam liczba. Bez sensu, że przypisujesz do tej zmiennej string.
3. Staraj się używać === zamiast ==
4. Najlepiej wydrukuj ten kod i spal w kominku

[com]

SHiP A kto Ci powiedział, że jakiekolwiek id to musi być liczba. Tu zapewne jest wiec dobrze zwrócić na to uwagę, ale nie kategoryzujmy tak

Co do autora to niestety sporo pracy przed tobą, jeśli trzymamy się podawania zasięgu metod to wszędzie, dla konstruktora również, nie ważne że jest domyślnie public

Poczytaj o standardach w php czyli PSR

[PHP] pobierz, plaintext 
 $this->articleId = $_GET['id'];
    $this->articleTitle = $_POST['title'];
    $this->articleText = $_POST['article'];
[PHP] pobierz, plaintext 

Coś takiego będzie generować noticy, a poza tym jest bez sensu bo klasę uzależniłeś od jednej słusznej implementacji, a nie o to w obiektowości chodzi

[SHiP]

@com: masz rację . Złą zmienną wybrałem. Po prostu zmyliło mnie to, że wszystkie mają przypisywane puste stringi. Wymieniam na $numRows

Ten post edytował SHiP 15.06.2016, 20:10:21

[KsaR]

Mi się nie podoba:

* echo w metodach.
Według mnie lepiej jakbyś to zwracał (return) i dopiero tam gdzie wywolujesz metodę dodawał przed nią echo lub i nie.

* Poczytaj o SQL Injection oraz "Prepared statements".

[PHP] pobierz, plaintext 
    $db->getResult("UPDATE `articles` SET `title`='$this->articleTitle',`article`='$this->articleText' WHERE `id`=$this->articleId");
[PHP] pobierz, plaintext 

* sprawdzaj jakoś czy zmienne istnieją przed przypisaniem bo mogły nie być wysłane ( isset )

[PHP] pobierz, plaintext 
    $this->title = $_POST['title'];
    $this->article = $_POST['article'];
    $this->img = $_POST['imgurl'];
[PHP] pobierz, plaintext 

Ten post edytował KsaR 15.06.2016, 20:24:18

[com]

sprawdzaj jakoś czy zmienne istnieją przed przypisaniem bo mogły nie być wysłane

KsaR a właśnie że nie, tego wgl tam ma nie być

Ale generalnie to się zgadzam, ze trzeba sprawdzać

SHiP jasne rozumiem

[Lion]

1. Nie podobają mi się komentarze, ich umiejscowienie, zbędność. Przede wszystkim komentarze po bloku metody, są one zbędne, a jeśli musisz je mieć żeby wiedzieć czego dotyczy dana klamerka to oznacza to że metoda jest zbyt długa by ją ogarnąć na jednym ekranie, jednym rzutem oka.

2. Klasa DbAction mogłaby być jakimś wrapperem na PDO, w środku produkującym prepared statement, lub zostać zastąpiona po prostu samym PDO. Niezależnie od tego jaką drogę byś wybrał to dobrze by było ją w jakiś sposób wstrzyknąć, czy to w konstruktorze, czy w samej metodzie, tak by odseparować się od tej zależności.