The reason why graph() works is that most of the variables in that script are being treated as globals, which is a bad practice as it makes it very difficult to debug.

Everything but $minEnergy, $maxEnergy, $xYen,and $yLen are used globally within graph(). Some of these are forgiveable, as they are being treated as constants.. but anything that could be modified should be passed to the graph() sub. This includes the $im object.

I would strongly advise enabling strict and warnings. It will tell you all of the places where these variables are being used inappropriately.

It would also be good to split some of the behaviour into worker functions that encapsulate one set of behaviour.

Thanks for your input monk. I will try to implement and then get back to you :)

The edited version of the code contains ## comments where I made changes or inserted comments.

Note in particular that the array @Y2 was renamed to @y2! That (presumed) typo was picked up by use strict;.

I don't see any particular issue with the while loop. Perhaps you could describe the problem that you are having with it?

Update bugs fixed - see replies

Dear GrandFather,

First of all, thanks for taking so much pain for my problem.

In the code I posted, mistakenly graph()(line 126) left out while pasting.

The code I have pasted is working and giving results. But without 'use warning' I don feel confident. And also global variable of this code are interfering with rest of my code(actually this piece is part of a large work).

For whileloop case, I didn't get, what is prompting it to run over complete array. like it is not written as while (@x) or while ($fh)
So what I think is, this should run only once when it enters in the sub; when for the first time control enters the loop while($incEnergy > $minEnergy)What is prompting it to return? I know its something v silly that I am not getting, but pl guide me.

Can you pl explain me how this part of your code is working!! Just briefly, what is the concept behind or some link.. Also while compiling its showing this line

Global symbol "$x1" requires explicit package name at monk_graph.pl line 150.

if(@$x1) {
            $$max = $y->[-1] if $y->[-1] > $max;
            $$min = $y->[-1] if $y->[-1] < $min;
        } else {
            $$min = $$max = $y->[-1];}
[download]

while ($incEnergy > $minEnergy)

In that while loop there is:

$incEnergy = $incEnergy+$scale;
[download]

So presumably, incEnergy will grow until it is larger than minEnergy(update: incEnergy will shrink until its less than minEnergy), and the while loop will terminate eventually if $scale is greaterless than zero. But you have:

$scale = ($minEnergy-$maxEnergy)/5;
[download]

Which is greater? min or max? Normally I'd think the max would be greater, so scale would be negative, and you may have an infinite loop. If the code is correct, then it is confusing. Nevermind. But at least it answers your question on how the loop terminates.

Global symbol "$x1" requires explicit package name at monk_graph.pl line 150.

I assume that is supposed to be "@x", not "@$x1" in the code.

The while loop decrements $incEnergy from $maxEnergy downward in steps of $scale (which is negative) until $incEnergy is less than $minEnergy. It looks fine to me, although it may be a good place for a C type for loop:

for ($incEnergy = $maxEnergy; $incEnergy > $minEnergy; $incEnergy += $
+scale)
[download]

and omit the $incEnergy = $incEnergy+$scale; at the end of the loop. It looks like the while loop is generating points along an energy axis.

if(@$x1) { should be if(@$x) {. The test checks to see if @$x contains any elements.

The LoadFile sub takes a number of reference parameters. The first line in the sub:

my ($filename, $x, $y, $min, $max) = @_;
[download]

declares the variables and sets them to the parameter values. The last four variables are references. So $$max is the scalar variable that $max refers to. $x and $y are references to arrays and $y->[-1] is the last element of the array refered to by $y. So the line:

$$max = $y->[-1] if $y->[-1] > $$max;
[download]

means assign the last element of @$Y (the array that $Y refers to) to the scalar that $max refers to if the element is greater than the current maximum. (Note there was an error in this code. See update in previous post.)

---

DWIM is Perl's answer to Gödel

So, can any monk would suggest me any better way to write this code that is more robust, with use strict'.

Update: fixed an edge case in sub read_file.

#!/usr/bin/perl
use warnings;
use strict;

use GD;


my ( $min1, $max1, $x1, $y1, $lines ) = read_file( 'fwd.stb' );

# Define constants
my $X1start = 50 + 20;               # X-axis
my $Y1start = 300;                   # X-axis
my $X1end   = 450 + $lines - 400;    # X-axis
my $Y1end   = 300;                   # X-axis
my $X2start = 50 + 20;
my $Y2start = 100;
my $X2end   = 50 + 20;
my $Y2end   = 300;

# create a new image
my $im = new GD::Image( $X1end + 50, $Y2end + 100 );

# allocate some colors
my $white = $im->colorAllocate( 255, 255, 255 );
my $green = $im->colorAllocate( 0,   255, 0 );
my $black = $im->colorAllocate( 0,   0,   0 );
my $red   = $im->colorAllocate( 255, 0,   0 );
my $blue  = $im->colorAllocate( 0,   0,   255 );


my $half_lines = int( $lines / 2 );
# set the caption for the graph
$im->string( gdLargeFont, $half_lines - 50, 20, 'ENERGY GRAPH', $black
+ );

# draw the y-axis & x-axis
$im->line( $X2start, $Y2start, $X2end, $Y2end, $black );
$im->line( $X1start, $Y1start, $X1end, $Y1end, $black );

# set the caption for x-axis and y-axis
$im->string(   gdLargeFont, $half_lines - 50, 335, 'NUCLEOTIDE POSITIO
+N', $black );
$im->stringUp( gdLargeFont, 5, 260, 'FREE ENERGY(delta G)', $black );

$im->string( gdSmallFont, $half_lines + 100, 20, ' Genomic  Sequence -
+------', $red );
$im->string( gdSmallFont, $half_lines + 100, 40, ' Shuffled Sequence -
+------', $blue );


my ( $min2, $max2, $x2, $y2 ) = read_file( 'total_shuffle.stb' );


my $min = $min1 <= $min2 ? $min1 : $min2;
my $max = $max1 >= $max2 ? $max1 : $max2;

graph( $x1, $y1, $min, $max );


open my $DISPLAY, '|-:raw', 'display', '-' or die "Cannot open pipe to
+ 'display' $!";
print $DISPLAY $im->png;
close $DISPLAY or warn $! ? "Error closing 'display' pipe: $!" : "Exit
+ status $? from 'display'";

exit 0;


sub read_file {
    my $file_name = shift;

    open my $fh, '<', $file_name or die "Cannot open '$file_name' $!";

    my ( $min, $max, @x, @y );
    while ( <$fh> ) {
        my @value = split;
        push @x, $value[ 0 ] - 7;
        push @y, $value[ 1 ];

        if ( @y > 1 ) {
            $max = $y[ -1 ] if $max < $y[ -1 ];
            $min = $y[ -1 ] if $min > $y[ -1 ];
            }
        elsif ( @y == 1 ) {
            $min = $max = $y[ -1 ];
            }
        }

    return $min, $max, \@x, \@y, $.;
    }

sub graph {
    my ( $x_ref, $y_ref, $minEnergy, $maxEnergy ) = @_;

    # Scaling the values
    my $Yscale = ( $Y2end - $Y2start ) / ( abs( $minEnergy ) - abs( $m
+axEnergy ) );

    my $scale = ( $minEnergy - $maxEnergy ) / 5;

    my $incEnergy = $maxEnergy;
    while ( $incEnergy > $minEnergy ) {   ##### Prob loop####

        my $x1 = $X1start - 2;
        my $y1 = int( ( ( abs( $incEnergy ) - abs( $maxEnergy ) ) * $Y
+scale ) + $Y2start );
        my $x2 = $X1start + 2;

        $im->line( $x1, $y1, $x2, $y1, $black );
        my $displayEnergy = sprintf '%2.1f', $incEnergy;
        $im->string( gdSmallFont, $x1 - 35, $y1 - 10, $displayEnergy, 
+$black );
        $incEnergy = $incEnergy + $scale;
        }

    # mark 10 points on the position axis
    my $posSkip = int( @$x_ref / 10 );
    # If the length of the fragment is lesser than the x-axis

    my $skipPixel = ( $X1end - $X1start ) / @$x_ref;

    my $count;
    for my $i ( 0 .. $#$x_ref - 1 ) {
        my $x1 = int( $i * $skipPixel + $X1start );
        my $y1 = int( ( abs( $y_ref->[ $i ] ) - abs( $maxEnergy ) ) * 
+$Yscale + $Y2start );
        my $x2 = int( ( $i + 1 ) * $skipPixel + $X1start );
        my $y2 = int( ( abs( $y_ref->[ $i + 1 ] ) - abs( $maxEnergy ) 
+) * $Yscale + $Y2start );

        $im->line( $x1, $y1, $x2, $y2, $red );
        $count++;

        if ( $count == $posSkip ) {
            $x1 = int( $i * $skipPixel + $X1start );
            $y1 = $Y1end - 2;
            $x2 = int( $i * $skipPixel + $X1start );
            $y2 = $Y1end + 2;

            $im->line( $x1, $y1, $x2, $y2, $black );
            $im->string( gdSmallFont, $x1, $y1 + 10, $i + 1, $black );
            $count = 0;
            }
        }
    }

__END__
[download]

Hi jwkrahn!!

Thanks a lot for the guidance. Actually after trying my best for a perticular problem, if I get to see code by monk I can really improve my style of writing and also doing things efficiently. To tell you the truth after joing this site I started adding use strict or let say error msgs to my scripts. And now I clearly understands the value of these imp things which freshers like me usually ignore.

Thanks again :)

   my $max1;
   my $max2;
   my $min1;
   my $min2;
   my $i;
[download]

my ($max1, $max2, $min1, $min2, $i) = (0,0,0,0,0, );
[download]

    open(FILE1,"fwd.stb"); should be:     open(FILE1,"fwd.stb") or die "Can\'t open file\n";

The same errors are repeated later in the code because the same code is repeated. Use subroutines to replace repeated code. At least you won't have to fix every error twice.

   for($i=0;$i<$len-1;$i++)This will skip the last element of the array. The conditional should be <= or a more perlish: for(0 .. scalar @x -1)

You have two variables $call and $count that are neither declared nor initialized. Strict has been trying to tell you that also.