r68694 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r68693‎ | r68694 | r68695 >
Date:01:21, 29 June 2010
Author:kaldari
Status:resolved (Comments)
Tags:
Comment:
handle small numbers of languages better in interface display
Modified paths:
  • /trunk/extensions/CentralNotice/SpecialCentralNotice.php (modified) (history)

Diff [purge]

Index: trunk/extensions/CentralNotice/SpecialCentralNotice.php
——@@ -454,10 +454,12 @@
455455 $project_langs = array();
456456 $project_langs = $this->getNoticeLanguages( $row->not_name );
457457 $language_count = count( $project_langs );
458 - if ( $language_count > 1 ) {
 458+ if ( $language_count > 3 ) {
459459 $languageList = "multiple ($language_count)";
 460+ } elseif ( $language_count > 0 ) {
 461+ $languageList = implode(', ',$project_langs);
460462 } else {
461 - $languageList = $project_langs[0];
 463+ $languageList = '';
462464 }
463465 $fields[] = $languageList;
464466

Follow-up revisions

RevisionCommit summaryAuthorDate
r70048block scoping style per suggestion at r68694 code reviewkaldari21:43, 27 July 2010
r70904localizing comma list per comment at r68694kaldari18:36, 11 August 2010

Comments

#Comment by MarkAHershberger (talk | contribs)   20:09, 27 July 2010

scoping: I know PHP allows this sort of funky scoping, but this 

	if ( $language_count > 3 ) {
		$languageList = "multiple ($language_count)";
	} elseif ( $language_count > 0 ) {
		$languageList = implode(', ',$project_langs);
	} else {
		$languageList = '';
	}

should be 

	$languageList = '';
	if ( $language_count > 3 ) {
		$languageList = "multiple ($language_count)";
	} elseif ( $language_count > 0 ) {
		$languageList = implode(', ',$project_langs);
	}
#Comment by Simetrical (talk | contribs)   20:13, 27 July 2010

It's a stylistic preference. Both are fine, IMO. People from a C background should just get used to non-block scoping – block scoping is not enforced in practically any scripting language, and it's often quite inconvenient.

#Comment by MarkAHershberger (talk | contribs)   20:31, 27 July 2010

Scoping issues in PHP can lead to E_NOTICE. Code like the following is all too common: 

  if( $var > 10 ) {
    $msg = "something"; # the only time it is assigned.
  }

  if( $var > 1 ) {
    echo "Somethings we say $msg.\n";
  }
#Comment by Kaldari (talk | contribs)   21:23, 27 July 2010

The block scoping feels redundant to me, but that's probably because I haven't touched C or Java in a few years :) I'm happy to change it to what you have suggested though if that is considered best practice.

#Comment by MarkAHershberger (talk | contribs)   01:31, 28 July 2010

fwiw, I haven't coded in C or Java in for-ever. The last time I did any "serious" C (I've only fixed a few bugs in Java) was 15+ years ago.

I just find that using sane scoping avoids some common coding pitfalls. That, and it makes maintenance easier.

#Comment by Kaldari (talk | contribs)   21:44, 27 July 2010

fixed in r70048.

#Comment by Tim Starling (talk | contribs)   01:35, 28 July 2010

I thought it was better as it was.

#Comment by Catrope (talk | contribs)   08:23, 11 August 2010

The comma should be make localizable by using $wgLang->commaList()

#Comment by Kaldari (talk | contribs)   18:36, 11 August 2010

fixed in r70904

Status & tagging log