[wxsqlite3] More suggestions

Talk here about issues with one of the components hosted at wxCode, or suggest features for it.
Post Reply
Jamie
Filthy Rich wx Solver
Filthy Rich wx Solver
Posts: 205
Joined: Wed Mar 30, 2005 10:56 pm

[wxsqlite3] More suggestions

Post by Jamie »

Hi,

I've got some more suggestions, here goes:

1. It is more efficent to pass wxString by const reference instead of by value. For example:

Code: Select all

wxLongLong GetInt64(const wxString columnName, wxLongLong nullValue = 0);
would become

Code: Select all

wxLongLong GetInt64(const wxString& columnName, wxLongLong nullValue = 0);
etc.

2. It would be better to replace this:

Code: Select all

const wxString wxERRMSG_NODB          = _("No Database opened");
...
const wxString wxERRMSG_BIND_CLEAR    = _("Error clearing bindings");
with this:

Code: Select all

static const wxChar* wxERRMSG_NODB          = _("No Database opened");
...
static const wxChar* wxERRMSG_BIND_CLEAR    = _("Error clearing bindings");
3. Replace this:

Code: Select all

wxString GetColumnName(const int columnIndex);
with this:

Code: Select all

wxString GetColumnName(int columnIndex);
etc.

Rationale here: http://wxwidgets.org/standard.htm#no_const_int
utelle
Moderator
Moderator
Posts: 1125
Joined: Tue Jul 05, 2005 10:00 pm
Location: Cologne, Germany
Contact:

Re: [wxsqlite3] More suggestions

Post by utelle »

Jamie wrote:I've got some more suggestions, here goes:
Thank you very much for your suggestions to improve the wxSQLite3 component. Suggestions for the enhancement of wxSQLite3 are always welcome!
Jamie wrote:1. It is more efficent to pass wxString by const reference instead of by value.
wxSQLite3 was my first wxWidgets component. In my other component wxPdfDocument I already use const wxString& whereever it's suitable. But I have to admit I forgot to change the code of wxSQLite3 in the same manner.
Jamie wrote:2. It would be better to replace this:

Code: Select all

const wxString wxERRMSG_NODB          = _("No Database opened");
...
with this:

Code: Select all

static const wxChar* wxERRMSG_NODB          = _("No Database opened");
...
Yes, I agree. _(...) returns wxChar*, so it's more efficient to use wxChar* for the constants as well.
Jamie wrote:3. Replace this:

Code: Select all

wxString GetColumnName(const int columnIndex);
with this:

Code: Select all

wxString GetColumnName(int columnIndex);
I'll try to keep the wxWidgets coding style guide in mind.

All suggested code changes are done already in CVS.

Thanks again,

Ulrich
Jamie
Filthy Rich wx Solver
Filthy Rich wx Solver
Posts: 205
Joined: Wed Mar 30, 2005 10:56 pm

Post by Jamie »

A few more:

Replace wxDateTime& with const wxDateTime& for these functions:

Code: Select all

  void BindDate(int paramIndex, wxDateTime& date);
  void BindTime(int paramIndex, wxDateTime& time);
  void BindDateTime(int paramIndex, wxDateTime& datetime);
  void BindTimestamp(int paramIndex, wxDateTime& timestamp);
Make this const:

Code: Select all

operator const char*() { return m_buffer; }
would become:

Code: Select all

operator const char*() const { return m_buffer; }
This allows these:

Code: Select all

  bool CheckSyntax(wxSQLite3StatementBuffer& sql);
  int ExecuteUpdate(wxSQLite3StatementBuffer& sql);
  wxSQLite3ResultSet ExecuteQuery(wxSQLite3StatementBuffer& sql);
  int ExecuteScalar(wxSQLite3StatementBuffer& sql);
  wxSQLite3Table GetTable(wxSQLite3StatementBuffer& sql);
  wxSQLite3Statement PrepareStatement(wxSQLite3StatementBuffer& sql);
to become these:

Code: Select all

  bool CheckSyntax(const wxSQLite3StatementBuffer& sql);
  int ExecuteUpdate(const wxSQLite3StatementBuffer& sql);
  wxSQLite3ResultSet ExecuteQuery(const wxSQLite3StatementBuffer& sql);
  int ExecuteScalar(const wxSQLite3StatementBuffer& sql);
  wxSQLite3Table GetTable(const wxSQLite3StatementBuffer& sql);
  wxSQLite3Statement PrepareStatement(const wxSQLite3StatementBuffer& sql);
utelle
Moderator
Moderator
Posts: 1125
Joined: Tue Jul 05, 2005 10:00 pm
Location: Cologne, Germany
Contact:

Post by utelle »

Jamie wrote:A few more: [...]
Done.

Thanks for taking the time to suggest improvements,

Ulrich
Jamie
Filthy Rich wx Solver
Filthy Rich wx Solver
Posts: 205
Joined: Wed Mar 30, 2005 10:56 pm

Post by Jamie »

A little improvement for the sample. Add this to the MyAuthorizer class:

Code: Select all

  wxString ConvertAuthToString(int type)
  {
    wxString s;

    switch (type)
    {
      case SQLITE_CREATE_INDEX:
        s = _T("SQLITE_CREATE_INDEX");
        break;
      case SQLITE_CREATE_TABLE:
        s = _T("SQLITE_CREATE_TABLE");
        break;
      case SQLITE_CREATE_TEMP_INDEX:
        s = _T("SQLITE_CREATE_TEMP_INDEX");
        break;
      case SQLITE_CREATE_TEMP_TABLE:
        s = _T("SQLITE_CREATE_TEMP_TABLE");
        break;
      case SQLITE_CREATE_TEMP_TRIGGER:
        s = _T("SQLITE_CREATE_TEMP_TRIGGER");
        break;
      case SQLITE_CREATE_TEMP_VIEW:
        s = _T("SQLITE_CREATE_TEMP_VIEW");
        break;
      case SQLITE_CREATE_TRIGGER:
        s = _T("SQLITE_CREATE_TRIGGER");
        break;
      case SQLITE_CREATE_VIEW:
        s = _T("SQLITE_CREATE_VIEW");
        break;
      case SQLITE_DELETE:
        s = _T("SQLITE_DELETE");
        break;
      case SQLITE_DROP_INDEX:
        s = _T("SQLITE_DROP_INDEX");
        break;
      case SQLITE_DROP_TABLE:
        s = _T("SQLITE_DROP_TABLE");
        break;
      case SQLITE_DROP_TEMP_INDEX:
        s = _T("SQLITE_DROP_TEMP_INDEX");
        break;
      case SQLITE_DROP_TEMP_TABLE:
        s = _T("SQLITE_DROP_TEMP_TABLE");
        break;
      case SQLITE_DROP_TEMP_TRIGGER:
        s = _T("SQLITE_DROP_TEMP_TRIGGER");
        break;
      case SQLITE_DROP_TEMP_VIEW:
        s = _T("SQLITE_DROP_TEMP_VIEW");
        break;
      case SQLITE_DROP_TRIGGER:
        s = _T("SQLITE_DROP_TRIGGER");
        break;
      case SQLITE_DROP_VIEW:
        s = _T("SQLITE_DROP_VIEW");
        break;
      case SQLITE_INSERT:
        s = _T("SQLITE_INSERT");
        break;
      case SQLITE_PRAGMA:
        s = _T("SQLITE_PRAGMA");
        break;
      case SQLITE_READ:
        s = _T("SQLITE_READ");
        break;
      case SQLITE_SELECT:
        s = _T("SQLITE_SELECT");
        break;
      case SQLITE_TRANSACTION:
        s = _T("SQLITE_TRANSACTION");
        break;
      case SQLITE_UPDATE:
        s = _T("SQLITE_UPDATE");
        break;
      case SQLITE_ATTACH:
        s = _T("SQLITE_ATTACH");
        break;
      case SQLITE_DETACH:
        s = _T("SQLITE_DETACH");
        break;
      default:
        s = _T("Unknown?");
    }

    return s;
  }
and make this change:

Code: Select all

-    cout << "AUTH: " << type << ","
+    cout << "AUTH: " << (const char*) ConvertAuthToString(type).mb_str() << ","
Another suggestion: when you need to break backwards compatibility consider changing this:

Code: Select all

-  virtual int Authorize(int type, wxString& arg1, wxString& arg2, wxString& arg3, wxString& arg4) = 0;
+  virtual wxAuthorizationResult Authorize(wxAuthorizationCode type, const wxString& arg1, const wxString& arg2, const wxString& arg3, const wxString& arg4) = 0;
Since the sqlite arguments are const char* and you already have enums for the type and return code. Perhaps you could mention it in the documentation until it is changed?

Thanks for all your effort!
utelle
Moderator
Moderator
Posts: 1125
Joined: Tue Jul 05, 2005 10:00 pm
Location: Cologne, Germany
Contact:

Post by utelle »

Jamie wrote:A little improvement for the sample. Add this to the MyAuthorizer class:
I added a static AuthorizationCodeToString method to the authorizer base class since such a conversion method might be useful not only in the sample.
Jamie wrote:Another suggestion: when you need to break backwards compatibility consider changing this:

Code: Select all

-  virtual int Authorize(int type, wxString& arg1, wxString& arg2, wxString& arg3, wxString& arg4) = 0;
+  virtual wxAuthorizationResult Authorize(wxAuthorizationCode type, const wxString& arg1, const wxString& arg2, const wxString& arg3, const wxString& arg4) = 0;
You are right. It's certainly better to enforce compile time type checking. I changed the result type and parameter type.

The changes can be found in wxCode CVS.
Jamie wrote:Since the sqlite arguments are const char* and you already have enums for the type and return code. Perhaps you could mention it in the documentation until it is changed?
I'm not sure I understand what you mean. All const char* parameters are converted to wxString before passing them to the Authorize method. Type and return code are int in the sqlite interface - and I was too lazy to convert them to enums - but this is now changed in CVS.

Regards,

Ulrich
Jamie
Filthy Rich wx Solver
Filthy Rich wx Solver
Posts: 205
Joined: Wed Mar 30, 2005 10:56 pm

Post by Jamie »

utelle wrote:I added a static AuthorizationCodeToString method to the authorizer base class since such a conversion method might be useful not only in the sample.
Yep, nice. Looking at latest sqlite header file there are 3 more values that I missed:

Code: Select all

#define SQLITE_ALTER_TABLE          26   /* Database Name   Table Name      */
#define SQLITE_REINDEX              27   /* Index Name      NULL            */
#define SQLITE_ANALYZE              28   /* Table Name      NULL            */
utelle wrote:The changes can be found in wxCode CVS.
Great!
utelle wrote:I'm not sure I understand what you mean. All const char* parameters are converted to wxString before passing them to the Authorize method. Type and return code are int in the sqlite interface - and I was too lazy to convert them to enums - but this is now changed in CVS.
Passing them as wxString& and not const wxString& gives the impression that modifying them has some effect even though it doesn't.

Thanks for quickly reviewing all of my suggestions.
utelle
Moderator
Moderator
Posts: 1125
Joined: Tue Jul 05, 2005 10:00 pm
Location: Cologne, Germany
Contact:

Post by utelle »

Jamie wrote:Looking at latest sqlite header file there are 3 more values that I missed:
Oops, you're right. These values were added in version 3.2.7 of SQLite. Another thing I have to keep in mind, when I upgrade wxSQLite3 to a new version of SQLite. Thanks for pointing it out.
Jamie wrote:Passing them as wxString& and not const wxString& gives the impression that modifying them has some effect even though it doesn't.
Oh, I see. I overlooked it when I modified the wxString references.

The needed modifications are done and can be found in CVS.
Jamie wrote:Thanks for quickly reviewing all of my suggestions.
Don't mention it. :)

Ulrich
Post Reply