Asking for a critique of first Java application code

[Jim Rosetti]

Hey everybody,

I'm not exactly a "programming greenhorn", but I'm learning JAVA and I'm looking for some feedback. I've been doing (mostly embedded) C/C++ for quite a while, but I found some things about JAVA different and I'm assuming there's "standard ways of doing things" in JAVA that I don't know about. So, my intent is to learn now what I'd figure out myself after a few years (accelerate the learning process).

I'd like to mention a few things to help point you in the right direction. I struggled a bit with sharing code between projects--had to create a shared jar file. I think it's a peculiarity of netbeans, but if it's not, please let me know. I also noticed more than one way to accomplish things, especially synchronization with containers--probably because of enhancements to the language over the years. So, if I did something "old school", I'd like to know about it. Oh, I almost forgot--I played around with using #import vs. fully qualifying things, so you'll see a mix. I still haven't made up my mind on that one.

I created a client server application and am making it open source. The client and server are functional and I'm using them every day on my htpc, but I haven't finished testing all the features yet. Similarly, there's some documentation, but it's not done yet. However, I did declare the javadoc stuff complete, so if you think there's an issue (too spartan perhaps?), please mention it. The link to the project is: http://sourceforge.net/p/aumus/code/HEAD/tree/trunk/" target="_blank" rel="nofollow">My code repository

I know this is a fair amount of code. Please don't think I'm asking you to look at every line. Again, I'm assuming the basics are OK ,but the JAVA specific stuff might need some improvements.


Thanks much,


Jim

[Mike. J. Thompson]

Hi Jim, welcome to the Ranch.

Something strange seems to have happened to your link. The visible text is correct but the link href contains an opening anchor tag. I managed to get to your code by copying and pasting the text. Did you use the URL button to create it?

Anyway i'll add my observations about your code here as I read it. I'll submit it as I go so may come back and add more later.

* By convention, method names should start with a lower case letter, and then be camel case thereafter.

* By convention, variable and field names should be as method names above.

*By convention, only Classes, Interfaces and Enums should start with a capital letter.

[Bear Bibeault]

And. speaking of capitalization: it's Java, not JAVA; it's not an acronym.

[Mike. J. Thompson]

* Your ConfigItem is written (and used) like it is a C/C++ Struct. All of the fields are public and so directly accessible and modifiable. While this is perfectly valid Java, it breaks encapsulation and is not best practice or good Object Oriented programming.

* Your ListenerThread has a race condition in it. The run() method accesses m_commandHandler, but that variable is not intialised until after the thread is started. If the run method reaches the line that dereferences the m_commandHandler variable before the constructor initialises it then you will get a NullPointerException.

* Your ListenerThread is not thread safe (other than just the race condition mentioned above). You have multiple threads accessing the same variable with no synchronization. This is not a beginner topic, but basically something that one thread does is not guaranteed to be 'seen' by any other thread in the system unless there is a 'happens-before' relationship. Take a look at this Oracle tutorial

[Jim Rosetti]

Hey everybody,

Thanks for the feedback! I skimmed everything, but I'm really busy, so I'll respond to posts as time permits.

I'm pretty sure I used the URL button to put that link in. I'll see if I can edit the original post and retry.

I didn't know this language had a convention on naming. I searched and found the "Code Conventions for the Java Programming Language" document. I'll make appropriate changes after I review it.

Funny--I have JAVA (not Java) all over the place. I just renamed a folder on my PC and found all caps in some comments (I'll correct later).

Mike--thanks for the heads up on the race! Oops.

That's all I have time for now. I'll get back to the other comments soon.

Thanks again!

[Jim Rosetti]

Well, I'm done making those changes and I looked over the remainder of the feedback...

Yes, ConfigItems is like a struct and it isn't the best oop practice, BUT in my opinion, adding a bunch of setters and getters to something this simple makes it cumbersome and adds little value. I look at things from a business model and I think my colleagues would shoot me if I did that. Now if that class was something an external customer was using, then it'd be a different story--we'd want to do range checking when setting a value.

Finally, concerning ListenerThread thread safety--I may have missed some synchronization with some public Player methods (will look at that closely) and definitely missed thread safety concerning the events/callbacks (onNewSong, onSongTimeChange). Thanks!

[Jim Rosetti]

I tried to insert the url again: My Java code repository

It looks correct now. I think I did it the same way as before. Oh well--problem solved.


Thanks.

[Mike. J. Thompson]

The link works fine now.