Beefy Boxes and Bandwidth Generously Provided by pair Networks
go ahead... be a heretic
 
PerlMonks  

The joys of bad code

by BUU (Prior)
on Oct 26, 2004 at 04:27 UTC ( #402454=perlmeditation: print w/ replies, xml ) Need Help??

I was discussing various stuff on IRC and one of my friends mentioned this great article, written by his company, discussing various "example programs" submitted to his company as part of a resume. http://i4031.net/article/2003/08/badcode.pH.

Some choice examples from the article:
From some C code:
... * Disclaimer: This source code has NOT been compiled. * I would estimate a couple more hours of development to * eliminate any typos and other compile time errors. * Then the fun starts. ... */
The ever popular perl version of arrays:
my ($f0, $f1, $f2, $f3, $f4, $f5, $f6, $f7, $f8, $f9); my ($f10, $f11, $f12, $f13, $f14, $f15, $f16, $f17, $f18, $f19); my ($f20); ... for ($idx = 0; $idx < $NUMBER_OF_ITEMS; $idx++) { $name = "mn" . $idx; $refval = eval "\$f". $idx; ... }
And my favorite:
$ref_urlsWithoutQueryStringTranslatedPassOne = translatePassOneWithoutQueryStringURLs($self->{urlsTrainingNonQueryGo +od}, $self->{CONSTANTS}); $ref_urlsWithoutQueryStringTranslatedPassOnePostGrouping = groupWithoutQueryStringURLsAfterPassOneTranslation($ref_urlsWithoutQu +eryStringTranslatedPassOne, $self->{CONSTANTS});
So what kind of terrible code have you seen or written in your time as programmers?

Update: I just want to make this absolutely clear. This article is not, in any way written by a company. Or a business. Or any sort of inanimate object. It if in fact, written by a human. I think. At least vaguely humanish. Kind of. Mostly.

Comment on The joys of bad code
Select or Download Code
Re: The joys of bad code
by FoxtrotUniform (Prior) on Oct 26, 2004 at 04:56 UTC

      i=i+1;

      I certainly see this in Perl all the time:
          $self->total( $self->total + 1 );

      These habits just never die.

      Cheers, Sören

        Happy-the-monk,
        This isn't quite the same thing. Unless you make the total method lvaluable, neither
        $self->total++; # nor $self->total = $self->total + 1;
        will work. I think the point wasn't about incrementing a variable by one, but feeling the need to document it. I do see the point in documenting it (not in what the code does, but why it does it), but to each their own.

        Cheers - L~R

        I certainly see this in Perl all the time:
          $self->total( $self->total + 1 );

        Before automatically writing this off as being bad code, consider:

        • The class has--or appears to have--a public accessor and a public mutator for total.
        • The mutator is free to do stuff other than setting total. For example, it might log that fact that total has changed, or it might notify other agents who have registered interest in that aspect of the the instance.
        • Subclasses are free to add interesting behavior to mutations of total, even if those mutations are generated internal to the class.

        By coding this as   $self->{total}++;
        you lose these benefits. Sometimes these benefits aren't important, sometimes they're vital. That said, perhaps   $self->increment_total;
        might have been better.

      The scary part is, some of them graduate.
      Its a little scarier when you see:
      /********************************** * * * Add one to i * * * **********************************/ i = i - 1;
      So is it the code or the comment that is wrong?
        So is it the code or the comment that is wrong?
        When the comments don't match the code, I normally assume they're both wrong.

      How about this one (seen in a previous $worklife):

      /* Assign 1 to x */ x=1; /* Just to be sure */ x=1;

      --MidLifeXis

Re: The joys of bad code
by pg (Canon) on Oct 26, 2004 at 05:50 UTC

    I modified some c code about 2 years ago, the code name for that project was ASSCU (sounds like ass cool, don't laugh, it is real, and it stands for "A-Series Source Catch Up"). To indicate the purpose of the change, I wrapped my modifications between this set of comments:

    /* ASSCU begin, Peter */ ... /* ASSCU end, Peter */

    Recently a guy, did some more change to the same .c file for a different project, he copy and pasted some of my code including comments, but made some smart move and changed my name to his name, so all his changes are wrapped between:

    /* ASSCU begin, Tom */ ... /* ASSCU end, Tom */

    During code review, I asked the guy what ASSCU means, and why he put it everywhere. He said that he didn't know, he saw it in my modification, and he just thought it was a company standard to wrap all changes between ASSCU. Everybody in the room laughed, and I told him that that was an old project completed 2 years ago. No more ASSCU.

Re: The joys of bad code
by Juerd (Abbot) on Oct 26, 2004 at 08:33 UTC

    cmd\fdisk\fdisk.c /* P.S. - To whoever winds up maintaining this, I will */ /* apoligize in advance. I had just learned 'C' when */ /* writing this, so out of ignorance of the finer points*/ /* of the langauge I did a lot of things by brute force.*/ /* Hope this doesn't mess you up too much - MT 5/20/86 */ cmd\fdisk\profile.h #define BEGIN { #define END }
    Still my favourite. This is from MS-DOS 6.00.

    Juerd # { site => 'juerd.nl', plp_site => 'plp.juerd.nl', do_not_use => 'spamtrap' }

        That's from the fdisk partition tool? Argh!

        Yes. Explains a lot, does it not?

        Juerd # { site => 'juerd.nl', plp_site => 'plp.juerd.nl', do_not_use => 'spamtrap' }

Re: The joys of bad code
by Mutant (Priest) on Oct 26, 2004 at 09:53 UTC

    I had to try to fix an incredibly broken site written in JSP a year or so ago. The code was so bad, I don't think the guy could've had a hope of passing a first year University paper, let alone coding something that was supposed to make money...

    The highlight for me was a dropdown box that had an onChange Javascript event that built SQL statements on the browser!

Re: The joys of bad code
by 5p1d3r (Hermit) on Oct 26, 2004 at 12:42 UTC
    Not really bad, but funny. I've seen a bit checking routine written by "John" with the misfortunate function name johnbitchk(). In the same source file as the above we have the following:
    some_really_long_type_name *myArray = NULL; /* so we dont have to type that horrible variable name */ myArray = global_pooled_child_array;
    myArray is used exactly once in the following code.

      I never really understood the whole "I dont want to have to type a long variable/package/class name" school of thought/excuses. Most programmers I know are pretty fast typers and it seems to me that it is worth the upfront time to craft well written code with clear and understandable variable/package/class names. IMO, using short names is the wrong kind of laziness.

      -stvn

        Same reason really good note-takers develop a shorthand. It's just faster that way.

        Abbreveations are generally good when they're well-choosen, but poorly-choosen ones suck the big one.

        IBM midrange systems are riddled with bad abbreveations. Could you tell me what a file named "HREMFP" is supposed to have in it? This is an actual file name in a beginners book for midrange programmers/admins that I was given. (And trust me, the name isn't clear from any of the context in the surrounding chapter, either). They love to keep to about 6 characters per file name, weather it could be made shorter or needs to be longer.

        Part of this is technical limitations of the platform, but the limitations seem to be so ingrained in the midrange community that I doubt people would start using sane naming standards even if the technical problems went away.

        "There is no shame in being self-taught, only in not trying to learn in the first place." -- Atrus, Myst: The Book of D'ni.

        TheLongerTheNameIsTheMoreLikekyYouMakeATypo. Of course you should be using strict, but that will not help you with subroutine names in Perl. Both having the names too short and too long is wrong IMHO. Now the question is what the right length is ;-)

        Jenda
        We'd like to help you learn to help yourself
        Look around you, all you see are sympathetic eyes
        Stroll around the grounds until you feel at home
           -- P. Simon in Mrs. Robinson

        I agree.

        To the folks who say that long names are slower to type, or easier to misspell, please do everyone a favor and get a better editor. I don't care which one, but almost all of the good ones have some form of abbreviations or completions. Start typing an identifier, and let the editor suggest the rest of the word for you. One third of the typing, or two hundred percent improvement in readability, depending on your point of view.

        --
        [ e d @ h a l l e y . c c ]

Re: The joys of bad code
by hardburn (Abbot) on Oct 26, 2004 at 13:29 UTC
    # =================================================================== # # Program: # # 1. Parse the input. # 2. Build the query string. # 3. Get the results. # 4. Display the results. # # ===================================================================

    Never mind that this describes almost every single program in existance. (Minus the CGI-specific points like "building the query string".)

    # invoke grep directly, avoid sub-shell (ala realtors script) open(HITS, "-|") || exec('grep','-i',"$key","$MEMBER_TABLE") || di +e "could not spawn grep";

    Yup, that's right. He calls out to grep to search a flat-file. Never mind that Perl's regular expression engine could do the same thing in the same ammount of lines. Sometimes he'll even check that the results from grep match against a Perl regex, just be really sure. Naturally, he does not sanitize $ENV{PATH}. At least he checks the return value of open.

    I don't know who this "realtor" guy is. The snippets above where done by a guy who left long before I started working here, but apparently he didn't know Perl at all and cobbled his CGIs together from various online sources (cargo-culter of the highest order).

    "There is no shame in being self-taught, only in not trying to learn in the first place." -- Atrus, Myst: The Book of D'ni.

Re: The joys of bad code
by demerphq (Chancellor) on Oct 26, 2004 at 14:29 UTC

    This charmer was found in the codebase im working on now. I almost fell out of my chair when I first encountered it:

    Public Function MyMonthName(ByVal m As Long, _ Optional ByVal appr As Boolean = False) As + String Dim tmp As String Select Case m Case 1: tmp = "January" Case 2: tmp = "February" Case 3: tmp = "March" Case 4: tmp = "April" Case 5: tmp = "May" Case 6: tmp = "June" Case 7: tmp = "July" Case 8: tmp = "August" Case 9: tmp = "September" Case 10: tmp = "October" Case 11: tmp = "November" Case 12: tmp = "December" Case Else: tmp = Format(m, "0") End Select If appr Then MyMonthName = Left(tmp, 3) Else MyMonthName = tmp End If End Function

    ---
    demerphq

      First they ignore you, then they laugh at you, then they fight you, then you win.
      -- Gandhi

      Flux8


      Perhaps I don't know enough Visual Basic (I think this is what this is anyways...). Care to elaborate so I too can fall out of my chair?

      thor

      Feel the white light, the light within
      Be your own disciple, fan the sparks of will
      For all of us waiting, your kingdom will come

        Glad somebody was brave enough to ask. It seems like reasonable code to me, too. There are better ways to do it in other languages, but if you're writing in VB, you have to live with the limitations that implies. (In Perl I'd either use DateTime or make it a hash lookup.)

        "There is no shame in being self-taught, only in not trying to learn in the first place." -- Atrus, Myst: The Book of D'ni.

Re: The joys of bad code
by McMahon (Chaplain) on Oct 26, 2004 at 14:54 UTC
    At my last job, I was asked to build an automated test harness for a large COBOL system. They wanted me to begin with a particular part of the system.

    So I cranked up the debugger with the intention of discovering and building tests for the error conditions the program was capable of generating.

    In the code I discovered over 100 instances of this:

    GOTO FATAL-OUT

    FATAL-OUT was a subroutine that shut down the process with no error messages, no stack dump, no evidence at all that the program had encountered a problem. Then the OS would restart the process automatically.

    Apparently some years back this program had routinely overflowed log files and caused performance problems by the sheer number of errors it generated. So the developers simply stopped it from generating errors.

    Problem solved!

    I didn't work there very long. But I did succeed in building a very nice user-driven system-level automated test harness for a *different* part of the system.
      Ouch that sound familiar. My colleague used to write shell scripts to repeatedly invoke perl scripts that died frequently. When i debugged the perl script (strict and warnigs help ;-) and it would finish the job, he still would't run it directly because he just didn't trust it. *sigh*
Re: The joys of bad code
by jayrom (Pilgrim) on Oct 26, 2004 at 15:30 UTC
    This is my all-time favorite funny code:
    my %hash = ( 1 => 1, 2 => 2, 3 => 3, 4 => 4 );
    Of course *I* didn't write this ;-)
    Strangely enough nobody's offered bad code they wrote...

    jayrom

      Strangely enough nobody's offered bad code they wrote

      Is this a challange? Oh boy, do I have some bad code that I've written. Of course, I know better nowadays. But back then... boy oh boy. Let me dig through my dead code archive for some examples.

      So yeah. Now that I've let some of this stuff out there, and everyone thinks I'm a terrible coder, are you happy? Did I live up to your challenge? :-)

      Update: added <readmore> tags.

      Update: I guess my willingness to post my own old, bad code gets me downvotes. How very pleasant.

        I can't believe you got downvoted!
        I wish I could have upvoted you more than once for your willingness to show yourself in an unflattering way.
        I, for one, has only to look at yesterday's code to already find stupid things that could be rewritten better... but I'd rather not share them ;-)

        jayrom

      Strangely enough nobody's offered bad code they wrote...

      I would like to offer up one of the first programs I ever wrote, a bowling 'simulation' written in BASIC when I was in high school. Talk about spaghetti code!

      I would like to..... But I can't. It's on paper tape. (Really!)

      I suppose I could scan it...

      TheEnigma

      That isn't *necessarily* bad code. It could be discrete input data that, by pure coincidence, turned out to be linear with no offset. :)
Re: The joys of bad code
by TomDLux (Vicar) on Oct 26, 2004 at 15:39 UTC

    At the place I worked last winter, there was code that would read files which were sometimes a million lines long:

    my @lines = read_file( ... ); # note copying, rather than passing a re +f for ( @lines ) { .... }
    So they read in and copy a million lines, then process it one line at a time.

    --
    TTTATCGGTCGTTATATAGATGTTTGCA

Re: The joys of bad code
by TomDLux (Vicar) on Oct 26, 2004 at 17:00 UTC

    Here's one I deal with at my present job. This isn't a mistake by an intern, but production code relied on by many people.

    Instead of writing a module that you include into your code, system X was implemented as a program which reads in your code! You invoke it as system_X my_customization_code.pl

    Then system X reads in your file and evals it. But compilers don't know about comments, so comment lines are skipped:

    # Read in configuration parameter file open (FILE,"<$scriptFile") || die "Cannot open $scriptFile"; while (<FILE>) { # Remove comments $script .= $_ unless /^\#/; } close FILE; eval $script; die "$@" unless $@ eq "";

    Besides the foolishness of having things backwards, using eval on a string instead of on a file name means that when I debug code ( M-x perldb in emacs ), the system doesn't know where the source code is located and so cannot display it. But that wasn't enough for the villain who wrote this .... by skipping comment lines, the line number displayed by the debugger bears no relation to the actual line in the file.


    But I have found a way to beat the system, considering I cannot change the basic system X files. My include file is short, and requires other files, which contain my actual code. So the eval reads in the files which contain my real code, and the debugger works. For debugging purposes, I have my own copy of system X which does not skip over comments, since the Perl compiler can handle that without my help.

    --
    TTTATCGGTCGTTATATAGATGTTTGCA

Re: The joys of bad code
by McMahon (Chaplain) on Oct 26, 2004 at 21:51 UTC
    I can't resist one more story, from the same company responsible for FATAL-OUT. (As a tester, I see the *weirdest* stuff)...

    So besides FATAL-OUT, this company had a custom config tool that was so byzantine that it took weeks of training and mistake-making to grasp even the bare essentials of what it was supposed to do.

    This system ran on the Tandem Guardian OS, so as always when I can't figure something out, I crank up the Tandem debugger to check it out. As I start to step into the stack, things are looking weirder and weirder.

    I realized that the original author, rather than use the normal OS commands, has *re-written* each Tandem command. And he's done it in Tandem's proprietary programming language, TAL.

    This is equivalent to writing from scratch in C and including in your application things things like "ls" or "./" or "cd", giving the user the impression that he is interacting with the OS and not with your program. This was unbelievably paranoid in every sense of the word. He is *impersonating* the actual OS. Did he not trust the native implementation? Tandem was/is *the* fault-tolerant system, those system calls are rock-solid. Did he want to be able to spoof the users later on? Who can tell...

    The more I looked at the code, the more I suspected that the developer had a very particular mental illness. I asked around and sure enough, he'd been fired years ago for a substance abuse problem. It showed in his code.
Re: The joys of bad code
by Golo (Friar) on Oct 26, 2004 at 23:46 UTC
    A colleague once had a problem with a script he wrote for some strange macro language. He was wondering why a loop was running to short. When I looked at the code I saw the following:
    1 = 1 + 1
    And the language really allowed this.
Re: The joys of bad code
by czth (Sexton) on Oct 27, 2004 at 00:44 UTC
    Bah, since when do companies write articles? I wrote the article (the link is to my site). BUU did ask my permission before posting ("I'm czth and I approve of this message"). Not all code is from applicants but I think most of it is; I tried to anonymize the code where I thought it necessary to protect the guilty.

    For those that are tired of rants, I wrote a more didactic version in response to some comments after I first posted the article (on k5). Standard disclaimer along the lines of "my code isn't always perfect either" goes here, too.

    Really, a lot of bad code comes from not refactoring, and whenever I see unfactored sprawling inefficient untidy code I itch to refactor it into something more elegant (and usually smaller). Doubtless my bad code collection will continue to grow.

Re: The joys of bad code
by Juerd (Abbot) on Oct 27, 2004 at 16:18 UTC

    $q = param("q"); if (!$q || $q eq ""){$q = "home";} ... $rid = md5_hex(rand(999999999999999)); ... if ($q eq "dbsetup"){ ... $dbh = DBI->connect("DBI:mysql:$MYSQL{database}:$MYSQL{serv +er}:3306",$MYSQL{user}, $MYSQL{pass},{ RaiseError => 0, AutoCommit => + 1 }); ... $dbh->do("DROP TABLE users"); $dbh->do("CREATE TABLE users (sid TEXT, user TEXT, pass TEXT, +userlevel char(1))"); ... } ... sub getmonthname{ my($nr) = @_; #my%names; $names{1} = "januari"; $names{2} = "februari"; $names{3} = "maart"; $names{4} = "april"; $names{5} = "mei"; $names{6} = "juni"; $names{7} = "juli"; $names{8} = "augustus"; $names{9} = "september"; $names{10} = "oktober"; $names{11} = "november"; $names{12} = "december"; return $names{$nr}; } ... my$email = param("email"); if ($email){$reply = $email;}else{$reply = "mailer-deamon\@CENSORED.nl +"} open (SENDMAIL,"|mail -s 'Reactie formulier CENSORED' $contactto -f $r +eply") || &printerror;
    This is all the same 1200 line index.pl (a CGI script). In this script,
    • no value is escaped anywhere
    • no DBI placeholders are used
    • every SELECT is SELECT *
    • every fetch is @row = $sth->fetchrow_array
    • dates are stored in a TEXT column in dd/mm/yyyy (or dd-mm-yyyy) format
    • ... or even in three different TEXT columns
    • lexicals are used for only a third of all variables
    • the DBI->connect(...) is repeated everywhere
    • virtually no error checking is done and it is usually even explicitly disabled (RaiseError => 0)
    • half of all code is HTML
    • there are lots of if (!$foo || $foo eq "") { $foo = "..." }
    • everything is hard coded (don't let %MYSQL fool you: that too is hard coded)
    • both Dutch and English are used, sometimes even in one place: "day", "maand", "jaar" (database columns), "newsoverzicht"
    • is a banner of the proud author:
      #-----------------------------------------------# # Site Script # # # # Designed for CENSORED # # by CENSORED CENSORED CENSORED # # # #-----------------------------------------------#
    Gotta love fixing broken features in this code. It is dangerously insecure, and there's no money available te replace it, and making it secure without replacing it entirely is exactly as much work.

    Juerd # { site => 'juerd.nl', plp_site => 'plp.juerd.nl', do_not_use => 'spamtrap' }

Re: The joys of bad code
by Your Mother (Canon) on Nov 02, 2004 at 02:40 UTC

    In the last week got to see (in large scale production code):

    if ( $test || 0 ) { ... # and if ( uc $string eq 'String' ) { ...
Re: The joys of bad code
by bgreenlee (Friar) on Nov 09, 2004 at 23:15 UTC

    I just came across these two gems written by a coworker (slightly simplified--comments are mine):

    if ($some_condition) { } else { # do something ... }

    and

    if ($x > 10) { # do something } elsif ($x <= 10) { # do something else } else { # do something else }

    -b

Re: The joys of bad code
by TomDLux (Vicar) on Nov 15, 2004 at 19:01 UTC

    Came across some code I just had to share. How many steps is this programmer from good, idiomatic perl?

    sub padfield { my ( $value, $fieldlength ) = @_; while ( length $value < $fieldlength ) { my $pad = '0'; $value = join( '', $pad, $value ); } return $value; }

    --
    TTTATCGGTCGTTATATAGATGTTTGCA

Re: The joys of bad code
by Random_Walk (Parson) on Nov 16, 2004 at 15:33 UTC

    I worked with a perl coder once who wrote something like this

    #!/usr/bin/perl -w `cd test_dir` `gunzip test_script.tar.gz` `tar -xvf test_script.tar` #you are getting the idea now

    I asked him why he did not just write a shell script, his answer with no note of irony was that he did not know shell !

    The following was causing serious memmory hogage in a production environment, multiple instances (30+) were running at any time, each took about 4 seconds to complete and eat a big chunck of RAM.

    # we enter here with a hostname $host and must look it up # in a file containing details for >100K hosts open FH, $hosts_file or die "something appropriate\n"; while (<FH>) { ($a,$b,$c,$d)=split $_, /\t/, 4; # first entry in each line is hostname # $b is the data we need for the host $hash{$a}=$b; } if (exists $hash{$host}) { return $hash{$host} } else { return error } # needles to say %hash was global, well everything was # and %hash used again

    after the obvious changes it reduced to running in around 0.3 seconds, with a tweak to sort the hosts_file file when it was generated (once per day) and binary search it it was down to <0.04 secs per run

    Cheers,
    R.

Re: The joys of bad code
by snookmz (Beadle) on Nov 22, 2004 at 20:34 UTC
    Unfortunately, my past (and probably present) code..

    When setting up a DBI query:

    $command = "UPDATE staff SET $tag=\"$value\" WHERE id=\"$params{\"id\" +}\" AND $tag!=\"$value\""; my $sth = $dbh->prepare("$command");


    Most of my old code is currently running a simple employee status / timesheet system. I always groan when i'm asked to changed / fix something in it as it's a house of cards just waiting to topple over.

    I like to think I've improved a lot, but when I think of the stuff the monks here write I realise just how much I have to go :)

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlmeditation [id://402454]
Approved by atcroft
Front-paged by grinder
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others pondering the Monastery: (7)
As of 2014-07-28 07:53 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    My favorite superfluous repetitious redundant duplicative phrase is:









    Results (193 votes), past polls