Code Critique

[JBUK]

Hi All,

I have developed a help desk in perl which I have made available under the GPL - I'm constantly trying to improve it and make the code cleaner etc, and would really love some feedback if anyone can spare a little while to check out my source.

At the moment I only have a .tar file available, if its a bit of a pain I'm happy to convert some of the files to .txt and make them viewable over a browser.

I appreciate anyone's feedback greatly

Thank you

John I have the source available here http://www.perldesk.com/dl/CURRENT/1.02.4.tar.gz A demo is here http://www.perldesk.com/demo.html

[tachyon]

I have had a look at your code. This is a big project but you have made it far more complex than it needs to be. The major and completely overriding comment is that you use *global variables* extensively. This not only makes code error and bug prone but also makes it a pain in the arse to follow. I commend Use strict warnings and diagnostics or die to you.

-T You really need to read perlman:perlsec and use taint checking.

You are interpolating scalar varaibles directly into SQL instead of using placeholders ? Worse still, you don't escape or untaint these values - some of which are raw user input. For details on why this is really bad and may make you sad see This Short DBI Tutorial by MJD It is possible for a malicious user to destroy your database as it currently stands. Even a well meaning O'Deary may wreak havoc.

A next comment is that you have hand rolled an HTML parser when HTML::Template is sitting there waiting for you.

You use die extensively which is rather silly in CGI programs. A die_nice() sub is usually a good idea. In this sub you print a cover all BS message to the user so they don't get a 500 internal server error. You can also email yourself, log the date/time/error etc. A typical structure might be:

open FILE, $file or die_nice("Can't open $file: $!");

sub die_nice {
    $real_problem = shift;
    $user_fob_off = "Sorry, our server can not respond to your request
+ due to ";
    $user_fob_off .= "routine maintenance ;-) Please try again later."
+;
    tell_user( Title=>"Sorry!", Content=>$user_fob_off, Link=>$home );
    email( To=>"admin", From=>"die_nice()", Subject=>"die_nice() error
+", Content=>$real_problem );
  exit;
}

sub email {
    my %mail = @_;
    *MAIL = *STDOUT; # open mailprog simulation
    my $message = unindent("
        To:      $mail{'To'}
        From:    $mail{'From'}
        Subject: $mail{'Subject'}
    
        $mail{'Content'}
    ");
    print MAIL $message;
}

# remove leading whitespace indentation to allow 
# nice code formatting with strings indented inline
sub unindent {
    $data = shift;
    $data =~ s/^\s+//;
    $data =~ s/^ +//gm;
  return $data;
}

sub tell_user{}

You might like to investigate the use of here pages, pod and modules

You have far too many scripts that are to all intents one or two subroutines long. This makes following your code tedious. It is better IMHO to bundle related functions into a module or 10 and then call these from your main script(s). Logically (to me) the main script just calls one of either the user/staff/admin interface modues. These then do their things by calling other modules such as say the Password module that does all the password validation/modification or the Job module that adds/modifies/deletes jobs.

cheers

tachyon

s&&rsenoyhcatreve&&&s&n.+t&"$'$`$\"$\&"&ee&&y&srve&&d&&print

[stefan k]

You should:
• make your links real links
• note the license somewhere on your website.
• not demand that downloaders must give name and email adress; this will prevent many people (like me) to actually download your programm and you need that to get the community involved
• be aware that one probably can't have a quick look at the source of a project the size of a helpdesk

Regards...	Stefan
you begin bashing the string with a +42 regexp of confusion

[JBUK]

Hi Stefan,

I put a form there just out of interest of who's using the program, its nice to know how many people etc - but i'll remove it if people are not d/l because of it.

I will note the license on the site, I forgot to do that, thanks for pointing it out

The SOURCE file is here
The demo is here

[IlyaM]

If you want some feedback just make that form optional. Many people will give you feedback if you ask them nicely instead of forcing them to do it.

--
Ilya Martynov (http://martynov.org/)

[gmax]

To be fair, I could download the source code without leaving any e-mail address.
And, indeed, every script says that it is under GPL.

However, one thing that is definitely missing is documentation. By looking at the code itself you can't get the feeling of how things are organized; and the demo can't tell you what's under the hood.
So, I would add some (a lot!) of POD to those scripts.

Finally, one last note. No time to have a look at your code, but, just a shot in the dark. Running:

grep -r "use strict" *

from the root of your snapshot, I get an empty list, meaning that none of your scripts are using it.
The same command inquiring for "-w" and "use CGI" returns a significant list.
gmax

[IOrdy]

Why dont people embrace the concept of templates? I'm the first to admit my perl knowledge is still very limited but one of the first modules I ever used was HTML::Template. Seperating your code/content/style just plain makes sense. (i.e what css is to html).

Apart from the use strict comment (I got use strict and -w bitch slapped into me when I first started sratching out code in perl) I think that templates should be the next thing you implement.

That's after any coding changes the monks suggest though.


Forgive my english, I'm Australian.

[belg4mit]

For me personaly, I've looked at them and while HTML::Template is kind if cool it seems that much of the time it just wouldn't be powerful enough for some of what I want to do. And why use templates when you have to output/construct half the HTML yourself ;-)? >Forgive my english, I'm Australian.
I suppose I can let it slide this time ;-)

--
perl -p -e "s/(?:\w);([st])/'\$1/mg"

[jeffa]

You still miss the bigger picture, it's not about making the generation of HTML from Perl code easier or abstract - it's about moving the HTML out of the Perl code. Trust me, this is a good thing. Also, understand that I didn't get it at first either - the whole 'seperation of presentation from logic' seemed like a complete waste of time. Remember, my first job out of school was developing in Cold Fusion. I literally had to take over the job of the UI designer, because they were too afraid to touch the site once all of their HTML had been moved into the Cold Fusion code. (you have read my tut on HTML::Template, right? ;))

I have heard you mention in the CB, why use a templating system when CSS is available. Because CSS isn't powerful enough, it only defines markup. Templating systems allow you to package up all dynamic content for a page in a convenient (well, convenient for a Perl programmer) data structure and apply it to HTML. Very similar to CSS, but instead it offers the power to control large chunks of presentation - only display this for users logged in, only display that for anonymous users. How do you think this site does what it does? ;)

You mentioned HTML::Template wouldn't be powerful enough for you most of the time. I can't agree with that, but i can agree that a templating system might be 'over-kill' most of the time. If you are working with a team of developers and a team of UI designers, then you will see the power of templating systems.

I am not saying that you should rush out and port all of your existing sites to a templating system, i am just saying please don't knock it till you have fully experienced it.

jeffa

L-LL-L--L-LL-L--L-LL-L--
-R--R-RR-R--R-RR-R--R-RR
F--F--F--F--F--F--F--F--
(the triplet paradiddle)

[vladb]

I agree... discounting a few things that HTML::Template may be useful for, I always found it hard to actually 'wrap' my code around a template. I know that if separating code from content is an issue, then wouldn't it be simpler to move on to Mason? However, I'm not particular fan of this either since many Mason scripts I looked at appeared to be quite a mess.

Having said that, I am actually trying to move more towards splitting display logic from program logic. One way to do this, I think, would be to have your program generate and store all of its 'working' data mean to be displayed in some fashion in a hash or some complex enough structure which yet could be looped through/accessed via a templating script (such as <TMPL_LOOP ... > </TMPL_LOOP> implemented in HTML::Template).

Since I'm yet not a strong believer in Perl templating utilities, would anyone with some related experience share in his/her joys of using them? I'd be interested in hearing from someone who was in a position similar to I'm in now (that is 'hardly decided') and how they managed to integrate this concept in their code.

PS: my native language is Russian so don't mind my English! ;-)



"There is no system but GNU, and Linux is one of its kernels." -- Confession of Faith

[tachyon]

Seems fairly functional. One small aesthetic note - some of your banners have an outline <table border="1"... and others do not.

Unless you want anyone to be able to have fairly free and full access to your site you should make your web dirs non-world readable. Have a look http://www.perldesk.com/dl/. If this is what you want the world to see then feel free to ignore....

cheers

tachyon

s&&rsenoyhcatreve&&&s&n.+t&"$'$`$\"$\&"&ee&&y&srve&&d&&print

[JBUK]

Hi Tachyon Thanks for the reply, I have the dirs world readable so people can go in an greab the release they want, the most recent will be in the current directory though Thanks John