GoogleCodeExporter
commented
9 years ago CODE REVIEW - Bus Stop Alarm
Date: Feb 19, 2010
File: BusDbAdapter.java
Revision: 109
Author: Huy Dang
Reviewer: Derek Cheng
Summary:
Second review of BusDbAdapter.java. Suggestions from first review has been
incorporated into code. Added methods for interacting with the database. This
review
will examine the changes made for the most part, and will focus on code
formatting,
adherence to Java / Android style guidelines, and adherence to object-oriented
principles.
Comments:
CODING STANDARDS
FileInputStream is never used in the code - remember to remove it if it won't
be used
in the future.
In the database query statements, have one space between the name of the
statement
and the = sign.
When initializing String arrays (or any other types of arrays), you can use the
shorthand notation without the "new String[]", e.g. you can write String[] args
=
{route_id, route_desc, .......}; in line 225, and other places.
In line 207, fix the spacing between the ")" and "{" in the declaration of
createDest(). There are other methods that have the same problem so make sure
to
check that.
In line 212-213, when you only have 1 line in an if block, please put braces
around
that single statement anyway, in order to adhere to Android guidelines.
There are some spacing issues in deleteDest() in the "catch" line, "finally"
line,
and "return" lines.
There is a minor spacing problem in line 367.
Minor spacing issues in line 561-565.
COMMENTS
Comment at top with author information so others would know who wrote this
file. Move
the file description comment before the import statements.
When documenting methods, use third person perspective consistently. ("opens"
vs.
"open")
There are two types in the comment in line 230.
LOGIC
In readDbFile() you have a while-true loop that breaks when the resource has
been
read through the end. Instead of using while-true, you can write "while
((line=bin.readLine()) != null) { ... }" as that would be much nicer and
potentially
not get stuck in an infinite loop for whatever reason.
ERROR HANDLING
deleteDest() - I assume that you are going to put in logging inside the catch
block,
so this is just to remind you that it should be implemented, preferably as soon
as
possible since it is generally a bad idea to leave catch an exception without
doing
anything. It might also be good to notify the user that the transaction was
unsuccessful.
CODE DECISION
In line 469-470 in getDestColumn(), mCursor is set to null, then immediately
set to
something else in the next line. The initial setting to null is redundant
unless it
is for detecting some sort of error (in which case, the statements should be in
a
try-catch block?) (also in getListDest())
As you mentioned in readDbFile(), getDestTbSize() is a helper method. Would it
be
better if we made that method private instead of public?
Original comment by chen...@gmail.com on 21 Feb 2010 at 7:21
Original issue reported on code.google.com by
QUANGHUY...@gmail.comon 19 Feb 2010 at 5:07