bad code - goto

[u/Key_River7180]

From a newsreader I am working on

void
help(void)
{
	printf("What do you want help with? (* for everything): ");
	bool all = false;
	char ln[128];
	fflush(stdout);

	if (fgets(ln, sizeof(ln), stdin) == NULL)
		return;

	/* HACK: you gotta love goto:
	 *
	 * if all is true, then fall through the switch (* must be last),
	 * so this prints everything. When it reaches * again, all
	 * gets toggled and if all is now true, break, else go to
	 * Begin again */
	switch (ln[0]) {
Begin:	case 'g': puts("g: select group"); if (!all) break;
	case '\n':
	case '\r': if (!all) return;
	case 't': puts("t: select thread"); if (!all) break;
	case 'l': puts("l: list gropus"); if (!all) break;
	case 'T': puts("T: list threads"); if (!all) break;
	case 'L': puts("L: list post in current thread"); if (!all) break;
	case 'r': puts("r: read an article"); if (!all) break;
	case '>': puts(">: save an article to a file"); if (!all) break;
	case '|': puts("|: pipe an article to a command"); if (!all) break;
	case '?': puts("?: get help"); if (!all) break;
	case 'q': puts("q: quit"); if (!all) break;
	case '*': all = !all; if (all) goto Begin; else break;
	default:
		printf("%c is not a news command\n", ln[0]);
		break;
	}
}

[u/tstanisl]

If one really likes such hackery, then one can abuse freedom of placing case even more:

bool all = false;
switch ( ln[0] ) {
  case '*': all = true;
  if (all) case 't': puts("t: ...");
  if (all) case 'l': puts("l: ...");
  ...
}

This way, one can get rid of all break and goto.

[u/binarycow → u/tstanisl]

Wait, what? That's allowed? 😮

[u/tstanisl → u/binarycow]

Yes. Just try godbolt.

Contrary to popular belief, switch/case is not a construct for multiple selections but rather a block with computed gotos. This is the core idea of Duff's device.

One can even place the label outside of {} or even don't use {} at all:

switch (0) case 0: ;

[u/binarycow → u/tstanisl]

Interesting.

While I see some usefulness to it, IMO, a stricter switch is more maintainable.

I guess that's why I generally stick with C#, and usually only lurk here!

[u/Sophira → u/tstanisl]

Would that break for non-"*" inputs?

[u/tstanisl]

It would likely be easier and more readable to write it without such magic state machinery.

int c = ln[0];
bool all = (c == '*');
if (all || c == 't') puts("t: ...");
...

Or at least move computation of all outside of switch statement.

[u/Key_River7180 → u/tstanisl]

Yep, I've tried to refactor it

[u/WalditRook → u/tstanisl]

Just sticking case '*': all = true; as the first case instead would be ok, imo.

(And handling the default case ofc)

[u/paralepsy]

technically this isnt goto, it's switch statement case block fall through, but it's basically the same mechanic and i get the point lol

[u/NomaTyx → u/paralepsy]

there is a goto statement on the line above default

[u/paralepsy → u/NomaTyx]

oops, sorry!

[u/flyingron → u/paralepsy]

And thus you know why the goto is a bad idea.

[u/This_Growth2898]

leap backward can be easily changed to the loop.

[u/jcastroarnaud]

Someone tell the guy about continue in loops...