Code review requested for BusDbAdapter.java

[GoogleCodeExporter]

What steps will reproduce the problem?
1. update to head version
2. do code review for BusDbAdapter.java

Original issue reported on code.google.com by QUANGHUY...@gmail.com on 16 Feb 2010 at 4:11

[GoogleCodeExporter]

update to version 91

Original comment by QUANGHUY...@gmail.com on 17 Feb 2010 at 4:22

[GoogleCodeExporter]

CODE REVIEW - Bus Stop Alarm
Date: Feb 17, 2010
File: BusDbAdapter.java
Revision: 91
Author: Huy Dang
Reviewer: Derek Cheng

Comments:

I think you've done a good job with comments documenting the variables and 
methods in 
the source file. I would put the description of the file (The one starting with 
"Simple notes...") at the top, along with the names of the authors. It would 
also 
help to put down where this class will be used (main page, etc.). 

I noticed you have created some String constants that are used in the SQL 
statements 
in the BusDbAdapter class that represent attributes of the entries in the 
database. 
The variables starting KEY_* in particular are declared public. Would it be 
more 
appropriate to declare them private if they will not be used in other classes?

Separating out instance variables from static variables would make the code 
easier to 
read. Also it would be great if you could briefly document what DatabaseHelper 
is 
used for. 

For pre-defined SQL statements such as DATABASE_CREATE_DEST, use capital 
letters for 
SQL keywords would make it easier to read. (e.g. CREATE TABLE destination ...)

In the nested private class DatabaseHelper, the constructor is declared package-
private. Would it be a better idea to declare it public instead? 

In the open() method, when you say "if it cannot be opened," does that mean 
that the 
database did not exist before, or the user does not have permission to open it? 
i.e. 
Would the behavior differ in either case? 

In close() method, it would be a good idea to document that no more operations 
should 
be performed after close() is called on that adapter. Also, what happens if you 
call 
close() without ever calling open()?

In the fetchAll[Major]Destinations() methods call the DatabaseHelper's query() 
method 
with 7 arguments, the last 5 of which are null. It would be great if you could 
briefly explain what the nulls are for, that would help those who read the code 
better understand what is going on when that method is called.

In the fetch[Major]Destination() methods, can you explain why movedToFirst() is 
called by mCursor if it is not null resulting from the query() method? Under 
what 
conditions will it return null?

Original comment by chen...@gmail.com on 17 Feb 2010 at 7:32

[GoogleCodeExporter]

Here's some additional info for this code review (for TAs, if you're checking 
from 
this page)

CODE REVIEW - Bus Stop Alarm
Date: Feb 17, 2010
File: BusDbAdapter.java
Revision: 91
Author: Huy Dang
Reviewer: Derek Cheng

Summary of review:

This is the first review of the file in question (BusDbAdapter.java). This file 
contains code responsible for interactions between the application and the 
database. 
The entire file (~150 lines) is examined for source code formatting, adherence 
to 
standard Java and Android programming style guidelines, correctness of 
predefined SQL 
statements, and general object-oriented principles. 

Comments:

COMMENTS

I think you've done a good job with comments documenting the variables and 
methods in 
the source file. I would put the description of the file (The one starting with 
"Simple notes...") at the top, along with the names of the authors. It would 
also 
help to put down where this class will be used (main page, etc.). 

CODE DECISIONS

I noticed you have created some String constants that are used in the SQL 
statements 
in the BusDbAdapter class that represent attributes of the entries in the 
database. 
The variables starting KEY_* in particular are declared public. Would it be 
more 
appropriate to declare them private if they will not be used in other classes?

Separating out instance variables from static variables would make the code 
easier to 
read. Also it would be great if you could briefly document what DatabaseHelper 
is 
used for. 

In the nested private class DatabaseHelper, the constructor is declared package-
private. Would it be a better idea to declare it public instead? 

In the fetchAll[Major]Destinations() methods call the DatabaseHelper's query() 
method 
with 7 arguments, the last 5 of which are null. It would be great if you could 
briefly explain what the nulls are for, that would help those who read the code 
better understand what is going on when that method is called.

CODING STANDARDS

For pre-defined SQL statements such as DATABASE_CREATE_DEST, use capital 
letters for 
SQL keywords would make it easier to read. (e.g. CREATE TABLE destination ...)

LOGIC / ERROR HANDLING

In the open() method, when you say "if it cannot be opened," does that mean 
that the 
database did not exist before, or the user does not have permission to open it? 
i.e. 
Would the behavior differ in either case? 

In close() method, it would be a good idea to document that no more operations 
should 
be performed after close() is called on that adapter. Also, what happens if you 
call 
close() without ever calling open()?

In the fetch[Major]Destination() methods, can you explain why movedToFirst() is 
called by mCursor if it is not null resulting from the query() method? Under 
what 
conditions will it return null?

Original comment by chen...@gmail.com on 21 Feb 2010 at 6:28

[GoogleCodeExporter]

Original comment by chen...@gmail.com on 23 Feb 2010 at 7:26

• Changed state: Fixed

[GoogleCodeExporter]

Original comment by chen...@gmail.com on 23 Feb 2010 at 7:29

• Changed state: Done

[GoogleCodeExporter]

Issue updated:
What steps will reproduce the problem?
1. Check out the newest version from svn
2. Do review in file BusDbAdapter

Expected:
The completed version of BusDbAdapter.java that should work properly and in 
good 
coding  stye

Actual:
There're a few errors in coding style needs to be pointed out in requested 
second 
code 
review

Original comment by QUANGHUY...@gmail.com on 14 Mar 2010 at 4:49