Extension:Echo/Review

• Selective events ([1])
• API module ([2]; 180 lines, mostly boilerplate)

• You might want to make getNotifications's LIMIT value configurable. Kaldari (talk) 20:07, 1 August 2012 (UTC)

• Object model ([3]; 500 lines)

• Notification.php
• line 27 - should it be $obj->user = false;?
• Property initialization can be put inside the constructor, line 6,7 and line 28,29 seems to be duplicated.
• $this->readTimeStamp doesn't seem to be used, in line 64, it's assigned with null other than $this->readTimeStamp
• Subscription.php
• line 29, should just use !is_string() instead of is_object() and should document the 63 length limit, it should be configurable as well
• Event.php
• line 62 - should further check if the type is a valid and enabled type

• NotificationController.php ([4]; 190 lines)

• (minor) The tabbing/formatting on lines 21-26 is confusing. Kaldari (talk) 21:06, 1 August 2012 (UTC)
• formatNotification should probably return false (or do something) if $wgEchoNotificationFormatters[$eventType] isn't set. Kaldari (talk) 21:06, 1 August 2012 (UTC)

• CSS and JS, particularly ext.echo.overlay.js ([5]; ~300 lines)

• This global seems like it might be better put in the mw.config object somehow.
• Seems like there are a lot of bits that could do with code-convention review, but noting the deployment window today, I'll hold off.
• Would it be better to clone the <a> tag here? You might be able to remove a DOM query. Also, since you refer to that link many times in the course of the same function, you could declare a variable to store it, further avoiding unnecessary DOM queries.
• It feels wrong to check for classes like this, maybe use $.hasClass()? Also, while clever, the selector magic is a little hard to read.
• Cheers! --MarkTraceur (talk) 19:16, 1 August 2012 (UTC)
• In ext.echo.overlay.js, I would divide the code into 'set-up' and 'execution' and only put the execution part inside the document.ready wrapper ( $( function() {} ). That way, when document.ready fires, all the set-up is already loaded. Probably doesn't actually make much difference in practice, but that's how I would write it if I were building it. Kaldari (talk) 19:53, 1 August 2012 (UTC)
• Need to remove the console.log() statement before deploy. Kaldari (talk) 19:55, 1 August 2012 (UTC)

• SpecialNotifications.php ([6]; 62 lines)

• It looks like some of this code is duplicated in the API module. Would it be possible to call the API's function instead? Kaldari (talk) 21:11, 1 August 2012 (UTC)

• Notifier.php ([7]; 38 lines)

• For future development, we might want to have the email sent by a job

• Email formatting ([8]; 30–40 lines of 168 lines)

• Comments would be useful here. Kaldari (talk) 21:13, 1 August 2012 (UTC)

• Hooks.php ([9]; 250 lines)

• line 39 - User::newFromName() maybe return false, should check that before calling getId()
• line 41 - creating $user again seems redundant
• Needs more inline comments. Kaldari (talk) 20:03, 1 August 2012 (UTC)

Total: ~1500 lines of code.