views:

2216

answers:

12

Can you post a short example of real, overdone spaghetti code, possibly saying what it does? Can you show me a little debugger's nightmare?

I don't mean IOCCC code, that is science fiction. I mean real life examples that happened to you...

Update

The focus has changed from "post some spaghetti code" to "what is exactly spaghetti code?". From a historical perspective, the current choices seem to be:

  • old Fortran code using computed gotos massively
  • old Cobol code using the ALTER statement
+4  A: 

Real spaghetti-code requires a multitude of non-local gotos. Sadly this is not possible using most modern languages.

Edit: Some suggest exceptions and longjmp as substitutes for GOTO. But these are far to limited and structured, since they only allow you to return up the callstack. Real GOTO allows you to jump to any line anywhere in the program, which is necessary to create real spaghetti.

JacquesB
Do you really mean "sadly"?
Joe Skora
How about simulated non-local gotos using Exceptions?
jop
longjmp and setjmp work well in C and C++ :-)
Nils Pipenbrinck
I agree with Joe... sadly?
Jason Down
It's sad that we can no longer write obscure, confusing stuff. That was real job security when I was a kid. Can't outsource support for something impenetrable. Futurama Quote: "Do you have an extra goto 10?"
S.Lott
far too limited ?using continuations as first class objects, passing them around and storing them in deeply rescursively nested structures can get you quite far these days... (just kidding)
blabla999
+6  A: 

You've asked for it, you'll get it:

This is the source of a DOS .com file that plays the Blue Danube waltz. The executable is just 176 bytes in size. Code is re-used as data and vice versa.

.286
.model tiny

g4 equ 55-48           ; removed note-decoding !
a4 equ 57-48           ; now: storing midi-notes for octaves 0..2 and convert
h4 equ 59-48           ; to 4..6 with a simple add 48.

c5 equ 60-48
d5 equ 62-48
e5 equ 64-48
g5 equ 67-48
h5 equ 71-48

c6 equ 72-48
d6 equ 74-48
e6 equ 76-48
g6 equ 79-48           ; = 00011111b

pp  equ 0              ;  c4 is not used in the walz, using it as play-pause.
EOM equ 1              ; c#4 is also available... End Of Music
                       ; warning: experts only beyond this point !

pau1 equ 00100000b     ; bitfield definitions for note-compression
pau2 equ 01000000b     ; you can or a pau to each note!
pau3 equ 01100000b

;rep1 equ 01000000b    ; rep1 is history (only used once).
;rep3 equ 11000000b    ; rep3 was never used.

rep2 equ 10000000b     ; or a rep2 to a note to play it 3 times.

drumsize equ 5

.code
org 100h

start:
                mov  ah,9
                mov  dx,offset msg
                int  21h                    ; print our headerstring

                mov  dx,0330h               ; gus midi megaem -port
                mov  si,offset music_code   ; start of music data

mainloop:

    ; get new note (melody)

                xor  bp,bp                  ; bp= repeat-counter

                lodsb                       ; get a new note
                cmp  al, EOM                ; check for end
                jne  continue
                ret

continue:
                jns  no_rep2                ; check for rep2-Bit
                inc  bp
                inc  bp                     ; "build" repeat-counter

no_rep2:
                push ax                     ; save the note for pause

    ; "convert" to midi-note

                and  al,00011111b
                jz   skip_pp                ; check pp, keep it 0
                add  al,48                  ; fix-up oktave

skip_pp:
                xchg ax,bx                  ; bl= midi-note

play_again:
                mov  cl,3
                push cx                     ; patch program (3= piano)
                push 0c8h                   ; program change, channel 9

    ; wait (cx:dx) times

                mov  ah,86h                 ; wait a little bit
                int  15h

    ; prepare drums

                dec  di                     ; get the current drum
                jns  no_drum_underflow
                mov  di,drumsize

no_drum_underflow:

    ; play drum

                push dx                     ; volume drum
                push [word ptr drumtrk+di]  ; note   drum
                mov  al,99h
                push ax                     ; play channel 10

    ; play melody

                push dx                     ; volume melody
                push bx                     ; note   melody

                dec  ax                     ; replaces dec al :)

                push ax                     ; play channel 9

    ; send data to midi-port

                mov  cl,8                   ; we have to send 8 bytes

play_loop:
                pop  ax                     ; get the midi event
                out  dx,al                  ; and send it
                loop play_loop

    ; repeat "bp" times

                dec  bp                     ; repeat the note
                jns  play_again

    ; check and "play" pause

                xor  bx,bx                  ; clear the note, so we can hear
                                            ; a pause
    ; decode pause value

                pop  ax
                test al,01100000b
                jz   mainloop               ; no pause, get next note

    ; decrement pause value and save on stack

                sub  al,20h
                push ax
                jmp  play_again             ; and play next drum

; don't change the order of the following data, it is heavily crosslinked !
music_code      db pp or rep2

                db g4 or rep2 or pau1
                db h4 or pau1, d5 or pau1, d5 or pau3
                db d6 or pau1, d6 or pau3, h5 or pau1, h5 or pau3

                db g4 or rep2 or pau1
                db h4 or pau1, d5 or pau1, d5 or pau3
                db d6 or pau1, d6 or pau3, c6 or pau1, c6 or pau3

                db a4 or rep2 or pau1
                db c5 or pau1, e5 or pau1, e5 or pau3
                db e6 or pau1, e6 or pau3, c6 or pau1, c6 or pau3

                db a4 or rep2 or pau1
                db c5 or pau1, e5 or pau1, e5 or pau3
                db e6 or pau1, e6 or pau3, h5 or pau1, h5 or pau3

                db g4 or rep2 or pau1
                db h4 or pau1, g5 or pau1, g5 or pau3
                db g6 or pau1, g6 or pau3, d6 or pau1, d6 or pau3

                db g4 or rep2 or pau1
                db h4 or pau1, g5 or pau1, g5 or pau3
                db g6 or pau1, g6 or pau3, e6 or pau1, e6 or pau3

                db a4 or rep2 or pau1
                db c5 or pau1, e5 or pau1, e5 or pau3, pp or pau3
                db c5 or pau1, e5 or pau1, h5 or pau3, pp or pau3, d5 or pau1

                db h4 or pau1, h4 or pau3
                db a4 or pau1, e5 or pau3
                db d5 or pau1, g4 or pau2

;                db g4 or rep1 or pau1
; replace this last "rep1"-note with two (equal-sounding) notes
                db g4
                db g4 or pau1

msg             db EOM, 'Docking Station',10,'doj&sub'
drumtrk         db 36, 42, 38, 42, 38, 59  ; reversed order to save some bytes !

end start
Nils Pipenbrinck
This doesn't count, assembler language relies on jumps - you basically can't write non-spaghetti assembler.
Code is reused as data, are you sure your name is not Mel?
Gamecat
Code reused as data does not count either. That is truly ingenious but it is not spaghetti code. Spaghetti code is code where the flow of control is nearly impossible to follow.
Ehh... try to follow the loop around play_loop. That loop executes 8 times and pops the data from stack. Now the big question is: Where do the 8 words that are popped come from?
Nils Pipenbrinck
+6  A: 

From a Linux SCSI driver (which shall remain nameless to protect the guilty):

wait_nomsg:
        if ((inb(tmport) & 0x04) != 0) {
                goto wait_nomsg;
        }
        outb(1, 0x80);
        udelay(100);
        for (n = 0; n < 0x30000; n++) {
                if ((inb(tmport) & 0x80) != 0) {        /* bsy ? */
                        goto wait_io;
                }
        }
        goto TCM_SYNC;
wait_io:
        for (n = 0; n < 0x30000; n++) {
                if ((inb(tmport) & 0x81) == 0x0081) {
                        goto wait_io1;
                }
        }
        goto TCM_SYNC;
wait_io1:
        inb(0x80);
        val |= 0x8003;          /* io,cd,db7  */
        outw(val, tmport);
        inb(0x80);
        val &= 0x00bf;          /* no sel     */
        outw(val, tmport);
        outb(2, 0x80);
TCM_SYNC:
/* ... */
small_id:
        m = 1;
        m <<= k;
        if ((m & assignid_map) == 0) {
                goto G2Q_QUIN;
        }
        if (k > 0) {
                k--;
                goto small_id;
        }
G2Q5:                   /* srch from max acceptable ID#  */
        k = i;                  /* max acceptable ID#            */
G2Q_LP:
        m = 1;
        m <<= k;
        if ((m & assignid_map) == 0) {
                goto G2Q_QUIN;
        }
        if (k > 0) {
                k--;
                goto G2Q_LP;
        }
G2Q_QUIN:               /* k=binID#,       */

How did I locate this gem?

find /usr/src/linux -type f -name \*.c | 
while read f
do 
    echo -n "$f "
    sed -n 's/^.*goto *\([^;]*\);.*/\1/p' $f | sort -u | wc -l
done | 
sort +1rn |
head

The output is a series of lines listing files ordered by the number of gotos to distinct labels, like the following:

kernel/fork.c 31
fs/namei.c 35
drivers/infiniband/hw/mthca/mthca_main.c 36
fs/cifs/cifssmb.c 45
fs/ntfs/super.c 47
Diomidis Spinellis
There are lots of gotos in the above but it's actually remarkably clear what they are doing, so this doesn't get my vote for spaghetti.
Remarkably clear? goto G2Q_QUIN? goto G2Q_LP?
Diomidis Spinellis
Hmmm... For sure, I don't know what it does. But the flow isn't excessively difficult to follow...
Federico Ramponi
I agree with BKB -- use of gotos by itself does not constitute spaghetti code.
mxg
Still, many of the gotos could be easily replaced with loop (while, do) and loop exit statements (break, continue).
Diomidis Spinellis
I agree with BKB goto used extensively as a "C" way to implement try catch mechanism and it's actually dictated way to do it at my work. It's make code very clear.
Ilya
This code was probably optimized to eliminate the function call overhead. Still those goto labels look almost like functions, so it's not that bad. It would be worse if they were functions but they would not be declared one after the other, but intermixed with other functions.
csuporj
I am sure that the code can be written in a structured more readable style without suffering from any significant overhead.
Diomidis Spinellis
+7  A: 

Real spaghetti code was done in COBOL and used the ALTER statement.

Here's an example. Here's another, while listed a "humor", I've seen this kind of thing. Almost got fired once for noting that any program with an Alter statement was obviously in a state of sin. I refused to "maintain" that program, it was quicker to replace it than understand it.

S.Lott
This seems the more realistic so far...
Federico Ramponi
That is so messed up...
PeterAllenWebb
A: 

Spaghetti code: Originating in the early 60's in Italy as an alternate recipe for certain pasta dishes, spaghetti code was cooked up by one restaurant entrepreneur who attempted to automate the creation of a fool-proof entree. Pressed by the lack of time to complete the design the engineer/chef cut corners which introduced problems in the recipe early on. In a frantic attempt to remedy a good idea gone bad, various spices were quickly added to the concoction as the recipe grew out of control. The result was a stringy, twisty, yet potentially tasty pile of text that would later grow to be a practice cherished by developers world-wide.

Cliff
I was with up to the potentially tasty pile... cherished by developers. Not sure I agree with tasty or cherished. Everything else, though, is true.
S.Lott
+1  A: 

This is from a MIDI parser I wrote some time ago. It was a quick and dirty proof of concept, but nevertheless, I shall take the blame for its ugliness: 4 levels of nested conditionals plus the dreaded multiple returns. This code was meant to compare 2 MIDI events in order to sort them by priority when writing to a file. Ugly as it was, it did the job decently, though.

internal class EventContainerComparer : IComparer {

 int IComparer.Compare(object a, object b) {
  MIDIEventContainer evt1 = (MIDIEventContainer) a;
  MIDIEventContainer evt2 = (MIDIEventContainer) b;

  ChannelEvent chanEvt1;
  ChannelEvent chanEvt2;

  if (evt1.AbsoluteTime < evt2.AbsoluteTime) {
   return -1;
  } else if (evt1.AbsoluteTime > evt2.AbsoluteTime) {
   return 1;
  } else { 
   // a iguar valor de AbsoluteTime, los channelEvent tienen prioridad
   if(evt1.MidiEvent is ChannelEvent && evt2.MidiEvent is MetaEvent) {
    return -1;
   } else if(evt1.MidiEvent is MetaEvent && evt2.MidiEvent is ChannelEvent){
    return 1;
   // si ambos son channelEvent, dar prioridad a NoteOn == 0 sobre NoteOn > 0
   } else if(evt1.MidiEvent is ChannelEvent && evt2.MidiEvent is ChannelEvent) {

    chanEvt1 = (ChannelEvent) evt1.MidiEvent;
    chanEvt2 = (ChannelEvent) evt2.MidiEvent;

    // si ambos son NoteOn
    if( chanEvt1.EventType == ChannelEventType.NoteOn 
     && chanEvt2.EventType == ChannelEventType.NoteOn){

     // chanEvt1 en NoteOn(0) y el 2 es NoteOn(>0)
     if(chanEvt1.Arg1 == 0 && chanEvt2.Arg1 > 0) {
      return -1;
     // chanEvt1 en NoteOn(0) y el 2 es NoteOn(>0)
     } else if(chanEvt2.Arg1 == 0 && chanEvt1.Arg1 > 0) {
      return 1;
     } else {
      return 0;
     }
    // son 2 ChannelEvent, pero no son los 2 NoteOn, el orden es indistinto
    } else {
     return 0;
    }
   //  son 2 MetaEvent, el orden es indistinto
   } else {
    return 0;
   }
  }
 }
}
Juan Pablo Califano
good example: you dont need goto for that good old spaghetti-look !
blabla999
+3  A: 

Here is the Duff's Device, from Matt's answer to this question:

int n = (count + 7) / 8;
switch (count % 8) {
case 0: do { *to = *from++;
case 7:      *to = *from++;
case 6:      *to = *from++;
case 5:      *to = *from++;
case 4:      *to = *from++;
case 3:      *to = *from++;
case 2:      *to = *from++;
case 1:      *to = *from++;
           } while (--n > 0);
}
Federico Ramponi
Duff's device isn't spaghetti though ... ;) Still you got +1 from me... :)
Thomas Hansen
+9  A: 

To me, a more modern example of spaghetti code is when you have 20 dlls and every DLL references each other in one way or another. Your dependency graph looks like a huge blob, and your code hops all over the place with no real order. Everything is inter-dependent.

Brian Genisio
Yeah. Goto isn't the only way to make something you can can never be sure is right. I like the term "macaroni code".
Mike Dunlavey
+5  A: 

There's also Ravioli Code, which is the opposite. Nice little chunks of functionality, a clean interface neatly wrapped around meaty goodness, all sat in a nice sauce of framework.

MadKeithV
Didn't know that! Perfect +1
Daniel Silveira
+5  A: 

Don't forget to mention Object-oriented spaghetti. This is when you try to use all the design patterns in the book, even when they don't make sense. This leads to spaghetti code at conceptual level, which is far more detrimental to quality than classical goto-based spaghetti code.

A: 

Have you ever looked at code generated by Flex/Bison scanner and generator? A plethora of labels and preprocessor directives.

It's absolutely impossible to understand what's inside.. and absolutely impossible to follow flow of the program.

That's definetely spaghetti code.

Jack
+3  A: 

I'm not pulling this out of my head. This is what I have had to work with, albeit simplified. Let's say that basically you have a program that needs an enum:

enum {
   a, b, c;
} myenum;

But instead what we have is

HashTable t;
t["a"] = 0;
t["b"] = 1;
t["c"] = 2;

But of course, no implementation of a hash table is good enough so there is a local implementation of hash tables, which contains about 10 times more code than an average open source implementation with half the features and double the number of bugs. The HashTable is actually defined virtual, and there's a factory HashTableFactory to create instances of HashTables, but true to the pattern HashTableFactory is also virtual. To prevent an infite cascade of virtual classes there's a function

HashTableFactory *makeHashTableFactor();

Thus everywhere where the code needs myenum's it carries a reference to the instance of a HashTable and HashTableFactory, in case you want to make more HashTable's. But wait, that's not all! This is not how the hash table is initialized, but it's done by writing a code that reads XML:

<enum>
  <item name="a" value="0"/>
  <item name="b" value="1"/>
  <item name="c" value="2"/>
</enum>

and inserts into a hash table. But the code is "optimised" so that it doesn't read an ascii file myenum.xml, but instead there's a compile time script which generates:

const char* myenumXML = [13, 32, 53 ....];

from myenum.xml and the hash table is initialized by a function:

void xmlToHashTable(char *xml, HashTable *h, HashTableFactory *f);

which is called:

HashTableFactory *factory = makeHashTableFactory();
HashTable *t = facotry.make();
xmlToHashTable(myenumXML, t, f);

Ok, so we have a lot of code to get an enum structure. It's basically used in a function:

void printStuff(int c) {
   switch (c) {
   case a: print("a");
   case b: print("b");
   case c: print("c");
   }
}

and this is called in a context where:

void stuff(char* str) {
   int c = charToEnum(str);
   printStuff(c);
}

So what we actually have is instead of

void stuff(char *str) {
   printf(str);
}

we have manged to generate thousands of lines of code (private new, buggy, complex, implementation of hashtables, and xml readers, and writer) in place of the above 3.

Joe Smith