Menu
▾
▴
Re: [Nms-cgi-devel] search/nms
|
From: Joseph R. <rya...@os...> - 2002-02-22 23:03:51
|
----- Original Message -----
From: "Alex BATKO" <ab...@mi...>
To: <nms...@li...>
Sent: Friday, February 22, 2002 9:59 AM
Subject: [Nms-cgi-devel] search/nms
>
> This is a general statement of concern about the state of nms project, and
> more specifically about search/* (because that's all i've looked at in any
> detail).
>
> I've been following this list since the end of January and I only started
> to look at the source code yesterday. I would like to present the
> following comments as I feel they might be useful.
> 02) The search.pl program doesn't work. I downloaded search from MSA and
> it worked without ANY modifications. I can't say this for the nms
> version - infact i'm still trying to get it to work.
>
Well, it works different than what we would like. I personally think the
search itself should be recursive, which is a huge step up from Matt's
design. I think the only reason why Matt uses the "*/.html, */*/.html"
method is that he didn't know how to make it recursive. Perhaps we should
remove this behavior entirely and make the search recursive...
>
> 03) The program is poorly designed. Just to give a simple example of poor
> design, i'll cite one major problem: In search.pl, one of the first
> functional things that happens is a call to the subroutine
> start_of_html. That subroutine pumps out the header, and begins the
> html to the point where the search results are ready to be displayed.
> When 'die' is called, a message is sent to the subroutine
> fatalsToBrowser, which begins by reprinting the header manually (not
> using 'print header'), and it prints out a new html page. So the
> result is a web page within a half started webpage. Nasty !!!
>
Good point, I don't think anyone thought about that. Should be a simple
fix...
>
> 05) Poor programming technique: multiple return points in one subroutine.
> A subroutine should be written in such a way that there is only one
> return statement (and it is the last statement of the subroutine).
> In this respect the do_search subroutine is a really great example
> of this poor technique.
>
You must not understand how File::Find works. File::Find's find function is
recursive, and if it returns undef (which is what a return in void context
does), it is equivalant to saying "Move on to the next file". It would be
VERY painful to code without using this technique. At any rate, I hardly
think that multiple returns are a bad construct - Subroutines should be
constructed so that they flow a logical way - and if the damn thing should
stop at some point, it should stop, and not dance around so that there is
only 1 return statement (Not to mention that CPU time is saved by not
running unnecessary code).
>
> 06) Poor commenting: When the code is publicly available (and one of your
> intentions is for people to actually LOOK at your code), it should be
> well commented. Each subroutine should have a description of its
> purpose, what the arguments are, and what the return value is.
>
> Major chunks of code within a subroutine should also have a comment
> outlining what will be done (unless it is extremely obvious).
>
> The idea is that one shouldn't have to run through a given program in
> his/her head, just to understand what is the purpose of a particular
> chunk of code (let alone what is the purpose of a particular
subroutine).
>
True, commenting is something we do need to work on. However, since the nms
scripts are still under development, this isn't the most urgent thing that
needs to be done. Perhaps someone with some spare time could do some
commenting?
>
> 07) No set/accepted coding style (rules), leads to INCONSISTENT code.
>
> This topic often brings forth "holy wars", but it must be surfaced at
> one point or another (when multiple authors are involved). nms has no
> such guidelines. If you're running linux, and you have the kernel
> source installed, take a look at /usr/src/Documentation/CodingStyle
> for an example of what i'm talking about.
>
> In the nms code, some people organize code by 3 spaces, some by 4,
some
> by tabs, etc. The suggested spacing for Perl is 4 spaces, and in all
> public code NEVER USE TABS (because different programs interpret TABS
> differently).
>
This has come up before; Although I think it is ridiculous to expect people
to write code the same way, the least that could be done is run code under
tidy to ensure consistant formatting.
> Other inconsistencies are things like
>
> if($variable) {
> if( $variable ) {
> if ( $variable ) {
>
Does so minute as spacing really matter?
>
> 09) search/README's format is ugly, and it's badly written. I couldn't
> understand what the heck $baseurl was. Laugh if you want, but the
> point is that the default value in search.pl isn't much of a url,
> so this lead to my confusion. I had to look at MSA search/README
> where it's explained properly (with an example).
>
I agree, the readmes do need work, but they were written rather hastily. At
first, we used Matt Wright's readmes - until Dave started recieving legal
threats from him. So, we had to remove the MW readmes and replace them with
our own.
>
>
> 11) Noone has taken the time to respond to the project bug reports
> http://sourceforge.net/tracker/?group_id=39625&atid=425769
> By respond, i not only mean look into the problems, but also to
> reply to the people who have taken the time to make a bug submission.
> (the one exception that i thought was notable was that the most recent
> version of search.pl addresses one of the bugs, but noone closed the
> bug tricket).
>
If it is any consolation, the bugs have been fixed within hours of the bug
reports... :(
>
> 12) search.html refers to /css/nms.css. That's CRAZY !!! The first
> error I ran into is "File Not Found": "The requested URL
> /css/nms.css was not found on this server". The project has been
> around for more then a month, and noone has tested this ????
>
If you have been following the rest of the mailing list, there already is
discussion about stylesheets.
>
> I hope that we can have a genuine discussion about these issues,
> and come up with a methodical game plan for search/*, and the other
> scripts. It is not wise to start patching minature problems, when
> there is a major underlying flaw (See 03, above).
>
> I can write a coding standard proposal, and we can all talk about whether
> it's acceptable and complete.
>
> I hope that this motivates discussion and improvements within nms.
>
> _______________________________________________
> Nms-cgi-devel mailing list
> Nms...@li...
> https://lists.sourceforge.net/lists/listinfo/nms-cgi-devel
>
|
