Avaliação de código

Dei uma pesquizada sobre singleton e parece que o mesmo não é aconselhável para conexão com base de dados, então retirei a implementação com singleton.

<?php
require_once(dirname(dirname(__FILE__)).'/funcoes.php');
protegeArquivo(basename(__FILE__));

class Database{

/**
* $mysqli
* propriedade privada para armazenar uma instancia da classe mysqli
*
* @var private
*/

	private $mysqli;

	/**
 * $database_config
 * propriedade privada para armazenar as configurações do Banco de Dados
 *
 * @var array
 */
	private $database_config = array();

	/**
 * Construtor da classe
 * Conecta com o banco de dados, armazenando a instancia deste em $mysqli
 *
 * @param void
 * @return void
 */
	public function __construct(){

 $this->database_config[0] = defined("DBHOST") ? DBHOST : "";
 $this->database_config[1] = defined("DBUSER") ? DBUSER : "";
 $this->database_config[2] = defined("DBPASS")	? DBPASS	: "";
 $this->database_config[3] = defined("DBNAME")	? DBNAME	: "";
 if(is_array($this->database_config) or !in_array("", $this->database_config))
 {
 $this->mysqli = new MySQLi($this->database_config[0], $this->database_config[1], $this->database_config[2], $this->database_config[3]);
 if($this->mysqli->connect_error)
 {
 throw new Exception('Falha na conexão com banco de dados: ' . $this->mysqli->connect_error);
 }
 }else{
 throw new Exception('Arquivo de configuração inexistente: ' . $this->mysqli->connect_error);
 }
	}

	/**
 * Consulta
 *
 * @param string $sql
 * @return mysqli_result Object
 * @return mysqli_result affected_rows
 */
	public function query($sql)
	{
 try{
 $result = $this->mysqli->query($sql);
 } catch (Exception $e) {
 throw new Exception($e);
 }
 if(substr(trim(strtolower($sql)), 0, 6) == 'select'){
 return $result;
 }else{
 return $this->mysqli->affected_rows;
 }
	}

	public function selectALL($tabela){
 $sql = "SELECT * FROM $tabela";
 try{
 return $this->query($sql);
 }catch (Exception $e) {
 throw new Exception($e);
 }
	}

	public function inserir($tabela, $campos_valores){
 $sql = "INSERT INTO ".$tabela." (";
 for($i = 0; $i < count($campos_valores); $i++){
 $sql .= key($campos_valores);
 if($i < (count($campos_valores)-1)){
 $sql .= ", ";
 }else{
 $sql .= ") ";
 }
 next($campos_valores);
 }
 reset($campos_valores);
 $sql .= "VALUES (";
 for($i = 0; $i < count($campos_valores); $i++){
 $sql .= is_numeric($campos_valores[key($campos_valores)]) ?
 $campos_valores[key($campos_valores)] :
 "'". $campos_valores[key($campos_valores)] ."'";
 if($i < (count($campos_valores)-1)){
 $sql .= ", ";
 }else{
 $sql .= ") ";
 }
 next($campos_valores);
 }
 try{
 return $this->query($sql);
 }catch (Exception $e) {
 throw new Exception($e);
 }
	}

	public function deletar($tabela, $campoPk, $valorPk){
 $sql = "DELETE FROM ".$tabela;
 $sql .= " WHERE ".$campoPk."=";
 $sql .= is_numeric($valorPk) ? $valorPk : "'".$valorPk."'";
 try{
 return $this->query($sql);
 }catch (Exception $e) {
 throw new Exception($e);
 }
	}

	public function atualizar($tabela, $campos_valores, $campoPk, $valorPk, $extra = NULL){
 $sql = "UPDATE ".$tabela." SET ";
 for($i = 0; $i < count($campos_valores); $i++){
 $sql .= key($campos_valores) . "=";
 $sql .= is_numeric($campos_valores[key($campos_valores)]) ?
 $campos_valores[key($campos_valores)] :
 "'". $campos_valores[key($campos_valores)] ."'";
 if($i < (count($campos_valores)-1)){
 $sql .= ", ";
 }else{
 $sql .= " ";
 }
 next($campos_valores);
 }
 $sql .= "WHERE ".$campoPk."=";
 $sql .= is_numeric($valorPk) ? $valorPk : "'".$valorPk."'";
 if($extra != NULL){
 $sql .= $extra;
 }
 try{
 return $this->query($sql);
 }catch (Exception $e) {
 throw new Exception($e);
 }
 }

	public function last_insert_id(){
 return $this->mysqli->insert_id;
	}

	public function retornaDados($result, $tipo = NULL){
 switch(strtolower($tipo)){
 case "array" :
 return $result->fetch_array();
 break;
 case "assoc" :
 return $result->fetch_assoc();
 break;
 case "object" :
 return $result->fetch_object();
 break;
 default:
 return $result->fetch_object();
 break;
 }
	}
}

Primeiro uma classe de conexão com o database, utilizado Singleton

Eu não vou entrar no mérito de singletons, mas com database eles não devem ser usados.

Se você precisa de um banco de dados apenas, instancie apenas uma vez, não crie um singleton para isso. Se você precisa de um ponto único de acesso, repense sua arquitetura.

>

<?php
require_once(dirname(dirname(__FILE__)).'/funcoes.php');

Existe a constante __DIR__ desde o PHP 5.3. Use dirname(__DIR__) ao invés de dirname(dirname(__FILE__)).

Além do mais, o PHP possui autoloading: http://php.net/manual/en/language.oop5.autoload.php

>

protegeArquivo(basename(__FILE__));

Você não precisa e não deveria estar fazendo isso.

Para proteger seus arquivos de acesso direto, coloque-os um diretório acima do diretório público (acessível pelo usuário).

>

 $database_config[0] = defined("DBHOST") ? DBHOST : "";
 $database_config[1] = defined("DBUSER") ? DBUSER : "";
 $database_config[2] = defined("DBPASS")	? DBPASS	: "";
 $database_config[3] = defined("DBNAME")	? DBNAME	: "";

Constantes para configuração? A configuração deveria ser injetada através de parâmetros no construtor.

Com constantes globais você está acabando com a flexibilidade (apenas um banco é permitido) e criando um "foco" para comportamentos inesperados (se as constantes não estiverem definidas, haverá um erro de conexão).

>

	private function __clone(){
 trigger_error("Operação não permitida!");
	}

Redundância. O método já é privado.. não precisa de lançar um erro.

>

	public function __construct(){
 $this->mysqli = Connect::conn();
	}

Acoplamento. Você deveria injetar a instância e não construí-la diretamente.

Dependency Injection

>

try{

 $result = $this->mysqli->query($sql);
 } catch (Exception $e) {
 throw new Exception($e);
 }

Você captura uma exceção para então lançar uma outra do mesmo tipo? redundância.. o try/catch não é necessário aí nem em nenhum outro lugar onde você está fazendo este tipo de coisa.

>

 if(substr(trim(strtolower($sql)), 0, 6) == 'select'){
 return $result;
 }else{
 return $this->mysqli->affected_rows;
 }

 

Como o return para com o resto da função, o else é desnecessário, basta isso:

 

 if(substr(trim(strtolower($sql)), 0, 6) == 'select'){
 return $result;
 }

 return $this->mysqli->affected_rows;

>

for($i = 0; $i < count($campos_valores); $i++){

 $sql .= key($campos_valores);
 if($i < (count($campos_valores)-1)){
 $sql .= ", ";
 }else{
 $sql .= ") ";
 }
 next($campos_valores);
 }

Você não precisa de for para isso. Basta usar um simples foreach.

>

	public function retornaDados($result, $tipo = NULL){
 switch(strtolower($tipo)){
 case "array" :
 return $result->fetch_array();
 break;
 case "assoc" :
 return $result->fetch_assoc();
 break;
 case "object" :
 return $result->fetch_object();
 break;
 default:
 return $result->fetch_object();
 break;
 }
	}

Um bad smell popular conhecido como Switch Statements Smell. Como você está usando OOP, Replace Conditional with Polymorphism resolve isso.

--------

Ademais:

• use PDO. Mysqli é limitado ao MySQL.
• este código é desnecessário. Você pode pegar o PDO e usar, simples assim. Não é necessário criar uma classe de database, a não ser que seja um ORM ou algo que realmente acrescente alguma coisa.
• siga um estilo de código. Veja: PSR-2.
• não crie comentários estúpidos do tipo "Cria uma instancia desta classe se ela não existir" e "propriedade privada para armazenar uma instancia da classe mysqli", por exemplo. Esse tipo de comentários só atrapalham e poluem o código, deixe o código falar por si.
• siga a especificação do phpDocumentor para os docblocks. Eu vi, por exemplo, "@var private", que é obsoleta e "@Exception se der erro", que é inválido, por exemplo.
• programe em inglês já que a linguagem de programação é em inglês. Programar em português ou em qualquer outro idioma que não seja inglês deixa o código sem sentido. É como se você lesse um texto que estivesse escrito metade em uma línguua e outra metade em outra.
• leia o Clean Code. Este livro é um livro técnico sobre como fazer, tornar e manter um código limpo.

• Enrico Pereira, obrigado por responder, fiz algumas das correções, fiquei um pouco perdido onde vc fala pra usar o foreach, poderia me auxiliar pra ver se melhoro ali, obirgado.

Na verdade, nem de foreach você precisa. Um simples implode resolve tudo isso aí.

$keys = array_keys($campos_valores);
$values = array_values($campos_valores);

$sql = sprintf('INSERT INTO %s (%s) VALUES (%s)', $tabela, implode(', ', $keys), implode(', ', $values));

Ficou show de bola, só que se for número nos valores, não pode-se te as aspas simples, e se for texto, tem que ter as aspas, o que sugere?

Isso:

$values = array_map(function ($value) {
 return is_numeric($value) ? $value : "'{$value}'";
}, $campos_valores);

$keys = array_keys($values);
$values = array_values($values);

$sql = sprintf('INSERT INTO %s (%s) VALUES (%s)', $tabela, implode(', ', $keys), implode(', ', $values));

Na verdade, se ele continuar trabalhando só com MySQL, é tranquilo passar não-strings com aspas. O DB faz a conversão.

Se for usar PDO, dá pra trocar essa zona por bound parameters.

Enrico Pereira, desculpe incomodar novamente, mas tentei entender sobre o que vc falou sobre bad smell, mas estou meio perdido na implementação, poderia me indicar mais informações para que eu entenda, obrigado.

O problema dos switch cases nesse tipo de caso é a diminuição da flexibilidade e a violação do Open-Closed Principle (OCP).

Se você tiver que adicionar uma outra forma de dar o fetch, precisará editar a classe. Este é o problema.

Na verdade, o problema é que você não deveria estar fazendo este tipo de coisa. É desnecessário. Tudo que você deseja é um Query Builder, não uma classe que manipule db, pois o PHP já a possui.

Você poderia usar um ORM, como o Doctrine.