Re: Error num in BadQuery patch

Jim Wallace wrote:
> any constructive criticism is welcome.

Oooh, you asked for it.  :)

> +                       throw BadQuery(error(),errnum());

When submitting patches to any project, not just MySQL++, follow the 
coding conventions you see in the surrounding code.  In this instance, 
there needs to be a space after commas.

>  class MYSQLPP_EXPORT BadQuery : public Exception
>  {
>  public:
> -       /// \brief Create exception object, taking C string
> -       explicit BadQuery(const char* w = "") :
> -       Exception(w)
> +       int     errnum; ///< error number associated with execption

Hide this data member behind an accessor, make it private, and add an 
underscore to the end of its name to indicate that it's private.  In 
other words, treat it just like Exception::what_.  This is just another 
instance of following existing coding conventions.

> +       /// \brief Create exception object, taking C string and error
> number

The briefer doxygen comment made sense with just one parameter, but with 
two, you should document both explicitly.  I'd also document the reason 
why the error number is included, probably using your Transaction 
example; the docs should answer the question, "why would I use this 
instead of calling Connection::errnum()?"

Ditto for the C++ string case.

> +       explicit BadQuery(const char* w = "", int err = 0) :
> +       Exception(w), errnum(err)

Variable initializations go on separate lines in the coding style used 
for MySQL++.

Also, all of the changes to BadQuery also need to be made to 
ConnectionFailed and DBSelectionFailed.  It's the same issue.  And 
therefore, the same patch.

> --- query.cpp   2007-07-11 17:37:16.000000000 -0400
> +++ query.cpp   2007-09-25 09:05:12.000000000 -0400
> @@ -81,13 +81,12 @@
>  my_ulonglong
>  Query::affected_rows() const
>  {
>         return conn_->affected_rows();
>  }
>  
> -
>  std::string
>  Query::error()
>  {
>         return conn_->error();
>  }

This is a bogus patch hunk.  Examine a patch's diff carefully before 
submitting it to remove pointless little changes like this.  You might 
be surprised how often you catch things like this.

> +       /// \brief Get the last error number.
> +       ///
> +       /// This just delegates to Connection::errnum().  Query has
> nothing
> +       /// extra to say, so use either, as makes sense in your program.
> +	/// If you want the string *and* number you must 
> +	/// call errnum() before calling error() since error() clears
> errnum()
> +       int errnum() const { return conn_->errnum(); }
> +

This is orthogonal to the BadQuery changes, so it should be a separate 
patch.  The first patch would call conn_->errnum() instead of 
Query::errnum().  The second would add this method and change all 
conn_->errnum() instances inside Query to just errnum().  This keeps the 
revision history clean, and ensures that you can separate effects due to 
each functional change.

Another way to think about this is, if you were writing the svn checkin 
message, would it have to be a list, or a paragraph, or a run-on 
sentence?  If so, the patch is probably trying to do too many things at 
once.  This discipline helps you think more clearly about what each 
change is doing.

>                 if (!result_) {
>                         if (throw_exceptions()) {
> -                               throw BadQuery("Results not fetched");
> +                               throw BadQuery("Results not
> fetched",ER_UNKNOWN_ERROR);

I think we need a new exception type here, not arm-twisting the current 
design to take a bogus error number.  Historically, BadQuery was grossly 
overused; it was essentially a generic exception back in the 1.7 days. 
I think this is just a remnant of this problem.

I suggest calling it UseQueryError.

In keeping with earlier advice, I'd make this a separate patch, too.  It 
should contain the new exception type defn and change the two BadQueries 
that don't take mysql_error() as the message to use it instead.

>         if (!d || !r) {
>                 if (throw_exceptions()) {
> -                       throw BadQuery("ROW or RES is NULL");
> +               throw BadQuery("ROW or RES is
> NULL",ER_INVALID_USE_OF_NULL);
>                 }
>                 else {
>                         return;
>                 }
>         }

The MySQL error constant you're misusing here refers to SQL NULLs, not C 
NULLs.  Also, it's another historical misuse of BadQuery.  Use the 
ObjectNotInitialized exception here instead.  (This is also a separate 
patch.)

--
MySQL++ Mailing List
For list archives: http://lists.mysql.com/plusplus
To unsubscribe:    
http://lists.mysql.com/plusplus?unsub=gcdmc-plusplus@xxxxxxxxxxx