Stan Sieler
2009-06-05
I frequently receive emails from strangers (students), asking:
Hi I really need help fast, and i need it back as soon
as you can get it back PLEASE.
All i need is for you to highlight the mistakes and correct them
and line them by pseudo code if you know how. Please I'm relying on you.
I'm desperate :/ please help
You REALLY need to say *WHAT THE HECK IS WRONG* when you ask for help!
I, and other people you ask for help, have a life ... and we don't need
to spend our time doing the job of the compiler (e.g., trying to spot
syntax errors if your program won't compile) or doing the job of guessing
where the program might not work the way you intended it to work ...
particularly when we don't know what you intended!
Said differently, there is absolutely nothing wrong with your program ...
if it's doing what you intended.
If it's doing something else, THEN WHAT THE HECK IS IT DOING?
Why should I have to GUESS what it's doing? TELL ME!
Here are some good examples of asking for help:
- Please look at my source code and comment on structure, variable naming,
and anything else you might feel is appropriate to comment on.
It's written in X, using the Y compiler from Z, running on a Q computer.
- My program won't compile. Line 23 is flagged as "unknown variable", but
that's puzzling, because it appears to be declared at line 10 (lines 1..23
included in the email).
- My program is aborting at line 34, with "segmentation fault" when it
tries to access "ptr". Can you shed any light on why this happens?
I thought I had all paths covered, so there was no way to reach
that line with "ptr" being bad.
- My program compiles and runs, but when it calculates the profit/loss
for a make-believe business, it gets the wrong answers (they're all too big
by a factor of exactly 100). Can you suggest why that might be?
(Yes, probably treating pennies as dollars, which is a factor of 100.)
- Can you comment on the tradeoff between readability, performance, and
testability for these two snippets of code?
In almost all cases, NEVER ASK FOR HELP IF THE PROGRAM DOES NOT COMPILE!
The compiler can check for syntax errors much faster and much easier than
any human can ... let it do its job first!
I received some code today with the above (indented) request for help:
} else if (splitted[0].equals("@sellequip")) {
try {
if (splitted.length < 2 | Integer.parseInt(splitted[1])<=0|
Integer.parseInt(splitted[2])<=0) {
player.message(Incorrect syntax. The format is @sellequip
itemid
quantity.)
} else {
try {
PreparedStatement
ps = DatabaseConnection.getConnection().prepareStatement("SELECT
shoptiems WHERE itemid = ?");
ps.setDouble(0, Double.parseDouble(splitted[1]));
ResultSet rs = ps1.executeQuery();
ps
= DatabaseConnection.getConnection().prepareStatement("SELECT
inventoryitems WHERE itemid = ? AND WHERE characterid = ?");
ps.setDouble(1, Double.parseDouble(splitte
Yes...formatted that badly, and starting with "} else", and trailing
off at the end.
In this case, the experienced programmer should see MANY problems ...
but the code might even "work", sort of. (I have no idea if the compiler,
whatever one it was, would accept it as is.)
Let's reformat the code and look at it:
1.else if (splitted [0].equals ("@sellequip"))
2. {
3. try
4. {
5. if (splitted.length < 2 | Integer.parseInt (splitted [1]) <= 0 |
6. Integer.parseInt (splitted [2]) <= 0)
7. player.message (Incorrect syntax.
The format is @sellequip itemid quantity.)
8.
9. else
10. {
11. try
12. {
13. PreparedStatement ps = DatabaseConnection.getConnection ().
prepareStatement ("SELECT shoptiems WHERE itemid = ?");
14.
15. ps.setDouble (0, Double.parseDouble (splitted [1]));
16.
17. ResultSet rs = ps1.executeQuery ();
18.
19. ps = DatabaseConnection.getConnection ().
prepareStatement ("SELECT inventoryitems WHERE itemid = ? AND
WHERE characterid = ?");
20.
21. ps.setDouble (1, Double.parseDouble (splitte
(NOTE: the following is using pseudo-code, somewhat Java/Pascal inspired.)
I put line numbers at the start of each line above, so we could refer to them
in the comments below.
I left some lines as LONG lines, figuring your email reader could just scroll.
(Yes, I put a blank in front of all "(" and "[" ... JUST LIKE IN ENGLISH,
DARN IT!)
(Yes, I put "{" and "}" on a separate line, because THEY'RE NOT PART OF THE
PRECEDING OR FOLLOWING STATEMENT!) (And, I differ from most people in
the world by doing that. I also program better than most people.
Perhaps the two things are related? :)
Presumably "splitted" is a object or record structure:
splitted ... record structure with:
length (int32) ... the number of tokens on the line (although there's
some indication it's the highest index into the
splitted array?)
splitted (array of strings) ... an array (0-based) of strings,
probably white-space delimited tokens from the
input line.
Problems:
- no quotes on message in line 7.
Should be something like:
player.message
("Incorrect syntax. The format is @sellequip itemid quantity.")
Did you even TRY to compile this?
- Line 5 is asking for an abort/trap/escape by "illegal index".
If a single item is in the array, "splitted.length" will be 1, and
the attempt to access "splitted [1]" will trap (because only "splitted [0]"
is valid)
It should probably be "if (splitted.length < 3
..."
(I'm assuming the programmer wants three things on the input line:
the string "@sellequip" and two numbers ... in that case, length will be
3.)
- No provision for handling unwanted extra data
There's no check for splitted.length
< 3
ALWAYS notice and handle extraneous data ... even if you just say you're
ignoring it. If the user thought it was important to enter, they darn
well don't want you to quietly ignore it!
- Use of constants 0, 1, and 2.
Ugh...although they may work, they make maintenance and readability of
the code much harder.
Imagine:
const num_args_for_sellequip = 3; // @sellequip, itemid, quantity
const arg_inx_command = 0; // splitted [0] = "@sellequip"
const arg_inx_itemid = 1; // splitted [1] = item id (> 0)
const arg_inx_itemid = 2; // splitted [2] = quantity (> 0)
...
if (splitted.length < num_args_for_sellequip)
fail ("Sorry, the @sellequip command requires more arguments " +
"(a total of (" + num_to_str (num_args_for_sellequip) +
")");
if (splitted.length > num_args_for_sellequip)
fail ("Sorry, the @sellequip command requires fewer arguments " +
"(a total of (" + num_to_str (num_args_for_sellequip) +
")");
// ok, we have the exact number of arguments we want...
if (Integer.parseInt (splitted [arg_inx_itemid]) <= 0)
fail ("Sorry, the itemid must be a positive number");
...
(We'll talk about invalid integers in a bit)
- Overuse of parseInt
The first time you "parse" one of the tokens as an integer, SAVE THE
RESULT:
int itemid = Integer.parseInt (splitted [arg_inx_itemid]);
if (itemid <= 0)
fail ("Sorry, the itemid must be a positive number");
...
ps.setDouble (1, (double) itemid);
- Is "splitted [1]" an integer or a floating point number?
We see both: parseDouble(splitted[1])) and parseInt(splitted[1])!
- Error handling
The error handling is very poor. Assuming the program catches any
exceptions thrown by parseInt, the user won't be able to tell
if the program is complaining about the first int, the second int,
or some other trap!
I suggest having a separate integer parser... pseudo-code:
function parse_integer (the_input : string;
var the_output : int) : good_failed;
// returns 'good' (and a value in the_output)
// OR
// returns 'failed' (and the_output is unchanged) *AND*
// displays an error message
try
{
int temp = integer.parsedInt (the_input);
the_output = temp;
return good;
}
catch
{
display ("Sorry, bad number: " + the_input);
return failed;
}
and then use it:
if (parse_integer (splitted [arg_inx_units], num_units) = failed)
fail ("Expected # of units")
if (parse_integer (splitted [arg_inx_cost], unit_cost) = failed)
fail ("Expected cost-per-unit")
That can obviously be extended:
function parse_integer (the_input : string;
var the_output : int;
min_value : int;
max_value : int) : good_failed;
which checks that the value is a legal integer *and* within a desired
range.
Stan