kohntark
Messages postés3705Date d'inscriptionlundi 5 juillet 2004StatutMembreDernière intervention27 avril 201230 16 sept. 2009 à 20:44
Salut,
A mon avis ta fonction pourrait faire 2 fois moins de lignes (voire plus), et surtout être plus rapide.
str_split consomme "beaucoup" de ressources. Pour qq chaînes de longueurs raisonnables ça ne change rien mais si cette fonction est présente partout dans ton code ça risque de poser problème. Imagine plusieurs chaines de 1000 caractères => à chaque fois c'est un tableau de 1000 valeurs en mémoire.
Par exemple $dossier_decom = str_split($dossier);[...] à quoi bon ?
Pourquoi pas :$dossier (substr($dossier,-1) '/') ? $dossier : $dossier.'/';
Comme cela a été dit, même chose pour l'extension. Il y a pas mal de solutions, ça par exemple :
$extension_possible = array('jpg','jpeg','bmp','png','gif');
if (in_array(pathinfo($complete_file_path, PATHINFO_EXTENSION), $extension_possible)) { ...
Ca te permettrait, moyennant quelques modifs, de ne passer que 2 arguments à la fonction.
Ou encore l'extension fileinfo (>= 5.3 sinon nécessite une bibliothèque)
Tu devrais laisser la possibilité de choisir le css.
Bref, il y aurait d'autres choses à optimiser (comme les car spéciaux par exemple), mais avant cela il faudrait définir la cible :
- est ce une fonction qui permet, au travers des données venant de l'utilisateur, d'afficher des images ?
- est ce une fonction qui n'est destinée qu'au webmaster pour lui éviter d'entrer trop de code ?
Dans le second cas son intérêt est limité. Personnellement je préfère coder les images "en dur" Ceci étant, elle peut trouver son intérêt si le contenu est dynamique, mais dans ce cas je suis de l'avis de Altalavista : une doc suffisante et la fonction est 4 fois moins longue.
Dans le premier cas il manque pas mal de tests (chemin url valide, traitements de la chaîne postée ou "gettée", etc ...)
Cordialement,
Kohntark-
cs_Astalavista
Messages postés192Date d'inscriptionlundi 24 décembre 2001StatutMembreDernière intervention 3 février 2010 16 sept. 2009 à 18:51
Au final, il faudrait mieux faire une doc pour la fonction et exiger une extension sans . et on fait juste un in_array et si non, une erreur... sa éviterais un surplus de commentaire .... XD
kankrelune
Messages postés1293Date d'inscriptionmardi 9 novembre 2004StatutMembreDernière intervention21 mai 2015 16 sept. 2009 à 18:07
oui sauf que dans le cas d'une vérif d'extension on se fout d'avoir "htaccess" ou "" pour faire la vérif et on économise une regexp... .. . ;o)
@ tchaOo°
neigedhiver
Messages postés2480Date d'inscriptionjeudi 30 novembre 2006StatutMembreDernière intervention14 janvier 201119 16 sept. 2009 à 14:12
Ben oui... ton code retournera 'htaccess' pour un fichier .htaccess
Et d'une manière générale, il retournera le nom du fichier si c'est un fichier caché...
kankrelune
Messages postés1293Date d'inscriptionmardi 9 novembre 2004StatutMembreDernière intervention21 mai 2015 16 sept. 2009 à 11:12
Une regexp pour récupérer une extension de fichier Ô_o
Pour ce qui est du trigger_error tout dépend de la manière dont tu gère tes erreurs... ne pas passer pas le gestionnaire d'erreur de php est justifié si tu gère toi même tes erreurs mais dans ce cas il faut mettre en place un moyen de les récupérer par exemple en passant $erreur_script par référence en paramètre de ta fonction... .. .
@ tchaOo°
neigedhiver
Messages postés2480Date d'inscriptionjeudi 30 novembre 2006StatutMembreDernière intervention14 janvier 201119 16 sept. 2009 à 09:49
Euh ouais... Je me suis arrêté à "extension d'un fichier" lol
Mais euh une image peut avoir une extension jpeg malgré tout. Ouais, j'aurais mieux fait de me coucher plutôt que de poster un commentaire somme toute pas franchement indispensable ^^
Damtux972
Messages postés1Date d'inscriptionmardi 25 mars 2008StatutMembreDernière intervention16 septembre 2009 16 sept. 2009 à 03:26
@Astalavista, ben pour etre sincère, moi non plus je ne sais pas que fait $erreur_script.
@neigedhiver, parcontre je ne vois pas trop l'interet d'autoriser toute les extensions puisque son but est de remplacer une lettre en image. Mais pourquoi pas, si sa fait plaisir.
@all : J'ai bien conscience que la vérification de l'extension est limité et qu'elle a été faite à l'arrache.
Merci d'avoir pris le temps de lire ce code tout bête et de me dire ce qui n'allait pas. Je l'ai coder sur un coup de tête (en janvier 2008) sans prendre le temps d'y revenir dessus, je vais l'améliorer quand j'aurais du temps libre. Merci les gars ^^
neigedhiver
Messages postés2480Date d'inscriptionjeudi 30 novembre 2006StatutMembreDernière intervention14 janvier 201119 16 sept. 2009 à 01:37
Salut,
Pour vérifier l'extension d'un fichier, il ne faut pas faire comme ça... Et si l'extension fait 4 voire 5 caractères ? => .jpeg, .mpeg, .html, .xhtml
Et si le fichier est un fichier caché ? => .htaccess
Bref... Le mieux est, à mon sens, d'utiliser une expression régulière... Par exemple :
$extension = '';
if (preg_match('`.+\.([^\.]*)$`', $nom_fichier, matches) {
$extension = $matches[1];
}
Cette expression régulière garde tous les caractères présents après le dernier point, à condition que celui-ci ne soit pas le premier caractère de la chaîne.
cs_Astalavista
Messages postés192Date d'inscriptionlundi 24 décembre 2001StatutMembreDernière intervention 3 février 2010 16 sept. 2009 à 00:29
Je te donne mon avis sur ton code :
- Je ne voit pas ce que $erreur_script fait dans ta fonction :) utilise trigger_error plutôt
- Ta vérification de l'extension n'est pas très optimisé, tu pourrais faire ca par exemple :
if(!in_array(substr($extension, -3), $extension_possible))echo "extension non pris en charge...";
- Pour ta boucle final, tu devrais enlever ton if a l'intérieur en initialisant ton $retour a une chaine sans rien : $retour = ""; juste avant, sa évite un if a tester a chaque fois ... Mais sinon, ta boucle pourrais être enlever, regarde voir la fonction preg_replace, tu pourra faire tout ca en une ligne :)
16 sept. 2009 à 20:44
A mon avis ta fonction pourrait faire 2 fois moins de lignes (voire plus), et surtout être plus rapide.
str_split consomme "beaucoup" de ressources. Pour qq chaînes de longueurs raisonnables ça ne change rien mais si cette fonction est présente partout dans ton code ça risque de poser problème. Imagine plusieurs chaines de 1000 caractères => à chaque fois c'est un tableau de 1000 valeurs en mémoire.
Par exemple $dossier_decom = str_split($dossier);[...] à quoi bon ?
Pourquoi pas :$dossier (substr($dossier,-1) '/') ? $dossier : $dossier.'/';
Comme cela a été dit, même chose pour l'extension. Il y a pas mal de solutions, ça par exemple :
$extension_possible = array('jpg','jpeg','bmp','png','gif');
if (in_array(pathinfo($complete_file_path, PATHINFO_EXTENSION), $extension_possible)) { ...
Ca te permettrait, moyennant quelques modifs, de ne passer que 2 arguments à la fonction.
Ou encore l'extension fileinfo (>= 5.3 sinon nécessite une bibliothèque)
Tu devrais laisser la possibilité de choisir le css.
Bref, il y aurait d'autres choses à optimiser (comme les car spéciaux par exemple), mais avant cela il faudrait définir la cible :
- est ce une fonction qui permet, au travers des données venant de l'utilisateur, d'afficher des images ?
- est ce une fonction qui n'est destinée qu'au webmaster pour lui éviter d'entrer trop de code ?
Dans le second cas son intérêt est limité. Personnellement je préfère coder les images "en dur" Ceci étant, elle peut trouver son intérêt si le contenu est dynamique, mais dans ce cas je suis de l'avis de Altalavista : une doc suffisante et la fonction est 4 fois moins longue.
Dans le premier cas il manque pas mal de tests (chemin url valide, traitements de la chaîne postée ou "gettée", etc ...)
Cordialement,
Kohntark-
16 sept. 2009 à 18:51
16 sept. 2009 à 18:07
@ tchaOo°
16 sept. 2009 à 14:12
Et d'une manière générale, il retournera le nom du fichier si c'est un fichier caché...
16 sept. 2009 à 11:12
$extension = '';
if (($pos = strrpos($nom_fichier, '.')) !== false)
$extension = substr($nom_fichier,$pos+1);
}
Pour ce qui est du trigger_error tout dépend de la manière dont tu gère tes erreurs... ne pas passer pas le gestionnaire d'erreur de php est justifié si tu gère toi même tes erreurs mais dans ce cas il faut mettre en place un moyen de les récupérer par exemple en passant $erreur_script par référence en paramètre de ta fonction... .. .
@ tchaOo°
16 sept. 2009 à 09:49
Mais euh une image peut avoir une extension jpeg malgré tout. Ouais, j'aurais mieux fait de me coucher plutôt que de poster un commentaire somme toute pas franchement indispensable ^^
16 sept. 2009 à 03:26
@neigedhiver, parcontre je ne vois pas trop l'interet d'autoriser toute les extensions puisque son but est de remplacer une lettre en image. Mais pourquoi pas, si sa fait plaisir.
@all : J'ai bien conscience que la vérification de l'extension est limité et qu'elle a été faite à l'arrache.
Merci d'avoir pris le temps de lire ce code tout bête et de me dire ce qui n'allait pas. Je l'ai coder sur un coup de tête (en janvier 2008) sans prendre le temps d'y revenir dessus, je vais l'améliorer quand j'aurais du temps libre. Merci les gars ^^
16 sept. 2009 à 01:37
Pour vérifier l'extension d'un fichier, il ne faut pas faire comme ça... Et si l'extension fait 4 voire 5 caractères ? => .jpeg, .mpeg, .html, .xhtml
Et si le fichier est un fichier caché ? => .htaccess
Bref... Le mieux est, à mon sens, d'utiliser une expression régulière... Par exemple :
$extension = '';
if (preg_match('`.+\.([^\.]*)$`', $nom_fichier, matches) {
$extension = $matches[1];
}
Cette expression régulière garde tous les caractères présents après le dernier point, à condition que celui-ci ne soit pas le premier caractère de la chaîne.
Ainsi :
'fichier.html' => 'html'
'.htaccess' => ''
'.fichier.cache' => 'cache'
16 sept. 2009 à 00:29
- Je ne voit pas ce que $erreur_script fait dans ta fonction :) utilise trigger_error plutôt
- Ta vérification de l'extension n'est pas très optimisé, tu pourrais faire ca par exemple :
if(!in_array(substr($extension, -3), $extension_possible))echo "extension non pris en charge...";
- Pour ta boucle final, tu devrais enlever ton if a l'intérieur en initialisant ton $retour a une chaine sans rien : $retour = ""; juste avant, sa évite un if a tester a chaque fois ... Mais sinon, ta boucle pourrais être enlever, regarde voir la fonction preg_replace, tu pourra faire tout ca en une ligne :)