User:Salvatore Ingala/Review 1

(in progress)

Use OOP, Luke!
Static stuff like getGadgetPrefsDescription is simply creepy, why not $gadget->getPrefsDescription? Further points dduced for duplication: the same function uses Gadget::loadStructuredList, then iterates over the array, then does its job. Let's see the callers... bah, ApiGetGadgetPrefs does the same!
 * ✅ in . Salvatore Ingala 18:06, 15 June 2011 (UTC)

Gadgets_body.php

 * This thingie had grown to the point where it could make sense to split it into several files.
 * ✅ in . Salvatore Ingala 16:48, 20 June 2011 (UTC)
 * Profiling: add wfProfileIn/wfProfileOut wherever appropriate.
 * ✅ in . Added to the UserLoadOptions hook, it's probably the only relevant place in new code. Salvatore Ingala 14:13, 21 June 2011 (UTC)
 * SQL injection via gadget name in getUserPrefs/setUserPrefs
 * Why duplicate stuff from class User here at all?
 * ✅ in : now using UserLoadOptions and UserSaveOptions hooks instead of direct DB access. Salvatore Ingala 17:07, 16 June 2011 (UTC)
 * Verify that all existing gadgets will survive wrapping into yet another closure.
 * Once getGadgetPrefsDescription has been turned into a member function, cache its result.
 * ✅ in . Salvatore Ingala 14:13, 21 June 2011 (UTC)
 * serialize is much faster than JSON for prefs storage.
 * ✅ in . Salvatore Ingala 21:59, 17 June 2011 (UTC)

ApiGetGadgetPrefs.php
Why not just merge it into ApiQueryGadgets?

jquery.formBuilder.js
]]".
 * Rename function $s, too close to "[[Manual:Coding conventions#jQuery|variable holding a jQuery object
 * ✅ in Salvatore Ingala 15:48, 20 June 2011 (UTC)

Other

 * I don't know how stuff works if there are no unit tests. It may work, but sometimes things work accidentally.
 * Don't use wfMsgInSaneNameThatIsAlsoLong-like functions, there's Message class for new code.
 * ✅ in . Salvatore Ingala 14:13, 21 June 2011 (UTC)
 * Use doxygen-compliant function documentation, see Manual:Coding conventions.