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 Perl Discussion :

Revue de code


Sujet :

Langage Perl

Vue hybride

Message précédent Message précédent   Message suivant Message suivant
  1. #1
    Membre confirmé
    Homme Profil pro
    Ingénieur intégration
    Inscrit en
    Mars 2015
    Messages
    138
    Détails du profil
    Informations personnelles :
    Sexe : Homme
    Localisation : France, Morbihan (Bretagne)

    Informations professionnelles :
    Activité : Ingénieur intégration
    Secteur : Service public

    Informations forums :
    Inscription : Mars 2015
    Messages : 138
    Par défaut Revue de code
    Bonjour,

    en poste depuis 3 ans au service de la production, il m'a été demandé à mon arrivée de développer des scripts batchs facilement portables sur les différentes plateformes (Aix, GNU/Linux, Microsoft Windows). Les précédents scripts étaient écrits en ksh ou Powershell.

    Ayant de vagues connaissances de Perl, je m'y suis remis grâce aux tutos du site ( à Sylvain Lhullier et aux autres rédacteurs ). Les résultats sont bons et le choix a été officiellement validé.

    Je dois commencer à former quelques personnes sur le langage ; par contre, mon code n'ai jamais été audité et je voulais savoir si cela est possible ici et si oui, de quelle manière (coller le code, pièces-jointes, en privé...).
    J'ai réécrit les jours derniers un modèle de script et 2 modules maisons (entre 100 et 150 lignes pour chaque éléments, lignes blanches incluses).
    Ces éléments s'exécutent correctement, mais je suis intéressé par tout commentaire, suggestion, critique, correction sur les règles de codage, nommage, sémantique, syntaxe utilisés.

    Merci

  2. #2
    Modérateur
    Avatar de kolodz
    Homme Profil pro
    Ingénieur développement logiciels
    Inscrit en
    Avril 2008
    Messages
    2 209
    Détails du profil
    Informations personnelles :
    Sexe : Homme
    Âge : 38
    Localisation : France, Rhône (Rhône Alpes)

    Informations professionnelles :
    Activité : Ingénieur développement logiciels
    Secteur : High Tech - Produits et services télécom et Internet

    Informations forums :
    Inscription : Avril 2008
    Messages : 2 209
    Billets dans le blog
    52
    Par défaut
    Bonjour,

    Si ton code est relativement court, il t'es possible d’utilisé la balise [CODE]. Il est préférable d'éviter les pièces jointes. Peu de personnes iront regarder des pièces jointes.



    Note : Peut-être faire attention, si les script contiennent des informations sensible.

    Cordialement,
    Patrick Kolodziejczyk.
    Si une réponse vous a été utile pensez à
    Si vous avez eu la réponse à votre question, marquez votre discussion
    Pensez aux FAQs et aux tutoriels et cours.

  3. #3
    Rédacteur/Modérateur

    Avatar de Lolo78
    Homme Profil pro
    Conseil - Consultant en systèmes d'information
    Inscrit en
    Mai 2012
    Messages
    3 612
    Détails du profil
    Informations personnelles :
    Sexe : Homme
    Localisation : France, Yvelines (Île de France)

    Informations professionnelles :
    Activité : Conseil - Consultant en systèmes d'information
    Secteur : High Tech - Opérateur de télécommunications

    Informations forums :
    Inscription : Mai 2012
    Messages : 3 612
    Billets dans le blog
    1
    Par défaut
    Oui, poste simplement ton code entre balises de code s'il n'est pas trop long (en anonymisant ce qui doit l'être, le cas échéant).

    On regardera.

    Commence déjà avec un seul script, il y aura peut-être des remarques générale qui s'appliqueront aussi aux autres.

  4. #4
    Membre confirmé
    Homme Profil pro
    Ingénieur intégration
    Inscrit en
    Mars 2015
    Messages
    138
    Détails du profil
    Informations personnelles :
    Sexe : Homme
    Localisation : France, Morbihan (Bretagne)

    Informations professionnelles :
    Activité : Ingénieur intégration
    Secteur : Service public

    Informations forums :
    Inscription : Mars 2015
    Messages : 138
    Par défaut Revue de code - squelette script principal
    Merci pour votre aide, voici dans un premier temps le modèle de script principal :

    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
     
    #!/usr/bin/perl -w
    #============================================================================
    #@(#) Procédure   : skel4.pl
    #@(#) Description : ...
    #@(#)
    #@(#) Pré-requis  : module Perl Shell::Source pour sourcer l'envbatch.
    #@(#)                   initialiser variable PERL5LIB dans ~/.profile pour déclarer
    #@(#)                    le répertoire contenant les modules maison
    #@(#)
    #@(#) Auteur      : xxx
    #@(#) Arguments   :  1 - reprise = balise de début ou ""
    #@(#)                      2 - fin     = balise de fin ou ""
    #@(#)
    #@(#) Historique  : jj/mm/aaaa début écriture programme
    #@(#)               ...
    #============================================================================
    use strict;
    use Cwd;
    use Shell::Source;
    use feature qw(switch say);
    use xxx_utils_v4;
    use oxa_utils_v4;
     
     
     
     
    # initialisation constantes, variables
    use constant h_const => {
        dft_reprise => "debut",
        dft_fin     => "fin",
        dft_nb_args => 2,       # nb arguments par défaut
        nb_args     => 0,       # nb arguments supplémentaires attendus
        domaine     => "PROD",
        application => "TECH",
        na          => "N/A",
    };
     
    our ( $script_absolute_name, $script_relative_name, $script_dir, $script_name, $script_extension, $script_user ) = utils_script($0);
    my $pid = $$;
    my ( $reprise, $fin );
     
    # Source envbatch et envadmin et initialisation variables (envexe sourcé dans .profile)
    my $envbatch = Shell::Source->new(shell => "ksh", file => "$ENV{'PROD_TECH_ENV'}/envbatch");
    $envbatch->inherit;
     
    my  $project        = $ENV{'PROJECT'};
    my  $prod_tech_env  = $ENV{'PROD_TECH_ENV'};
    our $dir_log_oxalys = $ENV{'REP_LOG_OXALYS'};
     
    # Initialisation et création fichier log, puis redirection STDOUT et STDERR
    our ( $basename_log, $log_file ) = utils_log( $script_name, $pid );
    open( my $log, ">", $log_file ) || die "FATAL: cannot open \"$log_file\" for writing: $!\n";
    my $r_log = \*$log;    # référence vers le fichier de log
    select $log;*STDERR = *$r_log;
     
    # Initialisation oxa
    oxa_main(); # pas vraiment utile, juste pour faire comme si ;-)
     
     
     
     
    # Fonctions internes
    sub usage {
        oxa_msg_err( $r_log, 0, "KO - Usage : $script_relative_name <balise de début ou \"\"> <balise de fin ou \"\">",1 );
    }
     
     
    sub controle_balises {
        my ( $balise_debut, $balise_fin, $r_balises_ok ) = @_;
     
        my $flag_err = 0;
     
        foreach my $tag ( $balise_debut, $balise_fin ) {
            if ( ! utils_ctrl_balise( $tag, $r_balises_ok ) ) {
                oxa_msg_warn( $r_log, 1, "KO - Balise $tag incorrecte" );
                $flag_err++;
            }
        }
     
        oxa_msg_err( $r_log, 1, "KO - Balises autorisées : @$r_balises_ok", 1 ) if $flag_err ;
     
        return;
    }
     
     
    sub next_step {
        my ( $step ) = @_;
        if( $reprise eq $fin ) { $reprise = 'fin' } else { $reprise = $step } ;
        return $reprise;
    }
     
     
    sub process {
        my $dummy = 0;
        return;
    }
     
    # main ()
    # - Initialisation et contrôle du nombre d'arguments attendus
    # - Initialisation entêtes OXA
    # - Initialisation variables contenant les arguments
    oxa_debut      ( $r_log, $script_absolute_name );
    oxa_msg_info ( $r_log, 0, "Script de xxxxxx à compléter xxxxxxxxxxxxx" );
    oxa_msg_info ( $r_log, 0, "Fichier de log ... $log_file" );
    oxa_app         ( $r_log, h_const->{domaine}, h_const->{application} );
     
    usage if utils_nb_args() != h_const->{dft_nb_args} + h_const->{nb_args} ;
     
    my $r_balises_ok = ["debut","fin","traitement"];  # réf. anonyme vers tableau des balises autorisées
     
     
    # Tests préliminaires
    # Contrôle balises reçues en arguments
    if ( $ARGV[0] eq "" ) { $reprise = h_const->{dft_reprise} } else { $reprise = $ARGV[0] };
    if ( $ARGV[1] eq "" ) { $fin     = h_const->{dft_fin} }         else { $fin     = $ARGV[1] };
    controle_balises( $reprise, $fin, $r_balises_ok );
     
     
    # Contrôles spécifiques à placer ci-après
     
     
     
     
    # Boucle de traitement principale
    until ( $reprise eq "fin" ) {
        oxa_msg_info( $r_log, 1,"LABEL : $reprise" );
        given ($reprise) {
            when ('debut') {
                oxa_msg_info( $r_log, 1, "===> Début de traitement" );
                next_step("traitement");
            }
            when ('traitement') {
                oxa_msg_info( $r_log, 1, "===> Traitement" );
                process();
                next_step("fin_traitement");
            }
            when ('fin_traitement') {
                oxa_msg_info( $r_log, 1, "===> Fin de traitement" );
                next_step("fin");
            }
            default {
                say "KO - Etiquette $reprise invalide";
                oxa_msg_err( $r_log, 1, "KO - Etiquette $reprise invalide", 99 );
                next_step("fin");
            }
        }
    }
     
     
    # Footer OXA et sortie normale
    oxa_msg_info( $r_log, 1, "--------------------------------------------" );
    oxa_msg_info( $r_log, 1, "                FIN $script_name");
    oxa_fin( $r_log, 0 );

  5. #5
    Modérateur
    Avatar de kolodz
    Homme Profil pro
    Ingénieur développement logiciels
    Inscrit en
    Avril 2008
    Messages
    2 209
    Détails du profil
    Informations personnelles :
    Sexe : Homme
    Âge : 38
    Localisation : France, Rhône (Rhône Alpes)

    Informations professionnelles :
    Activité : Ingénieur développement logiciels
    Secteur : High Tech - Produits et services télécom et Internet

    Informations forums :
    Inscription : Avril 2008
    Messages : 2 209
    Billets dans le blog
    52
    Par défaut
    Pour moi, c'est du code relativement propre.

    Note : je ne suis pas développeur perl !

    Je n'ai que 2 points de détails à mentionné :

    1. Les variable magique.
    Que cela soit des String ou des nombres, il me semble préférable d'avoir une variable, une constante ou enum pour cela. Car, il y a toujours le problème du c'est la même string sauf que non.

    2. Consistance de l'indentation des if (en une ligne ou non)
    C'est du détail en particulier dans ce cas, où tout est proprement indenté.

    Cordialement,
    Patrick Kolodziejczyk.
    Si une réponse vous a été utile pensez à
    Si vous avez eu la réponse à votre question, marquez votre discussion
    Pensez aux FAQs et aux tutoriels et cours.

  6. #6
    Rédacteur/Modérateur

    Avatar de Lolo78
    Homme Profil pro
    Conseil - Consultant en systèmes d'information
    Inscrit en
    Mai 2012
    Messages
    3 612
    Détails du profil
    Informations personnelles :
    Sexe : Homme
    Localisation : France, Yvelines (Île de France)

    Informations professionnelles :
    Activité : Conseil - Consultant en systèmes d'information
    Secteur : High Tech - Opérateur de télécommunications

    Informations forums :
    Inscription : Mai 2012
    Messages : 3 612
    Billets dans le blog
    1
    Par défaut
    Bonjour,

    dans l'ensemble, le code est propre.

    Il y a pas mal de choses faites dans des modules non présentés, je ne peux pas commenter là-dessus.

    Quelques remarques sur les "bonnes pratiques" actuelles et d'autres points parfois assez secondaires, mais pas toujours.
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
    1
    2
     
    #!/usr/bin/perl -w
    L'utilisation de l'option "-w" est périmée depuis une dizaine d'années, sauf pour les scripts unilignes. Il est bien préférable d'ajouter le pragma:
    vers le début de ton code. L'avantage principal, par rapport à l'option "-w" est que le pragma "use warnings" est lexical et n'est pas propagé automatiquement aux modules qui pourraient générer des warnings ou erreurs intempestifs. Il est également possible de le désactiver localement au besoin. Je sais que le tutoriel de Sylvain utilisait cette syntaxe, mais il l'a mis à jour assez récemment et a notamment remplacé les "-w" par des "use warnings;". Je t'engage à télécharger la dernière version de son tuto.
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
    1
    2
     
    our ( $script_absolute_name, $script_relative_name, $script_dir, $script_name, $script_extension, $script_user ) = utils_script($0);
    Je ne vois aucune raison d'utiliser "our" pour ces variables, il est bien préférable d'utiliser des variables lexicales (déclarées avec "my"). Cette remarque s'applique aux autres cas où tu utilises "our", qui ne sont, sauf erreur, pas justifiés. (Mais je n'ai pas vu ce que font tes modules. S'ils utilisent ou modifient ces variables, ce n'est peut-être pas une bonne idée, il est généralement préférable de passer les variables et les valeurs de retour, mais ce n'est pas nécessairement une règle absolue.)
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
    1
    2
    3
     
    my $envbatch = Shell::Source->new(shell => "ksh", file => "$ENV{'PROD_TECH_ENV'}/envbatch");
    $envbatch->inherit;
    Je doute que ce soit très portable sous Windows.
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
    1
    2
     
    # Initialisation et création fichier log, puis redirection STDOUT et STDERR
    Tu ne rediriges que STDERR, pas STDOUT.
    Par ailleurs, pourquoi rediriger STDERR? Ne peux-tu pas t'arranger pour écrire directement là où tu as besoin? Je pose la question parce que l'utilisation de select et de "*STDERR = *$r_log;" est assez "magique" et peut-être à éviter quand on peut.
    Est-ce que ça compile? Est-ce que ça sert à quelque chose?
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
    1
    2
    3
    4
    5
     
    sub process {
        my $dummy = 0;
        return;
    }
    A quoi sert cette fonction?

    Code : Sélectionner tout - Visualiser dans une fenêtre à part
    1
    2
     
    if ( $ARGV[0] eq "" ) { $reprise = h_const->{dft_reprise} } else { $reprise = $ARGV[0] };
    Perso, je réécrirais:
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
    1
    2
    3
    4
    5
    6
     
    if ( $ARGV[0] eq "" ) { 
        $reprise = h_const->{dft_reprise};
    } else { 
        $reprise = $ARGV[0];
    }
    Mais c'est peut-être une affaire de goût personnel.

    Code : Sélectionner tout - Visualiser dans une fenêtre à part
    1
    2
    3
    4
    5
     
    use feature qw(switch say);
    ...
    when ('debut') {
    ...
    L'utilisation du switch a été "rétogradée" au rang de "fonctionnalité expérimentale" suscitant un warnings à partir, je crois, de la version 5.18 et susceptible de disparaître dans une prochaine version de Perl. A éviter donc pour du code supposé être pérenne.

    Il vaut mieux écrire des if successifs:
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
    1
    2
    3
    4
    5
    6
    7
    8
    9
    10
     
    if ($reprise eq 'debut') {
        # ....
    } elsif ($reprise eq 'traitement {
        # ....
    } # ...
    } else {
        say "KO - Etiquette $reprise invalide";
        # ...
    }
    Si tu trouves la syntaxe un peu lourdingue, elle peut éventuellement être simplifiée comme suit:
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
    1
    2
    3
    4
    5
    6
    7
    8
     
    for ($reprise) { # reprise est maintenant dans $_
        if (/debut/) {
            # ...
        } elsif (/traitement/) {
            # ...
        # ....
    }
    L'autre solution, un peu plus "avancée", est d'utiliser une table de distribution, c'est à dire un hachage contenant des références vers le code à exécuter. Un truc du genre de ce qu'il y a ci-dessous, que je ne peux pas tester (donc il y a peut-être des erreurs) car je n'ai qu'une partie de ton code et n'ai pas les données, mais ça donnera l'idée de base.

    Initialiser un hachage comme suit:
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
    1
    2
    3
    4
    5
    6
    7
    8
    9
    10
    11
    12
    13
     
    my %r_balises_ok = 
        ("debut" =>
            sub { 
                oxa_msg_info( $r_log, 1, "===> Début de traitement" );
                next_step("traitement");
            },
         "traitement" =>
             sub { #....
            },
        "fin" => 
            # ...
        );
    Ensuite, dans la partie principale du traitement, un truc du genre:
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
    1
    2
    3
    4
    5
    6
    7
    8
    9
    10
     
    until ( $reprise eq "fin" ) {
        oxa_msg_info( $r_log, 1,"LABEL : $reprise" );
        if (exists $r_balises_ok{$reprise) {
            $r_balises_ok{$reprise}->();
        } else {
            say "KO - Etiquette $reprise invalide";
           # ...
        }
    }
    Il est peu probable que le code ci-dessus fonctionne sans des ajustements, en particulier en ce qui concerne les variables locales et les passages de paramètres, mais ça devrait donner l'idée.

    Tu trouveras plus d'information sur les tables de distribution et sur la fonctionnalité switch dans ce tutoriel. Mais rien ne t'oblige à te lancer dans les tables de distribution si l'idée te paraît obscure ou si tu as du mal à maîtriser certaines choses, les deux autres solutions fournies sont bien plus simples.

    Voilà, j'espère que ça t'aide, mais n'hésite pas à demander si certains points ne sont pas clairs.

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

Discussions similaires

  1. Outil pour la revue de code
    Par FABFAB125 dans le forum Outils
    Réponses: 7
    Dernier message: 25/11/2007, 10h35
  2. Outils de revue de code
    Par grabriel dans le forum EDI, CMS, Outils, Scripts et API
    Réponses: 2
    Dernier message: 22/08/2007, 11h56
  3. Outils de revue de code
    Par YAMKI dans le forum Qualimétrie
    Réponses: 2
    Dernier message: 15/02/2006, 12h29
  4. [Conseil] revue de code
    Par allstar dans le forum Langage
    Réponses: 2
    Dernier message: 09/11/2005, 11h02
  5. [Revue de code] Quels outils pour de grosses applis?
    Par franckR dans le forum Choisir un environnement de développement
    Réponses: 1
    Dernier message: 21/03/2004, 10h03

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