← Back to overview

Spaghetti Code

Strictly speaking, these are groups of objects written for one specific business process but not meant to be reused elsewhere. In practice it's copy-pasted code fragments that appear in more or less the same form at least 5–10 times across different modules.

Symptoms

  • Chaotic control flow: lots of goto, deep nesting, exits from the middle.
  • The same fragment appears in several places with minor edits.
  • Changing one place requires manual edits in five others.

Example

An example of spaghetti code from a Linux SCSI driver (with minor changes) from the early 2000s:

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) {        /* busy? */
                        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 select  */
        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:                   /* search 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#,       */

The "good" version: the same thing, but with explicit functions and no goto:

static int wait_ready(unsigned int port) {
    for (int n = 0; n < 0x30000; n++) {
        if ((inb(port) & 0x80) != 0)      /* busy? */
            return wait_io_done(port);
    }
    return TCM_SYNC;
}

Why it happens

What you need to know: at least the principles of functional programming. Ideally — OOP and architectural principles like DDD.

This is the kind of code with a very strong regional flavour, and if you outsource, say, to India, you'll run into it quite often.

How to fix it

  • Split large functions into small ones with a single level of abstraction.
  • Eliminate goto and deep nesting.
  • Extract repeated fragments into functions and methods.
  • Add characterization tests before refactoring.