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!

 Done in r90127. 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.
  •  Done in r90469. Salvatore Ingala 16:48, 20 June 2011 (UTC)
• Profiling: add wfProfileIn()/wfProfileOut() wherever appropriate.
  •  Done in r90525. 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?
  •  Done in r90215: 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.
  •  Done in r90525. Salvatore Ingala 14:13, 21 June 2011 (UTC)
• serialize() is much faster than JSON for prefs storage.
  •  Done in r90322. 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 "variable holding a jQuery object ".
  •  Done in r90466 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.
  • Partially done in r90660 and r90702. Salvatore Ingala 09:42, 24 June 2011 (UTC)
• Don't use wfMsgInSaneNameThatIsAlsoLong()-like functions, there's Message class for new code.
  •  Done in r90525. Salvatore Ingala 14:13, 21 June 2011 (UTC)
• Use doxygen-compliant function documentation, see Manual:Coding conventions#Documentation.