🚀 go-pugleaf

Well, most of this is prototyping - basicaly learning by doing, so don't
worry - I have no intentions of doing stuff like "talk to strangers" etc.
Most of this stuff was basicaly made to function with pictures stored on the
webserver, but that won't work, when they're stored in a jar-file, so I just
made the variables public instead of using set/get

Basically this goes for just about every "no no" you mention, including the
lack of a code-standard.

There's no need to nitpick on those, since basicaly everything will be
rewritten using "good" code.

Anyways, on to the good stuff ;-)

"Jon Skeet" <skeet@pobox.com> skrev i en meddelelse
news:MPG.171f759e31230a3398af97@dnews.peramon.com...
> Martin Schou <Martin.Schou@Grue-Hornstrup.dk> wrote:

> >         byte[] buffer = new byte[in.available()];
> >         in.read(buffer);
>
> Both of these lines are bad. Firstly, you don't know that in.available()
> is going to return the whole file, and secondly, in.read() isn't
> guaranteed to read for a whole buffer. You're ignoring its return value.
>
> See IOUtil at http://www.pobox.com/~skeet/java/skeetutil for some code
> to read a stream completely.
>
> Note that you're also not closing the input stream. That should be done
> within a finally block.

Hmm, hadn't really thought about that. I'll give that a try, 'cause it
sounds plausible

> >     catch( NullPointerException E )
> >       {
> >         E.printStackTrace();
> >       }
>
> Catching NullPointerException is very rarely a good idea

When trying to prototype my way through a problem, it probably is; I need to
know _why_ the NullPointer is thrown, so that I can avoid it. Ie, when an
Image fails to load, and I return it to another class, if I don't catch the
exception, I'm screwed - but you knew that :-)

> > public void Send( String Message, boolean EndLine )
> >   {
> >     byte[] message = Message.getBytes();
>
> Never use String.getBytes() - use String.getBytes (String), ie
> specifying an encoding. Use the same encoding as the server will
> understand.

Did not know that. Will do that from now on.

> >     catch( Exception E )
> >       {
> >         E.printStackTrace();
> >       }
> >   }
>
> Catching bare Exception is almost never a good thing to do - if you want
> to catch everything, use Throwable. If you don't, catch specific
> exceptions.

See NullPointerException above

> > public void SendEndLine()
> >   {
> >     try
> >       {
> >         byte cariageReturn = new String( "\r" ).getBytes()[0];
>
> new String (String) is almost never required, and certainly isn't
> required here. Why not just use DataOut.writeByte ('\r'); in the first
> place though?
>
> > public byte[] ReadAvailable()
> >   {
> >     byte[] tempByteArray = null;
> >     try
> >       {
> >         int numberOfBytes = DataIn.available();
> >         tempByteArray = new byte[ numberOfBytes ];
> >         DataIn.read( tempByteArray, 0, numberOfBytes );
> >       }
> >     catch( IOException E )
> >       {
> >         Error( E.getMessage() );
> >     Sleep( 100 );
> >     return tempByteArray;
> >   }
>
> Aside from the problems I mentioned earlier, why do you have a call to
> sleep here?

I think it's a leftover, from when I was using a continuous loop, and I
didn't want to hog the cpu. In this piece of code, it's a couple of bytes
that I could use later (I'm working on a VERY restricted environment, with
256 kilobytes for EVERYTHING).

>
> > public void ConnectM�ler()
> >   {
> >     String tempStorage = "";
>
> You never seem to use this variable.

Leftover from earlier. Will be scrapped in final edition, but thank you for
pointing it out.

> >     tempStorage = "";
> >     M�ler.Send( "login", true );//not true login
> >     /* Still "fast forwarding" */
> >
> >     while( M�ler.ReadAvailable().length > 0 )
> >       {
> >         M�ler.Sleep( 10 );
> >       }
>
> Why are you ignoring what's coming back?

I'm "fastforwarding". I already _know_ what's comming back, "Password:", so
there's no need to analyze it.

> >     tempStorage = "";
> >     tempStorage = null;
>
> These two lines are unnecessary, even if tempStorage was being used
> somewhere.

That is correct. If I use tempStorage and move its contents to some other
variable, I will probably be screwed big time. Took me two weeks to track
that down, when I did it to an array. Thanks!

> --
> Jon Skeet - <skeet@pobox.com>
> http://www.pobox.com/~skeet/

Thank you for the usefull input, even the things I knew in advance - this is
after all supposed to be a help to others, with similar problems, and if
they can learn from my mistakes - good for them.

Martin Schou