Discussion :

[awesomeman]

Bonjour,

J'ai commencé le perl il y a peu de temps et je venais vous présenter mon programme pour avoir un avis sur la conception et la propreté du code .

lorsque je lance mon programme celui ci va lire un dossier d'image / pdf et lire aléatoirement l'un de ces fichier .
Pour lire le fichier, j'utilise des commandes shell (evince pour pdf et deepin-image-viewer pour le reste).
L'utilisation de la fonction "system" est elle risqué niveau sécurité ? si oui qu'est ce que je peux utiliser à la place ?

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
#!/usr/local/bin/perl -w
 
use strict;
use warnings;
 
sub main()
{
	my $filename = "/tmp/tmpLIST";
	my $flux = "";
	my @AllFiles;
 
	system 
"echo \$(ls -R1 /home/f-society/Documents/informatique/infographie/*) > " . $filename; # on mets tout dans le tmp;
 
	open(my $fh, '<:encoding(UTF-8)', $filename) # open my file
	    or die "Could not open file '$filename' $!";
 
	while (my $row = <$fh>) # store all file 
	{
	    chomp $row;
	    $flux .= $row;
	}
	$flux .= "   ";	# for help the parsing on the last file ;)
 
	while (my $y = substr($flux, index($flux, '/home'), index($flux, ':'))) # get all dir
	{
      	    $flux = substr($flux, index($flux, ':')+1); #remove dir ref
	    my $files = substr($flux, 0, index($flux, '/home')); # get all files in this dir
	    $flux = substr($flux, index($flux, '/home')); # remove files ref
      	    $files =~ s/^\s+//; # remove left white space
 
	    while (my $file = substr($files, 0, index($files, ' '))) # get one file 
	    {
		$files = substr($files, index($files, ' '));
		$files =~ s/^\s+//;
		my $finalfile = $y . "/" . $file; # concat final path of file 
      		push @AllFiles, $finalfile; # push it in array 
	    }
	}
 
	my $size = scalar @AllFiles; # get array size
	my $var = int(rand($size)); # gen random num
	my ($ext) = $AllFiles[$var] =~ /(\.[^.]+)$/; # get ext of file 
	print $AllFiles[$var] . "\n" . $ext . "\n"; 
	if ($ext eq ".pdf")
	{
	    system "evince " . $AllFiles[$var]; 
	}
	else
	{
	    system "deepin-image-viewer " . $AllFiles[$var];
	}
}
 
main

la commande ls -R1 ... va me renvoyer tous les chemins des fichiers dans le dossier :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
/home/f-society/Documents/informatique/infographie/asm: test.pdf test1.pdf test3.png /home/f-society/Documents/informatique/infographie/CandCPP: ldqs.jpg hello.pdf 
 //Sous dossier 

ensuite je parse pour concaténer le nom des sous dossiers avec les fichiers puis je push tous les chemins absolu dans une seule array .



J'espère que le code n'est pas trop sale

Merci pour vos avis

[disedorgue]

Bonjour,

Voici un petit exemple qui pourrait te réduire ton code:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
use strict;
use warnings;
use File::Find;
use Data::Dumper;
my @AllFiles;
find(sub{push @AllFiles,"$File::Find::name" if -f;},"/home/f-society/Documents/informatique/infographie/")
print Dumper \@AllFiles;'

Le module Data:: Dumper, n'est là que pour permettre de te montrer le contenu du tableau via le Dumper.
Pour ce qui est de la commande system, celle-ci est toujours dangereuse quelque soit le langage quand elle est mal employée.
Ici, tu l'utilises d'une manière dangereuse, car tu ne donnes pas le chemin absolu de la commande qui doit être exécutée et donc rien ne nous empêche de faire exécuter une autre commande à ton script en créant notre propre commande du même nom et en faisant pointer la variable PATH dessus.
Il faut aussi protéger les caractères des noms de fichiers pour que la commande system ne puisse pas fournir une commande à cause d'un nom de fichier trompeur. Par exemple, rien n’empêche de créer un fichier avec le nom "toto;ls -l;titi.mp4" et celui-ci pourrait faire exécuter un ls -l au shell lors de l'appelle de la fonction system.

[Lolo78]

Bonjour,

le code est assez propre, mais pourrait être plus simple.

Quelques remarques.

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
 
#!/usr/local/bin/perl -w
 
use strict;
use warnings;

C'est très bien d'utiliser les deux pragmas use strict; et use warnings;. Mais du coup le -w de la première ligne n'est pas utile et peut même être nuisible dans certains cas. use warnings; est bien plus flexible que l'option -w.

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
sub main()
{ #...
}

Ça ne gêne pas vraiment, mais il n'est pas utile de mettre tout ton code dans une fonction main.

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
 
    system 
"echo \$(ls -R1 /home/f-society/Documents/informatique/infographie/*) > " . $filename;

A éviter.

Un changement minimal serait de récupérer directement dans un tableau les données renvoyées par ls (plutôt que de rediriger vers un fichier temporaire):

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
 
my @files = `ls -R1 /home/f-society/Documents/informatique/infographie/*`;

On peut cependant éviter complètement l'appel externe au système. Perl permet possède plusieurs mécanismes permettant de lire un répertoire sans passer par une commande système externe et sans avoir besoin d'écrire dans un fichier temporaire.

Par exemple:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
 
my @files = glob ("/home/f-society/Documents/informatique/infographie/*");

Là, tu récupères directement les noms de fichiers du répertoire courant dans le tableau @files. Du coup, tu n'as pas besoin d'ouvrir le fichier temporaire, ni de faire tous ces substrpour récupérer les noms de fichier.

Une autre solution est d'utiliser les commandes opendiret readdir.

Si tu désires récupérer récursivement l'arborescence d'un répertoire, alors la meilleure solution est sans doute le module use File::Find; proposé par disedorgue.

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
 
	while (my $row = <$fh>) # store all file 
	{
	    chomp $row;
	    $flux .= $row;
	}

Ce code devient inutile avec les modifications suggérées plus haut, mais si tu dois faire ceci, alors utilise plutôt un tableau qu'une chaîne de caractères pour stocker tes noms de fichiers:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
 
	ly @files;
	while (my $row = <$fh>) # store all file 
	{
	    chomp $row;
	    push @files, $row;
	}

Il suffit ensuite de parcourir le tableau, sans voir besoin de tous ces substr.

[awesomeman]

Ok, merci pour vos retours

je vais regarder pour tout ça !

[JeanMi3000]

certains diront qu'il ne faut pas utiliser 'ls' dans un script

[awesomeman]

certains diront qu'il ne faut pas utiliser 'ls' dans un script

Oui, je sais
j'ai changé ça, plus besoin du ls salase ..

voilà ce que ca donne :

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
#!/usr/local/bin/perl                                                                                                                                                              
 
use strict;
use warnings;
use File::Find;
 
sub main()
{
    my $flux = "";
    my @AllFiles;
 
     my $start_dir = "/home/f-society/Documents/informatique/infographie/";
    find( sub { push @AllFiles, $File::Find::name unless -d; }, $start_dir);
    my $size = scalar @AllFiles; # get array size                                               
    my $var = int(rand($size)); # gen random num                                                
    my ($ext) = $AllFiles[$var] =~ /(\.[^.]+)$/; # get ext of file                              
    print $AllFiles[$var] . "\n" . $ext . "\n";
    if ($ext eq ".pdf")
    {
        system "evince " . $AllFiles[$var];
    }
    else
    {
        system "deepin-image-viewer " . $AllFiles[$var];
    }
}
 
main

Encore Merci