Incorrect placeholder %s for uid, vid and nid [#418088]

hello mbutcher

Just gone through the quiz 3.x-dev code, i would like to let you know some points

1. You are using invalid placeholder %s for uid, vid and nid in quiz.module

-  if($quiz->takes > 0) {
-    $query = "SELECT COUNT(*) AS takes FROM {quiz_node_results} WHERE uid = %s AND nid = %s AND vid = %s";
+  if ($quiz->takes > 0) {
+    $query = "SELECT COUNT(*) AS takes FROM {quiz_node_results} WHERE uid = %d AND nid = %d AND vid = %d";

2. Missing CVS keyword $Id$ in some files
3. Violating the naming conversions of function i.e you are using mixed case names for some functions ( getData() instead of using get_data())
4. Other trivial things like spacing between control statement and braces.

I have created a patch to fix the above mentioned things.
(i know this is development version may not be good to point out these things at this time.)

--
Thanks
Sivaji

Comment	File	Size	Author
	quiz-3.x-dev.patch	26.34 KB	sivaji_ganesh_jojodae

Comments

Comment #1

mbutcher commented 30 March 2009 at 15:55

Issue tags:

+Coding standards

Wow! That's a huge patch.

One thing needs to be changed back, though.

The current convention is that strings should be concatenated like this: "string 1" . "string 2" (note the space on each side of the concatenation operator). That additional space is a fairly new thing, and some of the older handbook pages still erroneously suggest that the standard is to not put a space after a string literal. You can check out the core modules for examples.

CamelCase is the standard for naming when you are writing object oriented code like unit tests and classes. So sometimes getData() is correct. I couldn't find any places in the patch where you changed this, though.

Also, if you ever notice cases where an 'if' statement is does a loose check, feel free to correct if you want. I try to fix them as I go, but I haven't done a comprehesive search.

Examples of this:

if ($foo) // <-- BAD!

if (!empty($foo)) // <-- GOOD

if (isset($foo)) // <-- GOOD

if ($foo === TRUE) // <-- GOOD

Great patch, though! Looks like you did a lot of work. And I think you touched areas of the code that I have never looked at!

Matt

Comment #2

sivaji_ganesh_jojodae commented 1 April 2009 at 19:29

Dear mbutcher,

I have not changed any CamelCase names anywhere because i am not sure about them. Thanks for spending your time to enlighten my knowledge, i will stick on to your words.

--
Thanks
Sivaji