Students Rank calculation for the below perl script

[valavanp]

Hi Monks,
I wrote the following perl script to calculate the total marks, percentage and the pass/fail status of the students list. Here i have to calculate the rank of the students. Your suggestions to approach this will be helpful at this point of time.

student.pl
----------
#!/usr/bin/perl

use strict;
use warnings;

open(FILE, "marks.txt") || die("cannot access the file");
my($fullname,$markseachsubject,$totalmarks,$percentage,@marks,@total,$
+marks,$status,@rank,$mark,$value,%records);
while(my $line=<FILE>){    
    next if $line=~/^NAME/;
    chomp($line);
    $line=~/(\w+),(\w+),(\d+),(\d+),(\d+)/;
        $fullname=$1." ".$2;
        push(@marks,$3,$4,$5); 
        foreach $mark(@marks){
           $status=checkpass($mark);
        }
        $markseachsubject=$3." ".$4." ".$5." ".$status; 
    $totalmarks=$3+$4+$5;
        @total=$totalmarks;
        $percentage=$totalmarks/3;                          $percentag
+e=sprintf("%.2f",$percentage);
        if($status eq "Pass"){ 
           print $fullname." "."\t".$markseachsubject." "."\t".$totalm
+arks." "."\t".$percentage;
        print "\n";
        }else{
           print $fullname." "."\t".$markseachsubject." "."\t";
           print "\n"; 
        }  
        
}

#To check whether the student is pass or fail
sub checkpass{
    $marks=shift;
    if ($marks > 35){
      $status="Pass";
      return $status; 
    }else{
      $status="Fail";
      return $status;
     }
}


close FILE;

My input file is:

marks.txt
----------
NAME:SURNAME:MARK1:MARK2:MARK3
john,michael,100,50,60
sam,shane,50,60,70
tom,christo,30,40,50

Thanks all for your help

[moritz]

$line=~/(\w+),(\w+),(\d+),(\d+),(\d+)/;

And what happens when the regex doesn't match? don't use $1, $2, ... withouth checking that.

An alternative appraoch:

my ($firstname, $lastname, @marks) = split m/,/, $line;

This has the advantage that you can have an arbitrary number of marks. Of course you'll need to adjust the rest of the code a bit, for example;

$totalmarks = 0;
$totalmarks += $_ for (@marks);

        foreach $mark(@marks){
           $status=checkpass($mark);
        }

So $status is the status of the last mark only. Is that what you want?

You should test your script also with data of failing students.

BTW I strongly recommend to align the code better, one tab for each opening brace.

[Erez]

Several things:

This code compiles and has a result. It would be better if you explain why aren't the results to your liking - what is the exact assignment supposed to output.

Use the 3 arguments way for open, and if you hard code the file, use a full path. open(FILE, '<', 'c:\results\marks.txt') || die("cannot access the file - $! ");

All the variables are being declared at the start, which makes them global to the whole program. This nullify the benefits of scoped calls, and results from one student override the other. Also, the $status is always being set to the last result of the student. You also call %records, without referring to it. A better way would be to declare the variables just before you need them.

saying my @details = split (','); instead, is better than your regex matching. You can then check each of @details, remove unwanted characters and process it accordingly. Main reason is that you assume the file is set "john,michael,100,50,60" while it might be "Mr. john-bob,michael-mick ,100 ,50.5 ,60.3,74.29,...."

There's no need for the subroutine checkpass if you only check for the mark being larger than 35. place the check inside the loop and process it if $mark > 35. For instance, if you only want to calculate passed marks, say  $total += $mark if $mark > 35;
Last, no need for concatenation, Perl can do $string = "$var $var1, $var2 $var3\n".

[hipowls]

There are a few things I would do differently

• I'd put the most of the variables into the loop, restricting the scope to just the places they are used will spare you some grief later.
• You need to test that your regular expression matched, if it doesn't you end up processing the previous record again.
• If you have a student with the surname O'Hara you will fail to process her records.
• $status has the value of the last mark checked, I think you mean to check if any failed
• The use of . to join the strings is ugly, use variable interpolation instead.

Anyway this is my version, it still fails to report marks for Ms. O'Hara but at least you will get a warning, and I have to leave something for you;-) The other change is to not explicitly code the number of tests, this will work if the input file has results of four tests.

#!/net/perl/5.10.0/bin/perl

use strict;
use warnings;
use List::Util qw(sum);

my $input_file = 'marks.txt';

open my $DATA, '<', $input_file or die "Can't open $input_file: $!\n";

while ( my $line = <$DATA> ) {
    next if $line =~ /^NAME/;
    chomp $line;
    if ( $line =~ /^\w+,\w+(?:,\d+)+$/ ) {
        my ( $first_name, $last_name, @marks ) = split ',', $line;
        my $totalmarks = sum @marks;
        my $average    = sprintf "%.2f", $totalmarks / @marks;
        my $fail       = 0;
        foreach my $mark (@marks) {
            $fail ||= checkfail($mark);
        }
        if ( !$fail ) {
            print "$first_name $last_name\t@marks \t$totalmarks\t$aver
+age\n";
        }
        else {
            print "$first_name $last_name\t@marks \t\n";
        }
    }
    else {
        warn "Invalid input: $line\n";
    }
}

close $DATA or die "Can't close $input_file: $!\n";

#To check whether the student failed
sub checkfail {
    my $marks = shift;
    return $marks < 35;
}
john michael    100 50 60     210    70.00
sam shane    50 60 70     180    60.00
tom christo    30 40 50     

You may wish to give consideration to using Text::CSV_XS for parsing comma delimited text files.

Update: two small code modifications kindly suggested by moritz.

[moritz]

Just two minor remarks:

my $totalmarks = reduce { $a + $b } @marks;

If you resort to List::Util (Which isn't a bad idea at all) you can just as well use sum @marks.

my $fullname   = "";

Since you don't use it, you don't have to declare it.

[hipowls]

Damn, I can never remember what's in List::Util and what's in List::MoreUtils;-) Thanks for reminding me.

[wfsp]

You declare @marks outside of the while loop and then

push(@marks,$3,$4,$5);

inside the loop. @marks is accumulating the marks for all the students. If that is not what you want move the declaration inside the while loop.

while(my $line=<FILE>){ 
  my @marks;
  ...
}

[johngg]

If there is a chance that the header line might be repeated further down your data file, ignore this post.

If, on the other hand, your header line only occurs at the top of the data file, just read and discard the header outside the loop and get rid of the test next if $line=~/^NAME/; which you will be doing un-necessarily on every data line. (I wonder if there is anybody with a surname of 'Name'.)

my $marksFile = q{marks.txt};
open my $marksFH, q{<}, $marksFile
   or die qq{open: $marksFile: $!\n};
{
    my $header = <$marksFH>;
}

while ( my $line = <$marksFH> )
{
    chomp $line;
    ...
}

I hope thisis of interest.

Cheers,

JohnGG