Ves algun error en esta funcion?

YeltsinReyes · 17 Sep 2017

Hola programadores, eh creado esta funcion para actualizar o insertar un registro si no existe, la funcion me funciona bien
pero me gustaria su opinion por si puedo mejorarla o ven algun error!

PHP:

function getConexion(){
    $con = @new mysqli(DBHOST, DBUSER, DBPASS, DBNAME);
    if($con->connect_error){ 
        die('<h1>Error en la conexion: '.$con->connect_error.'</h1>'); 
    }
    if(!$con->set_charset('utf8')){ 
        die('<h1>Error al agregar utf8: '.$con->error.'</h1>'); 
    }
    return $con;
}

PHP:

function setResultado($datos = ''){
	if(!empty($datos)){
		$con = getConexion();
		$res = $con->query('SELECT * FROM resultados WHERE id = "'.$datos['id'].'" AND hash = "'.$datos['hash'].'" LIMIT 1');
		if($res->num_rows > 0){
			
			foreach($datos as $key => $val){
				$set[] = $key.'="'.$con->real_escape_string($val).'"';
			}
			
			$set = implode(',', $set);

			$sql = 'UPDATE resultados SET '.$set.' WHERE id = "'.$datos['id'].'" AND hash = "'.$datos['hash'].'" LIMIT 1';
			if($con->query($sql) && $con->affected_rows > 0){
				$return = true;
			} else {
				$return = NULL;
			}
			
		} else {
			
			foreach($datos as $key => $val){
				$keys[] = $key;
				$vals[] = '"'.$con->real_escape_string($val).'"';
			}
			
			$keys = implode(',', $keys);
			$vals = implode(',', $vals);
			
			$sql = 'INSERT INTO resultados ('.$keys.') VALUES ('.$vals.')';
			if($con->query($sql)){
				$return = true;
			} else {
				$return = NULL;
			}
			
		}
		
		$res->close();
		$con->close();
	} else {
		$return = NULL;
	}
	
	return $return;
}

PHP:

$datos['id'] = '1234567890';
$datos['hash'] = 'abcdef';
// mas datos...
if(setResultado($datos)){

}

Saludos!

lobogris · 17 Sep 2017

La función `getConexion` crea una nueva conexión a la base de datos cada vez que la usas, lo que no tiene mucho sentido, podrías solucionarlo cacheando la conexión en una variable estática, ej:

PHP:

function getConexion(){
    static $con;

    if (! $con) {
		$con = @new mysqli(DBHOST, DBUSER, DBPASS, DBNAME);
	  
	    if($con->connect_error){ 
	        die('<h1>Error en la conexion: '.$con->connect_error.'</h1>'); 
	    }
	    if(!$con->set_charset('utf8')){ 
	        die('<h1>Error al agregar utf8: '.$con->error.'</h1>'); 
	    }
	}
    
    return $con;
}

Luego en la función `setDatos` hay varias cosas sin sentido...
- El parámetro $datos que recibe la función, no tiene sentido que sea una cadena y vacía, lo propio si estás esperando un array sería `function setResultado(array $datos) {...}`
- Hay valores que no estás escapando, aunque tal vez no sea necesario
- En el update, estás actualizando valores que no son necesarios, id y hash
- Luego no sé si que tipo de valor es el id, pero si es un número único en MySQL es posible que no necesites utilizar el hash en las consultas...
- No es necesario que hagas un `return NULL;` cuando una función no devuelve un valor, el valor es NULL

Y por supuesto, te recomendaría olvidar Mysqli y pasarte a PDO 🙂

Y bueno respecto al código, yo o lo escribiría en Inglés o en Español, pero mezclarlo queda bastante feo jeje

YeltsinReyes · 17 Sep 2017

[MENTION=36167]lobogris[/MENTION] saludos man, muchas gracias por responder!! :encouragement:

La función `getConexion` crea una nueva conexión a la base de datos cada vez que la usas!!
No entiendo esta parte, solo la eh llamado 1 sola ves!!

- En el update, estás actualizando valores que no son necesarios, id y hash
aqui eh echo esto...

PHP:

foreach($datos as $key => $val){
	if($key != 'hash'){
		$set[] = $key.'="'.$con->real_escape_string($val).'"';
	}
}

- Luego no sé si que tipo de valor es el id, pero si es un número único en MySQL es posible que no necesites utilizar el hash en las consultas...
aqui necesito los dos id y hash ya que el id es el del usuario y el hash del resgistro que creo el usuario y el usuario puede tener varios registros!!

- No es necesario que hagas un `return NULL;` cuando una función no devuelve un valor, el valor es NULL

aqui guarde el valor a devolver en una variable $return; para poder usar $res->close(); $con->close(); antes de los return true; pero seguramente
si retorno return $return; y $return; no esta definidad seria null ???

saludos man!!

lobogris · 17 Sep 2017

De nada 🙂

Respecto a la función `getConexion` en lo que has mostrado de tu código solo la utilizas una vez, pero no sé como es el resto, por eso te lo digo ya que normalmente el código se encapsula (en funciones o clases) para poder reutilizarlo, según lo tienes cada vez que la llames creará de nuevo la conexión a MySQL

Sólo te daba mi punto de vista para simplificar y optimizar el código 🙂

Respecto al return, sí no me fijé muy bien, con que no es necesario quiero decir que el valor de una función es NULL por defecto, a no ser que tu devuelvas un valor. Si devuelves una variable no definida PHP mostrará un 'Warning'.

Esto

PHP:

if($con->query($sql)){
    $return = true;
} else {
    $return = NULL;
}

Se podría simplificar en

PHP:

$return = (boolean) $con->query($sql);

De modo que devolverías TRUE o FALSE, en vez de TRUE o NULL

Por otro lado ahora que me fijo creo (hace mucho que me pasé a PDO) que sobra `$res->close();` ya que no es la conexión a la BD

YeltsinReyes · 17 Sep 2017

[MENTION=36167]lobogris[/MENTION] gracias de nuevo man, tendre que apreder POD y pasame a el!!

Ves algun error en esta funcion?

YeltsinReyes

lobogris

YeltsinReyes

lobogris

YeltsinReyes

Temas similares