Code review comment for lp:~vjsamuel/drizzle/refactor-drizzle

Revision history for this message
Jay Pipes (jaypipes) wrote :

Hi Vijay!

Great work on refactoring the client to use program_options! It's great to see the use of mysys (drizzled::internal::) removed here.

Most of my comments are about naming and style. I've noticed you're a bit inconsistent in that area, so I'll try to point out a few things that need fixed :)

1)

put one and only one space between if|else|else if and the left parentheses.
put one space before and one space after comparison operators (==, !=, >, >=, etc)
put one space after negation operators (use if (! some_bool) or if (not some_bool))

You use a mix of a number of styles. For instance:

118 + if(in_connect_timeout>3600*12)
129 + if( in_max_input_line<4096 || in_max_input_line>(int64_t)2*1024L*1024L*1024L)
140 + if (s.find("--password")==0)
154 + if(s.substr(10)=="=")

Please go through and make sure you follow the style guidelines regarding proper spacing :)

Also, please be consistent in comment spacing...you use both this:

144 + //check if no argument is passed.
and
62 + // check if --password has quotes, remove quotes and return the value

Please use a space between // and the start of the comment

2.

Consider renaming the reg_password() function to something more descriptive, and placing a comment block before the function to indicate *why* that function exists and what it's doing so that developers hacking on the client can properly avoid all the pain you worked hard with Monty to work through! :)

Cheers!
jay

review: Needs Fixing

« Back to merge proposal