Loading ...
Sorry, an error occurred while loading the content.

FW: TWiki ( was RE: [XP] is the wiki open source and if so, where do I get it?)

Expand Messages
  • Michael Finney
    I do not know if you care or not about people discussing TWiki, but extremeprogramming@yahoogroups.com is a large group of intelligent people and many may
    Message 1 of 1 , Jul 30, 2001
    • 0 Attachment
      I do not know if you care or not about people discussing TWiki, but
      extremeprogramming@yahoogroups.com is a large group of intelligent people
      and many may benefit from the tool. Also, I like TWiki and recommend it for
      company intranets with the proper security applied (not sure what that would
      be right now).

      Personally, I do not care what the program's code looks like. From an admin
      point of view and user point of view can you add to the discussion at
      extremeprogramming@yahoogroups.com ? There were some concerns about
      security (world writable instructions) and speed (although I have not had a
      problem).

      Thank you for the tool and your time,
      Michael Finney
      Sun Certified Programmer for the Java 2 Platform
      Sun Certified Developer for the Java 2 Platform



      -----Original Message-----
      From: Tony Bowden [mailto:tony-xpire@...]
      Sent: Monday, July 30, 2001 1:08 PM
      To: extremeprogramming@yahoogroups.com
      Subject: Re: TWiki ( was RE: [XP] is the wiki open source and if so,
      where do I get it?)


      On Mon, Jul 30, 2001 at 04:27:24PM -0000, kentbeck@... wrote:
      > --- In extremeprogramming@y..., Tony Bowden <tony-xpire@b...> wrote:
      > > On Fri, Jul 27, 2001 at 02:49:08PM -0600, Michael Finney wrote:
      > > > Many people and I use TWiki for company internal communications and do
      not
      > > > concern ourselves with how the code looks. It has all we need.

      > > I hope you're not interested in speed, reliability or security then. And
      > > trying to add these is non-trivial due to how bad the code is...

      > One of my joys is that we can discuss programs here. Tony, would you
      > be so kind as to be specific enough in your criticism that we can
      > begin a discussion?

      I tried to install this on my server, which is Apache/mod_perl.

      [I'll skip for now past the "make these directories world writable by
      anyone at all" instructions]

      Under 'normal' Perl CGI, your scripts and modules have no lifespan and are
      totally stateless. However, with mod_perl, Apache caches your pre-compiled
      scripts and modules and gives them a much longer lifespan. As much of the
      'cost' of Perl is the compilation phase, this often brings a speed increase
      of an order of magnitude or more.

      However, with Twiki, nothing works under mod_perl as the code is filled
      with global variables, often being pulled from other packages directly
      into someone else's namespace, which don't get cleaned up. (In fact there
      are even comments that tell you to make all your variables global, rather
      than keeping them to the smallest possible scope).

      This makes refactoring a complete pain, as you can never be sure where
      else is using a variable. Of course, the HTML is never separated from
      the processing logic at all, and is hard-coded as text strings, rather
      than using the any of the facilities available in the standard packages.

      It never checks system calls for failure, even ones that are executing
      variables!

      There's lots of repetition of code, mainly becnuse it's written
      procedurally, rather than in an OO-manner. But even then, one of the
      modules just exports a 400 line subroutine!

      Everything is really difficult to read, especially as much of it is very
      un-Perlish, and was obviously written by a C-programmer: every script
      has a main() function, which the script calls, and there are lots of
      overly verbose constructs which don't advantage of more native Perl ways
      of doing things, e.g.:

      if( ! $theWebName ) {
      $theWebName = $wiki::webName;
      }

      instead of
      $theWebName ||= $wiki::webName;

      and

      for( $x = 0; $x < @lines; $x++ ) {
      $tmp = $lines[$x];
      # ... do stuff with $tmp
      }

      instead of

      foreach my $tmp (@lines) {
      # do stuff
      }

      and

      my $statTopViews = "";
      for( $x = 0; $x < @topViews; $x++ )
      {
      $statTopViews .= "$topViews[$x] <br>";
      }

      instead of

      my $statTopViews = join " <br>", @topviews;

      etc.

      This leads to really convulsed logic, and almost inpenetrable code.
      Let's take an example from the statistic package:

      sub getTopList
      {
      my( $doUser, $theMaxNum, @theList ) = @_;

      my %hash = ();
      my $tmp;
      foreach( @theList ) {
      $tmp = $_;
      if( $doUser ) {
      $tmp =~ s/^\|[^\|]*\|\s([^\.]*\.\S*).*/$1/go;
      } else {
      $tmp =~ s/^\|[^\|]*\|[^\|]*\|[^\|]*\|\s([^\.]*\.\S*).*/$1/go;
      }
      if( $hash{ $tmp } ) {
      $hash{ $tmp } = $hash{ $tmp } + 1;
      } else {
      %hash = ( %hash, $tmp, 1 );
      }
      }

      my @list = ();
      while( ( $key, $value ) = each( %hash ) ) {
      $tmp = " $value";
      $tmp =~ s/\s*(.{5})$/$1/go;
      $list[@list] = "$tmp $key";
      }
      @list = reverse( sort( @list ) );

      my @returnList = ();
      my $idx = 0;
      for( $x = 0; $x < @list; $x++ )
      {
      if( $x >= $theMaxNum ) {
      return @returnList;
      }
      $tmp = $list[$x];
      $tmp =~ s/^\s*(.*)/ $1/go;
      $returnList[$x] = $tmp;
      }

      return @returnList;
      }

      I've been writing in Perl as my primary language for over 8 years,
      professionally for 6, and I have no idea at a glance what this is
      doing. So, let's tidy up bits of it:

      I'll skip the first regular expressions, as I can't see clearly what
      they do yet, and go to the:

      if( $hash{ $tmp } ) {
      $hash{ $tmp } = $hash{ $tmp } + 1;
      } else {
      %hash = ( %hash, $tmp, 1 );
      }

      This is really surreal. I've never, ever, seen anyone extend a hash before
      by treating it as an array and adding a new key / value pair. However, this
      entire thing is unnecessary, as hashes autovivify, so we can just replace
      these 5 lines with:
      ++$hash{$tmp};

      This hash, which thus consists a count of occurrences of each of its keys,
      is processed by:
      my @list = ();
      while( ( $key, $value ) = each( %hash ) ) {
      $tmp = " $value";
      $tmp =~ s/\s*(.{5})$/$1/go;
      $list[@list] = "$tmp $key";
      }
      @list = reverse( sort( @list ) );

      Again, the array is constructed really weirdly, adding a value to the
      end each time by calculating how many elements are in the array and
      adding the new value to that position (treating @list in scalar context
      as the index of the array, and relying on the fact that arrays number
      from zero!), instead of just doing a:
      push @list, "$tmp $key";

      The substitution is also quite weird. We take the value (which is an
      integer, remember), add five spaces to the start of it, and then take
      the last 5 characters, and create an entry in the array which is this
      string of 5 characters, followed by the key (the original string).
      This is apparently just so we can sort the list!

      Instead, we'll be able to sort the hash directly, but let's see first
      what we're going to do with it:

      my @returnList = ();
      my $idx = 0;
      for( $x = 0; $x < @list; $x++ )
      {
      if( $x >= $theMaxNum ) {
      return @returnList;
      }
      $tmp = $list[$x];
      $tmp =~ s/^\s*(.*)/ $1/go;
      $returnList[$x] = $tmp;
      }

      Aha, we're just going to create a returnable version. (Note that we get
      rid of those leading spaces we put there earlier just to pad the string
      for sorting).

      Let's just combine these 2 sections into one now. We have a hash
      (%hash: string => count), and we want the top $theMaxNum in an array
      (further checking of the source would tell us whether we *really* want
      a list back, or whether a hash would do, but for now we'll stick to the
      existing interface):

      my @returnList;
      foreach (sort {$hash{$b} <=> $hash{$a}} keys %hash) {
      push @returnList, (join " ", ($hash{$_} => $_));
      last unless --$theMaxNum;
      }

      Now we're getting a lot clearer, (and can even run under 'strict':)

      sub getTopList
      {
      my( $doUser, $theMaxNum, @theList ) = @_;

      my %hash = ();
      foreach( @theList ) {
      my $tmp = $_;
      if( $doUser ) {
      $tmp =~ s/^\|[^\|]*\|\s([^\.]*\.\S*).*/$1/go;
      } else {
      $tmp =~ s/^\|[^\|]*\|[^\|]*\|[^\|]*\|\s([^\.]*\.\S*).*/$1/go;
      }
      ++$hash{$tmp};
      }

      my @returnList;
      foreach (sort {$hash{$b} <=> $hash{$a}} keys %hash) {
      push @returnList, (join " ", ($hash{$_} => $_));
      last unless --$theMaxNum;
      }

      return @returnList;
      }


      The only issue now is those regular expressions at the top. Looking closer
      now, we can see that they each gets called based on the value of the first
      paramter. So let's pull each of those out of the conditional, and out of
      the loop, (the 'o' modifier on the regex prevents it from being recompiled
      each time through the loop, thankfully, but we can do that using the qr//
      function:

      my $re = $doUser ? qr/^\|[^\|]*\|\s([^\.]*\.\S*).*/;
      : qr/^\|[^\|]*\|[^\|]*\|[^\|]*\|\s([^\.]*\.\S*).*/;

      And now we can flatten the loop:

      my %hash = ();
      foreach( @theList ) {
      my $tmp = $_;
      $tmp =~ s/$re/$1/go;
      ++$hash{$tmp};
      }

      That cosmetic $tmp can also now go, but turning the substitution into just
      a match:

      my %hash = ();
      foreach( @theList ) {
      /$re/ and ++$hash{$tmp};
      }

      Collapsing this reduces the entire function to:

      sub getTopList
      {
      my( $doUser, $theMaxNum, @theList ) = @_;

      my $re = $doUser ? qr/^\|[^\|]*\|\s([^\.]*\.\S*).*/
      : qr/^\|[^\|]*\|[^\|]*\|[^\|]*\|\s([^\.]*\.\S*).*/;

      my %hash; /$re/ and ++$hash{$1} foreach @theList;

      my @returnList;
      foreach (sort {$hash{$b} <=> $hash{$a}} keys %hash) {
      push @returnList, (join " ", ($hash{$_} => $_));
      last unless --$theMaxNum;
      }

      return @returnList;
      }

      If I was feeling more energetic I'd then make the whole thing much more
      Functional by splitting the function to produce the sorted list from
      the regular expression 'switch'. And end up with:

      sub getTopList {
      my( $doUser, $theMaxNum, @theList ) = @_;
      my $re = $doUser ? reForUser() : reForNotUser();
      my %hash = countElements([map /$re/ ? $1 : (), @theList], $theMaxNum);
      return map {
      join " ", $hash{$_} => $_
      } sort {$hash{$b} <=> $hash{$a}} keys %hash;
      }

      sub countElements {
      my ($listref, $wanted) = @_;
      my %hash; ++$hash{$_} foreach @$listref;
      return map { $_ => $hash{$_} }
      (sort {$hash{$b} <=> $hash{$a}} keys %hash)[0 .. $wanted - 1];
      }

      sub reForUser { qr/^\|[^\|]*\|\s([^\.]*\.\S*).*/ }
      sub reForNotUser { qr/^\|[^\|]*\|[^\|]*\|[^\|]*\|\s([^\.]*\.\S*).*/ }

      Depending on your background, this could either be much more readable
      than what we had a minute ago, or much less. If we were in an OO set-up
      I'd make it slightly less Functional, and do some template method stuff
      instead. It would become even more readable if I could make the calling
      code pass a better set of arguments, and/or accept a better return value
      (it's this return value that we do the complicated loop from the earlier
      example over, rather than running a 'join').

      But, w.r.t Twiki, the *entire code base* is this mad!

      Thus my original message.

      Tony
    Your message has been successfully submitted and would be delivered to recipients shortly.