r72348 MediaWiki - Code Review archive

I don't see anything listed at Manual:Coding conventions on proper indentation for switch statements. I don't necessarily object to that change (although the way I did it is the way vim likes to indent it, IIRC), but if we're going to impose that as standard, could you document it as such?

I kind of prefer the version without extra indentation. Even though I use tabsize of two, the lines tend to get very close to the 80 char width.

Agreed, it should be documented. I personally prefer indenting the cases in a switch, but that's just me.

Back to the original question: the Internet seems divided on the issue. I tried Googling around, and people seem to have mixed feelings.

• a K&R guide says to not indent the case and default.
• w:Indent_style - silent on the issue entirely :(
• astyle - interestingly has an --indent-switch parameter

And of course source code around the Internet is pretty mixed. Some indent, some don't. Nobody seems to care (nearly) as much about this as opposed to where to put the curly braces.

I also prefer indenting the cases.

Personally, I don't agree. Although, not always, I think the usage of braces indicates a level of indentation to be added (same with brackets if involving multiple lines

Granted, it makes no difference to the code, performance, it only changes "readability"

Is it worth throwing it out to the maiing lists, or what?

I personally believe that forcing curve braces everywhere actually makes code less readable.

I agree too. In certain cases, last of curly braces increases readability.

if( isset( $params['foo1'] ) ) $foo1 = true; if( isset( $params['foo2'] ) ) $foo2 = true; if( isset( $params['foo3'] ) ) $foo3 = true; if( isset( $params['foo4'] ) ) $foo4 = true; if( isset( $params['foo5'] ) ) $foo5 = true; if( isset( $params['foo6'] ) ) $foo6 = true; if( isset( $params['foo7'] ) ) $foo7 = true; if( isset( $params['foo8'] ) ) $foo8 = true;

if you're going to do all that you might as well do

for( $i = 1; $i < 9; $i++ ) {
	$var = "foo$i";
	$$var = isset( $params[$var] );
}

That was intended as an example. In a real case, you have a unique name for each element that can't be identified by for loops.

That code is extremely unreadable to start with. That level of manual repetition (even if you change the names around) indicates that you're doing something wrong. I don't think curly braces decrease readability at all in real-world cases, except where the code deserves to be rewritten anyway (lots of ifs in a row).

I personally think that

if( !isset( $foo ) ) $foo = 'foo'; if( !isset( $bar ) ) $bar = 'bar'; if( !isset( $baz ) ) $baz = 'baz';

is much more cleaner than

if( !isset( $foo ) ) {

   $foo = 'foo';

   $bar = 'bar';

   $baz = 'baz';

That style is only more readable if you have several in a row. If it's just a few sprinkled here and there, I find the one-line style much less readable, because I don't expect a statement to start so far into the line.

This is wikitext, mostly, so indent with a space to preserve formatting.

Agreed, for the occasional statement, it's fine. However, for huge blocks, I prefer the former.

Not that I really care either way now, does this need to stay fixme?

The "fixme" is that someone should add something to Manual:Coding conventions about indentation in switch statements – at least saying "we have no preference". At that point it doesn't matter much if this stays or goes, but at least it will be clear that future cleanup commits shouldn't change indentation in switch statements.

http://www.mediawiki.org/w/index.php?title=Manual:Coding_conventions&action=historysubmit&diff=357077&oldid=356359

http://www.mediawiki.org/wiki/Special:Code/MediaWiki/72348/reply/10123#c10123

closing code-tag here (entire page was wrapped in it, bug in CR)

closing another one