Extension:Echo/Review


 * Selective events
 * API module (180 lines, mostly boilerplate)
 * You might want to make getNotifications's LIMIT value configurable. Kaldari (talk) 20:07, 1 August 2012 (UTC)


 * Object model (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 (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 (~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  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 (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 (38 lines)
 * For future development, we might want to have the email sent by a job


 * Email formatting (30–40 lines of 168 lines)
 * Comments would be useful here. Kaldari (talk) 21:13, 1 August 2012 (UTC)


 * Hooks.php (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.