Need Experienced Java Programmer to assess a small project source.

[CSloan7597]

Hey everyone.

I have recently decided to try and learn how to use java Sockets for client/server communication, as well as swing and threads, etc. and have (pretty much) completed a small client server application.

The application is basically a syntax-highlighting notepad, a client swing GUI which connects to a (multi-threaded) server which handles data being sent to each client.

The main function of the program is turn-based code editing:

1. Client 1 Connects to Server
2. Client 2 Connects to Server
3. Client 1 requests Control of the program Lock
4. Client 1 edits the source, and then clicks "send data"
5. On sending, client 1 relinquishes the lock, and the new data is sent to every other connected client
6. Client 2 requests control, edits, etc.

The server takes connections, then creates a new thread to deal with each. The thread will read and write from/to the Clients. All threads of this type will be passed the shared "Lock" object.

There is more functionality I could add, like a notification that the lock is free, or a waiting queue, but I am more concerned with the possibility that the code I have produced is bad practice, or not structured very well.

I am not a new programmer, but I am still very inexperienced as i have only learnt in bits through academia, and am only now starting to pursue private projects.

I would really appreciate some constructive criticism, and my main areas of concern are:

• The way Messages are passed between Client/Server (Lack of proper protocol)
• Threading (poor use of synchronized methods)
• General Code Structure/practice
• Incorrect use of Sockets and Socket I/O

Be gentle, this is my first ever completed project :')

Source Files:

Client:
- clientGUI.java

Server:

- server.java
- lock.java
- clientServiceThread.java
- readerThread.java

[JamesCherrill]

I just had a really quick look and my immediate impression is of good quality code. Variable & method names, plus relevant comments make it easy to see what's what. Lots of runtime tracing to confirm good behaviour (maybe an idea to use a simple Logging class rather than System.out so you turn them all on/off easily?).
My only area of question is re the lock (should be Lock!) class where you seem to have home-brewed a pile of functionality that's already available in a fully-designed fully-debugged bullet-proof form in the existing Java SE API? I'm not the best person to take that any further but maybe someone who's more of a Locks & Threading guru could comment?
But, in summary, there's nothing in this that screams "inexperienced" to me.

[CSloan7597]

James, thanks for the quick reply :)

Wow, I didn't expect the first comment to be so positive! I hadn't come across the standard Lock stuff, i'll definitely look into that and make some changes ASAP, as well as creating a logging class.

Thanks again, i'll post any updated code on here once i've made some changes.

[JamesCherrill]

Your code is short enough that you could post it directly into the thread (inside [CODE} tags of course) then it will be easier for more people to read it.

Here's a good place to start on the "standard lock stuff"
http://download.oracle.com/javase/tutorial/essential/concurrency/highlevel.html

[CSloan7597]

Ahh i suppose i could, i only didn't because there were a few classes there.
Oh i can't edit my post anymore, do you think it's worth posting the code as a reply down here? :\

Thanks for the link aswell, currently reading :)

[JamesCherrill]

No, I wouldn't re-post the existing code now, but if, in the future, you have revised stuff to share...

[JamesCherrill]

.. and a few notes on the GUI specifically:

You don't need all those this. qualifications on your variables and methods.

Your menus are perfectly sensible for this size of app, but as things grow you will start to get more & more complex - eg if you duplicate any menu items by adding a toolbar, or some right-click context menus. If you are considering going that way it's better to use Actions ( http://download.oracle.com/javase/tutorial/uiswing/misc/action.html ). Even if you don't but you do add more menus, the use of a single listener (this) with a load of if (source is A) else if (source is B) else if ... gets really untidy and hard to read. Things scale better with one listener per menu item, using an inner class. Personally I prefer the anonymous inner class version.

You have a number of methods that throw Exception rather than catching it. In general this isn't a good idea. When you get an E you want to display the stack trace, plus as many relevant variables as may help diagnose the problem. If you throw the E up to the calling method you lose much of the context in which the E was originally thrown, which makes it hard to understand. More seriously you lose the ability to have a finally clause that tidies up after the error.
If you want to signal a problem to a calling method you should still catch it where it happens, print everything relevant, then throw an application Exception up to the caller with a user-friendly text - maybe an example...

void myMethod throws Exception {
  try {
     do some file I/O stuff
  } catch(IOException e) {
     e.printStackTrace();
     System,err.println(file details etc etc)
     throw new Exception("There was an error reading the file - please check the file exists and you have permission to access it", e);
  } finally {
     tidy up (close files etc)
  }
}

[CSloan7597]

Okay, erm, if I did implement try catch within the methods, could I not just remove "throws" and print the stacktrace and message there and then, still having a finally block?

I think I will use the actions advice, that seems much cleaner, I do think my actionperformed method is ugly as hell.
This is really helpful for me so if you don't mind keep it coming :)

[JamesCherrill]

Yes. There's no requirement for you to throw a new E if you have already caught the original one. The reason I included it was that the calling method probably needs to know that something has gone wrong, and maybe issue a user error message in some format appropriate to the user interface of the application. If you don't do something to tell the calling method there was a problem then it will continue executing and probably everything will fall apart in a horrible mess. If your method returns an Object, then you can signal the error by returning null (assuming that's not a valid return value) instead of throwing a new E.

[CSloan7597]

Okay yeah that makes sense, I think I would throw one actually.