Small issues before 0.2

[Sean Owen <sr...@gmail.com>]

Going through the code one more time before starting release for 0.2
and I have noticed some possible issues -- or else I miss something.


FPGrowth:258 : is this for loop really supposed to declare one
variable but update another? this could be correct but reads as
confusing

LuceneIterable, SequenceFileIterable. The Iterators here have the same
problem - hasNext() can't have side effects. next() is what does the
updating. In these implementations hasNext() is what advances the
underlying data structure. This isn't going to work in general.

(In fact, might be good to look for "TODO" in the code where I have
over time flagged things that look like a possible bug to me, mostly
stuff that is updated but never read or something like that. Some may
well not be; just remove the TODO then.)


Also let's not call System.exit()? it's abrupt to the point of not
necessarily running finalizers, which might be a bad thing. The return
value of main() rarely matters, and, throwing an exception is a more
conventional way to indicate failure and generate a non-zero return
value.

Also I find it confusing to give a generic parameter a name longer
than 1-2 characters. It looks like a class name.

and from the past ...

No * imports
Don't throw RuntimeException or Exception
CONSTANTS should be final
No serialVersionUID, it breaks things
Conventional modifier order is 'public static final'
Conventional order of elements of class file: statics, members,
constructor, methods, inner classes
Still lots of javadoc problems -- bad refs, empty elements

it's small, but good to follow a standard coding convention, and keep
the code clean.

Re: Small issues before 0.2

[Sean Owen <sr...@gmail.com>]

 Right now it's a for loop on i, but, the for loop also declares an
apparent index variable called j, which is not the index. A for loop
ought to be used when the index variable begins life at the start of
the loop, and of course the index variable declared in the for syntax
ought be the actual index variable of the loop. For those two reasons
it seems clearer to unpack this as a while loop and rename j.

I'm not going to block on the Iterator issues but those really ought
to be fixed.

On Tue, Oct 20, 2009 at 11:05 AM, Robin Anil <ro...@gmail.com> wrote:
>> i is being updated not j, the initial location is selected by a binary
> search(4 lines above).  And the function continues only if search returns
> correct.
>

Re: Small issues before 0.2

[Robin Anil <ro...@gmail.com>]

On Tue, Oct 20, 2009 at 2:19 PM, Sean Owen <sr...@gmail.com> wrote:

> Going through the code one more time before starting release for 0.2
> and I have noticed some possible issues -- or else I miss something.
>
>
> FPGrowth:258 : is this for loop really supposed to declare one
> variable but update another? this could be correct but reads as
> confusing
>
> i is being updated not j, the initial location is selected by a binary
search(4 lines above).  And the function continues only if search returns
correct.



 LuceneIterable, SequenceFileIterable. The Iterators here have the same
> problem - hasNext() can't have side effects. next() is what does the
> updating. In these implementations hasNext() is what advances the
> underlying data structure. This isn't going to work in general.
>
> (In fact, might be good to look for "TODO" in the code where I have
> over time flagged things that look like a possible bug to me, mostly
> stuff that is updated but never read or something like that. Some may
> well not be; just remove the TODO then.)
>
>
> Also let's not call System.exit()? it's abrupt to the point of not
> necessarily running finalizers, which might be a bad thing. The return
> value of main() rarely matters, and, throwing an exception is a more
> conventional way to indicate failure and generate a non-zero return
> value.
>
> Also I find it confusing to give a generic parameter a name longer
> than 1-2 characters. It looks like a class name.
>
> and from the past ...
>
> No * imports
> Don't throw RuntimeException or Exception
> CONSTANTS should be final
> No serialVersionUID, it breaks things
> Conventional modifier order is 'public static final'
> Conventional order of elements of class file: statics, members,
> constructor, methods, inner classes
> Still lots of javadoc problems -- bad refs, empty elements
>
> it's small, but good to follow a standard coding convention, and keep
> the code clean.
>