453781 - Merge TableExists and IndexExists, and replace nsCString with nsCAutoString

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Alfred Kayser]

Spinoff from bug 452836:
I noticed some local nsCString's here. Should these not be replaced by
nsCAutoString (because generally these queries are less than 64 characters,
thus saving a heap malloc)?

Also TableExists nd IndexExists are roughly the same (but with subtle and
probably unintended differences in error handling), so these should be factored
out, to remove code duplication (and inconsistencies).

Comment 1

[Alex Gartrell]

Attachment: Patch refactors code slightly

May not be up to par style-wise.  Your input is appreciated

Comment 2

[Alfred Kayser]

Good that it is taken up.
Two minor smaller style issues:
1. don't use tabs, replace them by spaces.
2. the opening brace { is not on the next line, but on the same line (after switch,if,etc)

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 363497 [details] [diff] [review]
Patch refactors code slightly

> NS_IMETHODIMP
>+mozStorageConnection::DBElementExists(enum DBElementType aElementType, 
>+									const nsACString& aSQLStatement, PRBool *_retval)
This actually needs to be nsresult.  NS_IMETHODIMP is used for methods from an idl, which this isn't.


>+    nsCAutoString query(NS_LITERAL_CSTRING("SELECT name FROM sqlite_master WHERE type = '"));
I think you can just do nsCAutoString query("SELECT...") here and not wrap it in an NS_LITERAL_CSTRING.

>+	switch(aElementType)
>+	{
>+		case INDEX:
>+			query.Append(NS_LITERAL_CSTRING("index"));
>+			break;
>+		case TABLE:
>+			query.Append(NS_LITERAL_CSTRING("table"));
>+			break;
Same with these - should be able to use the const char * here.

>+	}
>+	query.Append(NS_LITERAL_CSTRING("' AND name ='"));
and here

>     query.Append(aSQLStatement);
>     query.AppendLiteral("'");
and here

>+NS_IMETHODIMP
>+mozStorageConnection::TableExists(const nsACString& aSQLStatement, PRBool *_retval)
>+{	
>+	return mozStorageConnection::DBElementExists(TABLE, aSQLStatement, _retval);
no need to use the classname, just the method name is fine.


>+	return mozStorageConnection::DBElementExists(INDEX, aIndexName, _retval);
ditto

>+	/* enum for the DBElementExists function */
>+	enum DBElementType { INDEX, TABLE };
I'm not too fond of this name, but coming up with a better one has been difficult.  I think DatabaseElement would be better.

Also, could you format the enum like this (but don't need to assign a value to it) and a similar describing comment please:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/src/nsDownloadManager.h#224

>+
>+	/**
>+	 * Has functionality shared by IndexExists and TableExists
>+	 */
full javadoc style comments please explaining all the pArameters.

>+	NS_IMETHODIMP DBElementExists(enum DBElementType aElementType, 
>+		const nsACString& aSQLStatement, PRBool *_retval);
Let's go with DatabaseElementExists too please, aSQLStatement should probably be aElementName.  Lastly, see the previously linked nsDownloadManager.h for the right way to format multiline function declarations.

Thanks for doing this.  This will make it easier to add support for views as well.

Comment 4

[Alex Gartrell]

Attachment: patch 2

Fixed some of the style issues I had before.  Removed STRING wrappers, changed type to nsresult, added javadoc style comments above function in header file, changed tabs to 4-spaces.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 363610 [details] [diff] [review]
patch 2

>+nsresult
>+mozStorageConnection::DatabaseElementExists(enum DatabaseElementType aElementType, 
>+									const nsACString& aElementName, PRBool *_retval)
You still have tabs here, and please just have one pArameter on each line.

>     query.AppendLiteral("'");
Can you change this to just Append please since we are no longer using an NS_LITERAL_CSTRING

>@@ -474,45 +484,29 @@ mozStorageConnection::TableExists(const 
>     } else if (srv == SQLITE_DONE) {
>         exists = PR_FALSE;
>     } else {
>         HandleSqliteError("TableExists finalize");
>         return ConvertResultCode(srv);
>     }
> 
>     *_retval = exists;
>-    return NS_OK;
>+	return NS_OK;
nit: tab

>+NS_IMETHODIMP
>+mozStorageConnection::TableExists(const nsACString& aSQLStatement, PRBool *_retval)
>+{	
>+	return DatabaseElementExists(TABLE, aSQLStatement, _retval);
nit: tab

> NS_IMETHODIMP
> mozStorageConnection::IndexExists(const nsACString& aIndexName, PRBool* _retval)
>+	return DatabaseElementExists(INDEX, aIndexName, _retval);
nit: tab

>+    /**
>+     * Type to indicate type of element to check for existence of
>+     *  in database with protected function DatabaseElementExists
nit: no indent on next line; end sentence with punctuation please

>+    /**
>+     * @brief sets value of *_retval to true if element exists, false otherwise
>+     * @param aElementType the type of element to check the existence of
>+     * @param aElementName the name of the element to check for
>+     * @param _retval pointer to PRBool in which true is placed if the element
>+     *      exists, and false otherwise
>+     * @return NS_OK on success, NS_ERROR_NOT_INITIALIZED or converted error code
>+     *  on failure
>+     */
I always forget we don't quite do javadoc comments like everyone else.  Lose the @brief, and give a basic description of that this does (Determines if the specified primitive exists), then have a newline.

For @params, have the name, then one a newline include the description indented to match the a prefix.

And of course, xpcom makes stuff a bit weird.  Since there is only one return value have the @returns line describe what _retval is going to be, and ignore the fact that we actually return nsresult.  These turn into exceptions in JS anyway.

While we are at it (sorry, forgot this last time), why don't we call _retval _exists instead to better reflect what it means.

>+    nsresult DatabaseElementExists(enum DatabaseElementType aElementType, 
>+        const nsACString& aElementName, PRBool *_retval);
nit:
nsresult DatabaseElementExists(enum DatabaseElementType aElementType, 
                               const nsACString& aElementName,
                               PRBool *_retval);

Almost there!  Thanks!

Comment 6

[Alex Gartrell]

Attachment: patch 3

Used a style script I was pointed to by humph.  Hopefully it'll save you some time.

Comment 7

[Alfred Kayser]

Two more small nits:
One should check if _retval is not null before using it:
  if (_exists != nsnull) *_exists = exists;

Change the description from:
   sets value of *_retval to true if element exists, false otherwise
to: 
   Determines if the specified primitive exists in the database.

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> One should check if _retval is not null before using it:
>   if (_exists != nsnull) *_exists = exists;
we have NS_ENSURE_ARG_POINTER for that, but I'm also pretty sure that passing null for an outparam is a violation of xpcom rules.  We don't check that anywhere else in storage, and I don't see the point of checking now.

Comment 9

[Alfred Kayser]

NS_ENSURE_ARG_POINTER indeed.
But xpcom rule doesn't apply to the new private function itself,
so other code within that class could potentially call this function with a null...

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 363647 [details] [diff] [review]
patch 3

r=sdwilsh

I'll address a few minor nits I have, make sure the tests still pass, and then attach the patch for landing.  Thanks!

Comment 11

[Shawn Wilsher :sdwilsh]

Attachment: for checkin

Comment 12

[Shawn Wilsher :sdwilsh]

Thanks for doing this!

http://hg.mozilla.org/mozilla-central/rev/04243842d3fa