Frequency analysis for simultaneous dice rolls

Question

My professor told me to optimize my perl code, but he wouldn't tell me what exactly - just make it simpler.

I changed some little things but nothing major. Am I missing something?

The task was:

Perl program that allows the user to simulate the simultaneous throwing of several cubes. The simulation result is then a total number of points, which is the sum of the numbers of the eyes results in single throws. If many of these throws are performed with several cubes, the frequency distribution for the total number of eyes is calculated. This frequency distribution should be in the console window in the form of a table and as a simple overview chart.

At the beginning the following entries should be requested by the user:

• the number of dice he wants to roll.
• the number of throws he wants to perform with the selected number of dice.

The frequency distribution for the total number of eye sets should be displayed in two different ways:

• Tabular representation as a comparison of the number of points obtained and the associated frequency, both the absolute values ​​of the frequency and the normalized values ​​that result in the division of the absolute values ​​resulting from the total number of throws are issued.
• Graphic representation as a simple (tilted by 90 degrees) bar chart (e.g. corresponding number of *)

Use subroutines.

use warnings;                   
use diagnostics;                
use GD::Graph::hbars;        
use GD;
use List::Util qw(max);
my $number_of_dice = 0;   
my $number_of_rolls  = 0;     
my $sum_of_throw = 0;     
my @total_sum_of_eyes;       
my @single_sum_of_eyes;   
my @frequency;            
my %count;                   
my $count =0;                 
my $number =0;                

print "\n Please insert the number_of_dice: ";   
chomp($number_of_dice=<STDIN>);
print "\n Please instert the number_of_rolls: ";
chomp($number_of_rolls=<STDIN>);


while ($count< $number_of_rolls) {                  
    roll($number_of_dice);                      
    push(@total_sum_of_eyes,$sum_of_throw);     
    $count++;                                           
}  
frequency();                        
representation ();


sub roll{ 
    $sum_of_throw = 0;
    for($i = 0; $i< $number_of_dice; $i++)      
    {
        $number = int(rand(6) +1);                  
        $sum_of_throw+=$number;                     
    }
}

sub frequency{
    foreach my $eyes(sort{$a <=> $b} @total_sum_of_eyes){                       
        $count{$eyes}++;
    } 
}
sub representation{
    #Tabular representation
    print "\n the frequencys-distribution is: \n";
    print "\n total_sum_of_eyes\t frequency\t frequencys-distribution in %\n";
    foreach my $eyes (sort{$a <=> $b} keys %count){         
        push(@frequency, $count{$eyes});                #data for the bar chart     
        push(@single_sum_of_eyes, $eyes);               #data for the bar chart         
        printf "\n\t $eyes\t\t\t  $count{$eyes}\t\t%g",($count{$eyes}/$number_of_rolls)*100;
    }
    print "\n";
    #bar chart
    my $graph = GD::Graph::hbars->new(1600, 600);       
$graph->set(                                            
    x_label             => 'total_sum_of_eyes',         
    y_label             => 'frequency',             
    title               => 'frequencys-distribution',   
    y_max_value         =>  max(@frequency)+1,          
    y_min_value         =>  0,                          
    y_tick_number       => 8,                           
    transparent         => 0,                           
    bgclr               => 'white',                     
    long_ticks          => 1,                           
) or die $graph->error;                                 
my @data = (\@single_sum_of_eyes,\@frequency);      
$graph->set( dclrs => [ qw(green) ] );   
my $gd = $graph->plot(\@data) or die $graph->error;  
open(IMG, '>gd_bars.gif') or die $!;
binmode IMG;
print IMG $gd->gif;
}

Answer 1

The code can be simplified.

It would be better to move some of the variables from the main scope into the subroutines. For example, the roll sub can take an input value and return a value. Then, the return value can be used directly as a key to the %count hash.

If you change the while to a for loop, that eliminates the need for the $count variable. This, combined with the change to roll, eliminates the need for the frequency subroutine and the sort therein.

You should use strict instead of diagnostics. The latter should only be used for temporary debugging.

Indentation

The representation subroutine is missing indentation for the bar chart code.

Output formatting

I think %f is more apt than %g in this context. Also, there is no need for as many digits of precision; 2 should suffice after the decimal point (%.2f).

Typos

instert should be insert.

frequencys is not a word. I would just use frequency in the output.

Style

for is simpler than foreach.

Add space between control keywords (if, while) and the opening paren (.

Add space around operators, and 1 space after commas.

Use the range operator (..) instead of the C-style for loops.

There is no need for parentheses with your push builtins.

Trailing whitespace is not needed and can be annoying. Just remove it.

Use exit to separate the main code from the subroutines.

Input checking

Consider adding code to check the inputs to make sure they are numbers (instead of words, for example) and that they are in reasonable ranges. Refer to looks_like_number.

Documentation

Consider adding some usage notes at the top of the file as plain old documentation (POD), something as simple as:

=pod

Roll the dice...

=cut

---

Here is new code with some of the suggestions above:

use strict;
use warnings;
use List::Util qw(max);
use GD::Graph::hbars;
use GD;

my $number_of_dice;
my $number_of_rolls;
my %count;

print "\n Please insert the number_of_dice: ";
chomp($number_of_dice = <STDIN>);
print "\n Please insert the number_of_rolls: ";
chomp($number_of_rolls = <STDIN>);

for (1 .. $number_of_rolls) {
    $count{ roll($number_of_dice) }++;
}

representation();
exit;

sub roll {
    my $number_of_dice = shift;
    my $sum_of_throw = 0;
    for (1 .. $number_of_dice) {
        $sum_of_throw += int(rand(6) + 1);
    }
    return $sum_of_throw;
}

sub representation {
    my @frequency;
    my @single_sum_of_eyes;

    # Tabular representation
    print "\n the frequency-distribution is: \n";
    print "\n total_sum_of_eyes\t frequency\t frequency-distribution in %\n";
    for my $eyes (sort {$a <=> $b} keys %count) {
        push @frequency, $count{$eyes};     # data for the bar chart
        push @single_sum_of_eyes, $eyes;    # data for the bar chart
        printf "\n\t $eyes\t\t\t  $count{$eyes}\t\t%.2f", ($count{$eyes}/$number_of_rolls)*100;
    }
    print "\n";

    # Bar chart
    my $graph = GD::Graph::hbars->new(1600, 600);
    $graph->set(
        x_label             => 'total_sum_of_eyes',
        y_label             => 'frequency',
        title               => 'frequencys-distribution',
        y_max_value         =>  max(@frequency)+1,
        y_min_value         =>  0,
        y_tick_number       => 8,
        transparent         => 0,
        bgclr               => 'white',
        long_ticks          => 1,
    ) or die $graph->error;
    my @data = (\@single_sum_of_eyes,\@frequency);
    $graph->set( dclrs => [ qw(green) ] );
    my $gd = $graph->plot(\@data) or die $graph->error;
    open(IMG, '>gd_bars.gif') or die $!;
    binmode IMG;
    print IMG $gd->gif;
}