File APIs for Java Developers
Manipulate DOC, XLS, PPT, PDF and many others from your application.
http://aspose.com/file-tools
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic

Just finished my first java program :-) Can you please give me suggestions on making my code better?

 
Sebastion Hill
Ranch Hand
Posts: 48
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hey all, I am pretty excited because I just finished my first program. It's pretty basic but it is the first part of an on going personal project. It runs good and does what I was hoping it would but now I am curious as to which parts I could have done better on.

Just as a background note, I plan on making an application for my android phone. I typically wish that I could have a better planner (cause I am not that great at time management) so I thought I would make an application suited to my needs. This will be based upon the philosophy from guy who wrote 7 successful habits etc., Stephen Covey I think.

Anyways, this is the first part and I plan on making a GUI for this program after I get feedback on ways to improve the code. I broke the arrays into 4 different sections here because I plan on making them color coded in my GUI (e.g. blue for highest priority, maize for second highest, green third, and maybe red for the lowest priority).

But yeah, if anyone wouldn't mind providing feedback on ways that I can improve I would be EXTREMELY grateful. :-) My code is listed below:



and the activity class ...

 
David Newton
Author
Rancher
Posts: 12617
IntelliJ IDE Ruby
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'm not sure I would have made separate arrays for the priorities--most likely I would have simply made the priority a part of the object. It also seems to be recursive, which I'm pretty sure isn't your intent (for example, selectionScreen() calls prompt() which calls selectionScreen() etc. It'll take awhile, but eventually you'll run out of memory and crash.

The checkActivityArray() method seems to basically repeat large chunks of code. Any time you find yourself using "paste" as a programming paradigm it's time to re-think and re-factor.
 
Seetharaman Venkatasamy
Ranch Hand
Posts: 5575
Eclipse IDE Java Windows XP
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
1.Use LogWriter instead of System.out.println()


2.

Check method parameter before using them
 
Harshit Rastogi
Ranch Hand
Posts: 131
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator


Change this to


you should always use the interface class . Thats a good practice. Because if in case you want to change it to Vector simply change the implementation
 
Sebastion Hill
Ranch Hand
Posts: 48
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
David Newton wrote:I'm not sure I would have made separate arrays for the priorities--most likely I would have simply made the priority a part of the object. It also seems to be recursive, which I'm pretty sure isn't your intent (for example, selectionScreen() calls prompt() which calls selectionScreen() etc. It'll take awhile, but eventually you'll run out of memory and crash.

The checkActivityArray() method seems to basically repeat large chunks of code. Any time you find yourself using "paste" as a programming paradigm it's time to re-think and re-factor.


Thanks David. I originally thought about trying to make an array of arraylists for the priority parts but I didn't know if that was possible. I like your ideal to put the priority into the actual object, I think I'll take some time and think about how I can do that (still a newbie but it sounds good). Thanks again, I'll remember the "pasting code" you mentioned for future effors
 
Sebastion Hill
Ranch Hand
Posts: 48
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
seetharaman venkatasamy wrote:1.Use LogWriter instead of System.out.println()


2.

Check method parameter before using them


Hi Seetharaman, thanks for the feedback. I am curious as to the benefits of using LogWriter instead of System.out.println() ... the reason why I ask is that I've never heard of LogWriter and wanted to double check to make sure it's not personal preferences

Also, by check method parameter did you mean just add a try-catch or is there something else I should be doing?
 
Sebastion Hill
Ranch Hand
Posts: 48
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Harshit Rastogi wrote:

Change this to


you should always use the interface class . Thats a good practice. Because if in case you want to change it to Vector simply change the implementation


Hey Harshit, for my clarification ... I just need to change the first ArrayList to List ... due to the interface class? I'll read up on it more to get a better understanding but if you don't mind explaining it that would be great, because I am not really sure about your vector comment that follows. Thanks again
 
Rob Spoor
Sheriff
Pie
Posts: 20372
44
Chrome Eclipse IDE Java Windows
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Suppose you find that Priority4 gives you problems. You debug a bit, and find it is a threading issue - two threads are adding elements at the same time, and the ArrayList breaks. You think: I should synchronize the ArrayList! Big problem: now you have to change all points of access to the ArrayList. Auch!

Now, let's say you declared the field as List. Now all you need to do is change just one line of code:
Or, a method I prefer:
And voila! All access to Priority4 is now synchronized*.


* When using an Iterator you should still synchronize manually:
Still, much less code needs to be changed.
 
Fred Hamilton
Ranch Hand
Posts: 684
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'm gonna nitpick on something that doesn't really affect functionality, but may affect readability.

Give some thoughts to the names you give your Boolean variables and methods. IMO they should be more suggestive of yes or no answers or true/false answers

So instead of things like Boolean importance and Boolean urgency, you would have things like Boolean isImportant and Boolean isUrgent.

if( activity1.isImportant ) sounds better to me than if( activity1.importance ) , see what I mean?
 
Andrew Ceronoski
Greenhorn
Posts: 16
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Adding a GUI is really nice...but also a lot of work. For letting the user know they made an error try using a dialog box (real easy ).

Another idea is to use HTML to format your text. You often print multiple lines....you could use one "System.out.println" and use the html command \n to have a new line.

Neat program!
 
Campbell Ritchie
Sheriff
Pie
Posts: 47232
52
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I would only suggest using html tags in the documentation comments, which are supposed to be translated to html anyway. There are some Swing components which support html too.

The \n character is not an html tag, and since Java5 it has been far better to use the %n tag which obtains the platform-specific line separator than \n.
 
Sebastion Hill
Ranch Hand
Posts: 48
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Rob Prime wrote:Suppose you find that Priority4 gives you problems. You debug a bit, and find it is a threading issue - two threads are adding elements at the same time, and the ArrayList breaks. You think: I should synchronize the ArrayList! Big problem: now you have to change all points of access to the ArrayList. Auch!

Now, let's say you declared the field as List. Now all you need to do is change just one line of code:
Or, a method I prefer:
And voila! All access to Priority4 is now synchronized*.


* When using an Iterator you should still synchronize manually:
Still, much less code needs to be changed.


Thanks Rob. I will take you up on your suggestion and I will also research synchronized more to get a better understanding. I know what synchronized means in real world talk but a lot of these comments are going over my head but I am looking forward to understanding them all better! :-) thanks again
 
Sebastion Hill
Ranch Hand
Posts: 48
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Fred Hamilton wrote:I'm gonna nitpick on something that doesn't really affect functionality, but may affect readability.

Give some thoughts to the names you give your Boolean variables and methods. IMO they should be more suggestive of yes or no answers or true/false answers

So instead of things like Boolean importance and Boolean urgency, you would have things like Boolean isImportant and Boolean isUrgent.

if( activity1.isImportant ) sounds better to me than if( activity1.importance ) , see what I mean?


No worries, I was being lazy and I am glad you pointed this out. I need to make sure to develop good habits ;-)
 
Sebastion Hill
Ranch Hand
Posts: 48
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Campbell Ritchie wrote:I would only suggest using html tags in the documentation comments, which are supposed to be translated to html anyway. There are some Swing components which support html too.

The \n character is not an html tag, and since Java5 it has been far better to use the %n tag which obtains the platform-specific line separator than \n.


Andrew and Campbell, html is kind of over my head right now, though I plan on looking into it. Do I need to know HTML before learning Java or is it something that I can learn afterwards? Is html needed for the GUI?
 
Campbell Ritchie
Sheriff
Pie
Posts: 47232
52
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The only bit of Java which usually uses HTML is the documentation comments. You don't need HTML to write Java. You can write documentation comments without knowing any HTML. You can create 99.99% of any GUIs without knowing HTML.
If you wish to execute an applet you will need a 3-line HTML document, which you can more or less copy out of the books.

There are a few GUI components where you get a better-looking text display with HTML. Your documentation comments are converted into HTML, which looks better if you put a few tags in. It will take ages to learn the amount of HTML you would actually use for Java programming, at a rough estimate maybe half an hour .
 
Sebastion Hill
Ranch Hand
Posts: 48
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Campbell Ritchie wrote:The only bit of Java which usually uses HTML is the documentation comments. You don't need HTML to write Java. You can write documentation comments without knowing any HTML. You can create 99.99% of any GUIs without knowing HTML.
If you wish to execute an applet you will need a 3-line HTML document, which you can more or less copy out of the books.

There are a few GUI components where you get a better-looking text display with HTML. Your documentation comments are converted into HTML, which looks better if you put a few tags in. It will take ages to learn the amount of HTML you would actually use for Java programming, at a rough estimate maybe half an hour .


Lol, I think that is a fair trade-off then. Thanks Campbell
 
Campbell Ritchie
Sheriff
Pie
Posts: 47232
52
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
You're welcome

And for the % tags, try this Java™ Tutorials section, which is brief but easy to understand, or the Formatter class, which is comprehensive but difficult to understand. Or there might be something in your book.
 
Sebastion Hill
Ranch Hand
Posts: 48
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
So I have done some updates, mainly trying to use the suggestions I have received from forum members. I felt like an idiot after thinking about how I could eliminate the repeat code for the arrays ... lol, it finally hit me that I only needed 1 array (so this eliminated the need for synchronization, though I still plan on learning more about it).

Also, I updated my boolean expressions to make them better in terms of syntax and I also spent some time learning tags using System.out.format (again, VERY useful though I still have lots to learn).

I also added an automatic save feature as well as the option to delete activities (since they are now being saved and will start to add up).

I think I have still failed in regards to the recursive issues, any suggestions on fixing that? :-/

Well here it is, the updated code ...



 
Sebastion Hill
Ranch Hand
Posts: 48
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
If no one sees any major problems with this code then my GUI work will start on Monday :-)
 
With a little knowledge, a cast iron skillet is non-stick and lasts a lifetime.
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic