IdentifiantMot de passe
Loading...
Mot de passe oublié ?Je m'inscris ! (gratuit)
Navigation

Inscrivez-vous gratuitement
pour pouvoir participer, suivre les réponses en temps réel, voter pour les messages, poser vos propres questions et recevoir la newsletter

Langage PHP Discussion :

Votre avis critique sur cette classe 'Languages'


Sujet :

Langage PHP

Vue hybride

Message précédent Message précédent   Message suivant Message suivant
  1. #1
    Membre éprouvé
    Homme Profil pro
    Ingénieur en électrotechnique retraité
    Inscrit en
    Décembre 2008
    Messages
    1 718
    Détails du profil
    Informations personnelles :
    Sexe : Homme
    Âge : 73
    Localisation : France, Bas Rhin (Alsace)

    Informations professionnelles :
    Activité : Ingénieur en électrotechnique retraité

    Informations forums :
    Inscription : Décembre 2008
    Messages : 1 718
    Par défaut Votre avis critique sur cette classe 'Languages'
    Bonjour à tous,
    J'aimerais recueillir vos avis critiques sur cette classe.
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
    1
    2
    3
    4
    5
    6
    7
    8
    9
    10
    11
    12
    13
    14
    15
    16
    17
    18
    19
    20
    21
    22
    23
    24
    25
    26
    27
    28
    29
    30
    31
    32
    33
    34
    35
    36
    37
    38
    39
    40
    41
    42
    43
    44
    45
    46
    47
    48
    49
    50
    51
    52
    53
    54
    55
    56
    57
    58
    59
    60
    61
    62
    63
    64
    65
    66
    67
    68
    69
    70
    71
    72
    73
    74
    75
    76
    77
    78
    79
    80
    81
    82
    83
    84
    85
    86
    87
    88
    89
    90
    91
    92
    93
    94
    95
    96
    97
    98
    99
    100
    101
    102
    103
    104
    105
    106
    107
    108
    109
    110
    111
    112
    113
    114
    115
    116
    117
    118
    119
    120
    121
    122
    123
    124
    125
    126
    127
    128
    129
    130
    131
    132
    133
    134
    135
    136
    137
    138
    139
    140
    141
    142
    143
    144
    145
    146
    147
    148
    149
    150
    151
    152
    153
    154
    155
    156
    157
    <?php
     
    /* This class checks available languages of browser
     * and handle choice of preferred language
     * 
     * Version 2.0
     * Author Marc Paris
     * Language identifiers are according to ISO 639-1 standard
    */
     
     
    namespace Languages;
     
    class Languages
    {
     
    	/*
    	 string language selected language
    	 */
    	private $language;
     
    	/*
    	 string allowedLanguages
    	  (languages which are available on the website)
    	 */
    	private $allowedLanguages;
     
     
    	/*
    	 __construct save language in cookie
    	 if days is <0 cookie is deleted
    	 if days = 0 previous cookie is hold
    	 if days >0 cookie is overwrite
    	 */
    	public function __construct(int $days=0, $lang='')
    	{
    		if (empty($lang))
    			$lang = $this->setLanguage($this->getLanguages()[0][0]);
     
    		if (isset($_COOKIE['language']) && $days <0)
    		{
    			unset($_COOKIE['language']);
    		}
    		if (isset($_COOKIE['language']) && $days == 0)
    		{
    			if (strlen($_COOKIE['language']) == 2)
    				$this->setLanguage($_COOKIE['language']);
    			else
    				throw new \Exception(sprintf("Cookie language '%s' is unknown in %", $_COOKIE['language'], __METHOD__));
    		}
    		if ($days > 0)
    		{
    			setcookie('language', $lang, $days*24*3600);
    			$this->setLanguage($lang);
    		}
     
    	}
     
    	/*
    	 getLanguages search all available browser languages
    	 return userLanguages
    	 */
    	public static function getLanguages()
    	{
    		//check to see if browser languages are setted
    		if ( isset( $_SERVER["HTTP_ACCEPT_LANGUAGE"] ) )
    		{
    			$languages = strtolower( $_SERVER["HTTP_ACCEPT_LANGUAGE"] );
    			// need to remove spaces from strings to avoid error
    			$languages = str_replace( ' ', '', $languages );
    			$languages = explode( ",", $languages );
     
    			foreach ( $languages as $language )
    			{
    				// pull out the language, place languages into array of full and primary
    				// string structure:
    				$temp_array = [];
    				// slice out the part before ; on first step, the part before - on second, place into array
    				$temp_array[0] = substr( $language, 0, strcspn( $language, ';' ) );//full language
    				$temp_array[1] = substr( $language, 0, 2 );// cut out primary language
    				//place this array into main $userLanguages language array
    				$userLanguages[] = $temp_array;
    			}
    			unset($language);
    		}
    		// else if no language found
    		else
    		{
    			$userLanguages[0] = array( '','','','' ); //return blank array
    		}
     
    		return $userLanguages;
    	}
     
    	/*
    	 setAllowedLanguages
    	 parameter langs: languages to be allowed
    	 */
    	public function addAllowedLanguages(string|array $lang)
    	{
    		if (is_string($lang))
    			$lang = (array) $lang;
    		foreach($lang as $lng)
    		{
    			if (strlen($lng) != 2)
    				throw new \Exception(sprintf("Wrong parameter value in %s. Each item must be 2 characters long.",__METHOD__));
    		}
    		$this->allowedLanguages[] = $lang;
    	}
     
    	/*
    	 getAllowedLanguages
    	 return allowed languages
    	 */
    	public function getAllowedLanguages()
    	{
    		return $this->allowedLanguages;
    	}
     
    	/*
    	 setLanguage set selected Language
    	 parameter lang: language to be setted
    	 */
    	public function setLanguage(string $lang)
    	{
    		if (strlen($lang) != 2)
    			throw new \Exception(sprintf("Wrong parameter value in %s. Parameter must be 2 characters long.",__METHOD__));
    		$this->language = $lang;
    		if(!checkLanguage($lang))
    			return false;
    		return true;
    	}
     
    	/*
    	 checkLanguage checks if selected Language is allowed
    	 parameter lang: language to be checked
    	 */
    	public function checkLanguage(string $lang)
    	{
    		if(in_array($lang,$this->allowedLanguages))
    			return true;
    		return false;
    	}
     
    	/*
    	 getLanguage
    	 return selected language
    	 */
    	public function getLanguage()
    	{
    		return $this->language;
    	}
     
    }
     
    // make the class directly available on the global namespace
    class_alias('Languages\Languages', 'Languages', false);

  2. #2
    Membre Expert
    Profil pro
    Inscrit en
    Mai 2006
    Messages
    721
    Détails du profil
    Informations personnelles :
    Localisation : Belgique

    Informations forums :
    Inscription : Mai 2006
    Messages : 721
    Par défaut
    Bonjour,

    En êtes-vous l'auteur ?
    Il serait intéressant de décrire en deux lignes ce qu'elle fait, ainsi on pourrait l'analyser pour déterminer si l'implémentation est conforme au but fixé.

    Pas que je suis fainéante, j'aime faire du code review mais j'aime bien connaître le contexte, et même la raison (quel est le problème que l'on veut résoudre).

  3. #3
    Membre Expert
    Avatar de cavo789
    Homme Profil pro
    Développeur Web
    Inscrit en
    Mai 2004
    Messages
    1 797
    Détails du profil
    Informations personnelles :
    Sexe : Homme
    Localisation : Belgique

    Informations professionnelles :
    Activité : Développeur Web

    Informations forums :
    Inscription : Mai 2004
    Messages : 1 797
    Par défaut
    Je suis fainéant itou et ce code, au niveau de sa syntaxe (et j'ai juste regardé ça) est vieux.

    Sous php 8 on peut faire nettement mieux (clean code). Donc, mon avis : il s'agit d'un vieux code abandonné et qui donnera des soucis très vite.

  4. #4
    Membre éprouvé
    Homme Profil pro
    Ingénieur en électrotechnique retraité
    Inscrit en
    Décembre 2008
    Messages
    1 718
    Détails du profil
    Informations personnelles :
    Sexe : Homme
    Âge : 73
    Localisation : France, Bas Rhin (Alsace)

    Informations professionnelles :
    Activité : Ingénieur en électrotechnique retraité

    Informations forums :
    Inscription : Décembre 2008
    Messages : 1 718
    Par défaut
    Dans un premier temps merci pour vos remarques.
    La méthode getLanguages() provient d'un code récupéré que j'ai adapté et modifié. Le reste est de mon cru.
    Le but de cette classe est de gérer la langue à utiliser pour un site en fonction de la ou des langues utilisateur (en paramètre du navigateur), des traductions disponibles sur le site (méthodes *allowedLanguages) et d'un éventuel choix de langue par l'utilisateur. Le constructeur recherche si une langue est déjà enregistrée en cookie et la modifie éventuellement.
    J'ai essayé de mettre des explications en tête de chaque fonction lorsque cela me paraissait nécessaire.

  5. #5
    Membre Expert
    Avatar de cavo789
    Homme Profil pro
    Développeur Web
    Inscrit en
    Mai 2004
    Messages
    1 797
    Détails du profil
    Informations personnelles :
    Sexe : Homme
    Localisation : Belgique

    Informations professionnelles :
    Activité : Développeur Web

    Informations forums :
    Inscription : Mai 2004
    Messages : 1 797
    Par défaut
    Le code n'est donc pas abandonné 🤭

    Tu écris en php comme on pouvait le faire en php 5... Le langage a énormément évolué depuis et la qualité du code, aujourd'hui, est bien meilleure et robuste.

    Tu oublies les types de données, les return type, la syntaxe des if est incorrecte (il faut toujours passe à la ligne), etc. Beaucoup de choses pourraient être amélioré.

  6. #6
    Membre Expert
    Profil pro
    Inscrit en
    Mai 2006
    Messages
    721
    Détails du profil
    Informations personnelles :
    Localisation : Belgique

    Informations forums :
    Inscription : Mai 2006
    Messages : 721
    Par défaut
    Disclaimer: je n'ai pas cherché à faire tourner ce code, donc je pars du principe qu'il fonctionne comme prévu et qu'il n'y a pas de gros bug évident.

    Commentaires en vrac, sans ordre particulier.

    Il y a très longtemps, j'ai codé un truc un peu similaire mais plus basique, qui consistait en gros à parser HTTP_ACCEPT_LANGUAGE et renvoyer un tableau associatif avec les langues et le facteur de pondération. De mémoire, c'était un regex en une ligne pour parser.
    La décision de rediriger le client, je la faisais hors de ma fonction.

    Typiquement, sur les sites que j'ai créé, une fois le langage détecté ou choisi par le visiteur, je redirige vers domain.com/en, /fr etc.
    Un cookie n'est pas forcément requis si on fait des redirections.
    Donc pour moi ce code n'est pas utile et ne m'apporte pas de flexibilité.


    Le nom de la classe est beaucoup trop générique et pas assez descriptif.
    Je l'appellerais peut-être BrowserLanguage ?

    Il y a un peu de répétition, par exemples lignes 37-44:
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
    1
    2
    3
    4
    5
    6
    7
    8
    9
    10
    11
     
    if (empty($lang))
        $lang = $this->setLanguage($this->getLanguages()[0][0]);
     
    if (isset($_COOKIE['language']) && $days <0)
    {
        unset($_COOKIE['language']);
    }
    if (isset($_COOKIE['language']) && $days == 0)
    {
    ...
    On peut aisément refactorer cette section pour supprimer les ifs inutiles.


    Je pense qu'il ne faut pas abréger les noms de variables inutilement, économiser quelques lettres n'apporte rien et diminue la lisibilité:
    => appelons la variable $language tout simplement

    Il y a une utilisation abusive de unset, par exemple unset($_COOKIE['language']); et puis unset($language); ailleurs.
    Les flux de décisions sont dispersés.


    Au niveau des conventions de nommage, je pense que les noms de fonctions devraient être plus explicites.
    Par exemple getLanguages devrait plutôt s'appeler getBrowserLanguages, car sans regarder les commentaires on n'a aucune idée a priori de ce que fait cette fonction.

    C'est d'autant plus important si on veut publier le code dans un git public, pour que quelqu'un ait envie de le forker et contribuer il vaut mieux que le code soit intuitif pour les profanes. Et évidemment, on codera de préférence en anglais, ce qui est le cas ici (et logique pour un code dont l'objectif est de gérer l'internationalisation).

    Si on reste dans la même veine, checkLanguage pourrait s'appeler isLanguageAllowed car "check language" n'est pas du tout évocateur. Sans regarder le code on ne peut pas deviner ce que ça fait. Si la fonction est nommée avec soin, le code devient plus parlant et moins confus.
    Heureusement qu'il y a des commentaires.

    De même private $language; devient: private $selectedLanguage; en respectant votre convention de nommage.

    Ce qui me dérange un peu c'est qu'on fait un setcookie dans __construct, je m'attendrais plutôt à ce que ça se fasse dans setLanguage, qui du reste est appelé dans la foulée (L53-54). En réalité, __construct fait déjà trop de choses et setLanguage n'a rien à y faire. C'est juste l'endroit où on initialise la classe et ce n'est pas un endroit où on implemente le "business logic".
    Pire, cette fonction y apparaît non pas une fois mais trois.

    La fonction addAllowedLanguages semble actuellement inutilisée, si ce code est complet. Ça ressemble à un artefact d'un dév précédent, il faut donc supprimer le "dead code" qui est une distraction, éventuellement le committer dans son git pour garder l'historique.
    getAllowedLanguages: pareil... non référencé ailleurs, donc à nettoyer.

    Je ne suis pas tellement emballée par ce code. Je pense que je chercherais d'autres implémentations, quitte à l'améliorer à ma sauce.
    Ce n'est pas une classe structurée dans les règles de l'art, mais je pense aussi que ça a été codé avant de mettre les idées à plat.

    Comme quoi, même dans un code succint on peut trouver beaucoup de choses à commenter

+ Répondre à la discussion
Cette discussion est résolue.

Discussions similaires

  1. Votre avis/critiques sur mon CV
    Par riyahi dans le forum CV
    Réponses: 26
    Dernier message: 02/12/2008, 15h51
  2. Votre avis svp sur ce pc portable
    Par Amalsim dans le forum Ordinateurs
    Réponses: 1
    Dernier message: 14/09/2008, 15h06
  3. Besoin de votre avis svp sur mon site www.monwebcv.com
    Par italiasky dans le forum Mon site
    Réponses: 14
    Dernier message: 19/03/2007, 11h57

Partager

Partager
  • Envoyer la discussion sur Viadeo
  • Envoyer la discussion sur Twitter
  • Envoyer la discussion sur Google
  • Envoyer la discussion sur Facebook
  • Envoyer la discussion sur Digg
  • Envoyer la discussion sur Delicious
  • Envoyer la discussion sur MySpace
  • Envoyer la discussion sur Yahoo