1. PHP / Говнокод #3823

    +147

    1. 01
    2. 02
    3. 03
    4. 04
    5. 05
    6. 06
    7. 07
    8. 08
    9. 09
    10. 10
    11. 11
    12. 12
    13. 13
    14. 14
    15. 15
    16. 16
    17. 17
    18. 18
    19. 19
    20. 20
    21. 21
    22. 22
    23. 23
    24. 24
    25. 25
    26. 26
    27. 27
    28. 28
    29. 29
    30. 30
    31. 31
    32. 32
    33. 33
    34. 34
    35. 35
    36. 36
    37. 37
    38. 38
    39. 39
    40. 40
    41. 41
    42. 42
    43. 43
    44. 44
    45. 45
    46. 46
    47. 47
    48. 48
    49. 49
    50. 50
    51. 51
    52. 52
    53. 53
    54. 54
    55. 55
    56. 56
    57. 57
    58. 58
    59. 59
    60. 60
    61. 61
    62. 62
    63. 63
    64. 64
    65. 65
    66. 66
    67. 67
    68. 68
    69. 69
    70. 70
    71. 71
    72. 72
    73. 73
    74. 74
    75. 75
    <?php

require_once 'MDB2.php';

$dsn = "mysql://user:pass@localhost/db";

$mdb2 = & MDB2::singleton($dsn);
if (PEAR::isError($mdb2)) {
    die($mdb2->getMessage());
}

class DB {

    static private $instance = NULL;
    static private $mdb2 = NULL;

    private function  __construct() {
        self::$mdb2 = & MDB2::singleton();
        self::$mdb2->exec("SET NAMES utf8");
        self::$mdb2->setFetchMode(MDB2_FETCHMODE_ASSOC);
    }

    static function getInstance() {
        if(self::$instance == NULL) {
            self::$instance =  & new DB();
        }
        return self::$instance;
    }

    public function query($sql = false) {
        $res = self::$mdb2->query($sql);
        if (PEAR::isError($res)) {
            die($res->getMessage());
        }
        if(!$res->numRows()) {
            return FALSE;
        }
        return $res;
    }

    private function __clone() {

    }
}


class Page{

    public   $limit = 10;
    private $conn = FALSE;  

    function  __construct() {
        $this->conn = & DB::getInstance();
    }

    public function getPageList() {

        $result = FALSE;

        $sql = "SELECT * FROM table LIMIT ".$this->limit;
        $res = $this->conn->query($sql);
        if($res) {
            $result = $res->fetchAll();
        }
        
        return $result;
    }

}

$p = & new Page();
$nodes = $p->getPageList(25);
print '<pre>'.print_r($nodes, 1).'</pre>';

?>

    Дайте, пожалуйста, оценку такой конструкции. Не говнокод ли?

    Запостил: cartman, 29 Июля 2010

    Комментарии (6) RSS

    • ava Kornew 30.07.2010 00:21 # −2

      $sql = "SELECT * FROM table LIMIT "
      - не есть хорошо....
      лучше бы было, если имя таблицы передавалось бы в параметрах метода:
      $sql = "SELECT * FROM ' " . $table . " ' LIMIT "
      $sql = "SELECT * FROM [ " . $table . " ] LIMIT "
      -- не помню как будет точнее, если вдруг имя таблицы будет кириллицей;

      function getPageList();
      $p->getPageList(25);
      -- функция ведь объявлена без параметров, зачем в неё передавать параметр?


      а так вроде норм (мб чот упустил)
      Ответить

    • ava Анонимус 30.07.2010 01:28 # −3 

      $p = & new Page();
$nodes = $p->getPageList("1;; DELETE FROM table"); //вот и все
      Ответить

      • ava cartman 30.07.2010 01:54 # −2

        //$nodes = $p->getPageList("1;; DELETE FROM table"); //вот и все

        Вы хуйню написали...

        Дело не в возможности инъекции или нет, это не продакшн код. Собственно говоря, суть вопроса. Класс DB враппер MDB2, что позволяет не таскать по всему коду проверку на PEAR::isError() а делать его в одном месте, (класс легко расширить любыми другими методами). Конкретно смущает реализация синглтона DB::getInstance(). Чем смущает сказать не могу, может кто-то знает, как это по другому делается?

        P.S.
        function getPageList();
        $p->getPageList(25);
        -- функция ведь объявлена без параметров, зачем в неё передавать параметр?

        да... проебал должно быть примерно так:

        function getPageList($limit = 0) {
        if(!empty($limit) && is_numeric($limit)) { $this->limit = $limit; }
        .....
        Ответить

        • ava Анонимус 30.07.2010 02:24 # −1

          давайте по пунктам тогда

          >>не таскать по всему коду проверку на PEAR::isError() а делать его в одном мест
          и делать die, что бы клиентский код не смог ни залогировать ошибку ни передать управление дальше.
          Особенно это клево, если Вы будете писать phpmyadmin например. Надо кидать exception или делать логирование прямо там.

          >>Конкретно смущает реализация синглтона DB::getInstance().
          Одиночек так и делают.

          на худой конец так: 
          public final static INSTANCE = new Db(); //но есть ли в php final -- не помню. Если нет -- то так не надо делать.



          Итого: единственный смысл Вашей обертки это "SET NAMES utf8". Имеет право на существование впринципе, особенно если заменить die на логирование и exception.

          Класс Page мы не обсуждаем, верно?
          Ответить

          • ava telnet 30.07.2010 09:00 # +1

            > но есть ли в php final -- не помню
            Есть с пятой версии.
            Ответить

        • ava telnet 30.07.2010 09:00 # 0

          > Конкретно смущает реализация синглтона DB::getInstance()
          Меня смущает тем, что в метод MDB2::singleton() нужно передавать параметр инициализации. Реализуя "одиночку", я предпочитаю писать конструктор так, чтобы он сам доставал откуда ему надо конфигурационные данные. Это делает возможным ленивую инициализацию, когда класс инициализируется не раньше, чем он потребуется другим частям программы.
          Ну ещё я всё-таки предпочитаю называть метод singleton(), а не getInstance(), следуя совету coding standard'а ZF (чтобы из названия метода следовало его назначение). Но это уже личное предпочтение.
          Ответить

    Добавить комментарий