• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Jeanne Boyarsky
  • Ron McLeod
  • Paul Clapham
  • Liutauras Vilda
Sheriffs:
  • paul wheaton
  • Rob Spoor
  • Devaka Cooray
Saloon Keepers:
  • Stephan van Hulst
  • Tim Holloway
  • Carey Brown
  • Frits Walraven
  • Tim Moores
Bartenders:
  • Mikalai Zaikin

Just finished my first GUI program (a planner application) ... could you give me feedback?

 
Ranch Hand
Posts: 48
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Since I'm not in any kind of java programming class I can't really get graded on my projects to see if I did things well or not. I was hoping you, the community, could help me out and give me a grade and feedback on my application.

I'm still new to OO so I probably could have done a few things differently in regards to being more efficient with my coding. I also have a few runtime errors that don't really affect my program but I'd like to understand why they are there and how I can fix them.

If you wouldn't mind reposting bits of code with suggestions/improvement/comments/etc. that would be great so that I can visually see what you are talking about. I often have to write down my thoughts since I am not very adept at seeing code in my head like most people on the forum. Thanks in advance :-)




 
Bartender
Posts: 1561
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
This looks pretty good, and it's impressive that you're taking on a large and somewhat complex task. A couple of suggestions and points from a non-professional hobbiest, for what it's worth:

Activity.java:
* The priorityCode int field is always calculated based on two other fields, activityUrgency and activityImportance. What if the two properties that are used to calculate priority change but the setPriorityCode() accidently isn't called? What happens is that priorityCode will be out of sync with the object. To prevent this from happening, I'd not make it a field but instead a method that is calculated on the fly and only when it is needed.
* Consider having your Activity class implement the Comparable<Activity> interface and use the priorityCode and perhaps date results as the basis of the sort. <edit: I see that you are using a Comparator object for this purpose in your GUI section, good, but I'd be inclined to consider this a "natural" ordering of the class and would use Comparable on the class instead./>
* I'm a bit concerned with the two Object fields, activityDateValue and activityTimeValue. You should avoid using Object variables if possible as this goes against OOP principles of objects that are responsible for their own data and behavior. What behaviors do these entities have? Do you want to use java.util.Date or a Calendar object here instead?

PlannerApp2.java (The GUI)
* There's a lot of code there, so I can't say that I've gone through it all. Sorry.
* Kudos to you -- you're creating your GUI by hand, not via a code-generator.
* Kudos again -- you don't have your main GUI class subclass a JFrame or other GUI component.
* Bummer: you use GridBagLayout quite a bit, and I HATE GridBagLayout. I'll get over it, I suppose.
* PlannerApp2 is a very large class, perhaps too large, but I do see that a lot of it has been broken down into inner classes, which is good since divide and conquer is a very good strategy when dealing with a large and complex project. You may want to make some of these classes stand-alone and try to "de-couple" them as much as possible which is where interfaces may help. It can make debugging and enhancing easier.
* You may want to incorporate more MVC concepts in your code including getting the core model -- the list of activities -- out of the GUI and also extract out file IO as well.

Good luck and congrats on getting such a large program off the ground!
Pete
 
Ranch Hand
Posts: 686
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I see you chose GridBagLayout for your first GUI, that certainly is a challenge, it is probably the most difficult to learn and implement, but if you can master it, it is pretty powerful. I can't remember if I mentioned this before, but this page is really good for learning various layouts.

http://java.sun.com/developer/onlineTraining/GUI/AWTLayoutMgr/shortcourse.html

Also, I see you have a number of JPanels for various sections of the GUI, but it wasn't clear to me if you had one highlevel JPanel for all the 'sections', or if you laid out the sections directly in the contentPane of the JFrame. It looks like the second method, but I am not sure. One advantage of adding all the sections to a JPanel, then setting the single JPanel to the contentPane of the JFrame, is that you can just as easily add the single JPanel to a JApplet.

Anyways, if you could clarify the high level layout of your GUI, that would be helpful.

ps Pete beat me to it, looks like he is more adept than I at figuring out what is going on.
 
pete stein
Bartender
Posts: 1561
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Fred Hamilton wrote:Also, I see you have a number of JPanels for various sections of the GUI, but it wasn't clear to me if you had one highlevel JPanel for all the 'sections', or if you laid out the sections directly in the contentPane of the JFrame. It looks like the second method, but I am not sure. One advantage of adding all the sections to a JPanel, then setting the single JPanel to the contentPane of the JFrame, is that you can just as easily add the single JPanel to a JApplet.


Hey Fred, it looks like he's creating a JTabbedPane as the main JComponent and adding it to the contentPane via the method addComponentToPane(Container pane).

ps Pete beat me to it, looks like he is more adept than I at figuring out what is going on.


I'm certainly not sure about that, but perhaps I'm just as or more opinionated than you. ;) Good to see you.
 
Sebastion Hill
Ranch Hand
Posts: 48
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Thank you Pete and thank you Fred :-) I need to sit down and digest what you two had to say and see if I can figure out a way to address those things but I definitely appreciate your feedback and hope that I can do those things you suggested more naturally after I have programmed some more. Will be back with more questions for clarification though :-D thanks again
 
Fred Hamilton
Ranch Hand
Posts: 686
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

pete stein wrote:

Fred Hamilton wrote:Also, I see you have a number of JPanels for various sections of the GUI, but it wasn't clear to me if you had one highlevel JPanel for all the 'sections', or if you laid out the sections directly in the contentPane of the JFrame. It looks like the second method, but I am not sure. One advantage of adding all the sections to a JPanel, then setting the single JPanel to the contentPane of the JFrame, is that you can just as easily add the single JPanel to a JApplet.


Hey Fred, it looks like he's creating a JTabbedPane as the main JComponent and adding it to the contentPane via the method addComponentToPane(Container pane).

ps Pete beat me to it, looks like he is more adept than I at figuring out what is going on.


I'm certainly not sure about that, but perhaps I'm just as or more opinionated than you. ;) Good to see you.



Well, I'm not too familiar with JTabbedPane, but I would imagine then that it is a single entity that houses the entire GUI, and since it is being added to the contentPane of a JPanel, that it can just as easily be added to a JApplet. Anyways, it all points to a strategy of mine, whenever practical I like my high level container class to be just a shell, for the reason I described, so I can easily drop my GUI in one piece into a JApplet.
 
Rancher
Posts: 3324
32
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
1) A big nitpick with me is readability of the code. One of the easiest ways to make code readable is to be consistent. Blocks of code should be indented by the same amount every time. You appear to use 1, 2, 3, 4, 5, 7, 10 ... or whatever you feel like.

2) I'll jump on the (anti) GridBagLayout bandwagon as well. Not so much because you used it but because it wasn't used properly. Try resizing the frame to see how the layout changes. All the text fields, spinner and buttons resize to unnatural large sizes as the frame size it increased. Layouts should grow gracefully. This generally means that text fields and spinners and buttons "should not" grow. Only components like scroll panes with tables, text areas etc should grow. By using other layout managers it will be much easier to control this.

Of course another alternative it to make popup dialogs non-resizeable, but I believe the proper design it to use the layout managers effectively first.

3) I don't think the "Create New Activity" should be a tab. Generally when you want more data from a user you use a popup dialog. Another advantage of this approach is that you only need a single form to create and/or maintain the data. Right now you have duplicated code because you didn't use a shared panel to enter/change the Activity information.

4) A picky comment about the 3 buttons on the "Current List of Activities". They take up way too much space. You should either have all 3 button in one row, or if you want them displayed vertically, then they should be in a vertical column to the right of the tabbed pane.

5) The "Delete" button should have a "Cancel" button so you can change your mind.

6) Generally a "Description" type field would be a text area to allow the user more room to provide a detailed description.

7) Also, I'm sure you packaged all the code in one class to make it easy for us to test, but you really should have multiple source files. That is another reason why using popup dialogs to gather information is a good idea. It allows you to design the dialog and its data in a separate source file, shrinking the size of the main class a great deal.

Overall its a good start, but I always say the best way to learn is to copy the LAF of existing applications. The general look of applications is:

a) title bar
b) menu bar
c) toolbar
d) application area
e) status bar

This is why Swing frames by default have a titlebar and a place for the menubar. That is also why the default layout is a border layout as it allows you to easily add a toolbar, application area and status bar to the north, center and south as required. So until you become more familiar with GUI design just follow this approach so you don't have to think too much.
 
pete stein
Bartender
Posts: 1561
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I've been playing with JTables and TableModels lately, so I implemented a quick and dirty JTable to try to display the data held in a Priorities.ser file:


edit: added due time column with a custom time cell renderer
 
Sebastion Hill
Ranch Hand
Posts: 48
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
It's always a bit of a challenge trying to decide what information needs to accompany the code and what doesn't. My ultimate end goal is to make an application for cellphones (specifically Android phones). I think most of the comments posted are still valid but I just wanted to throw that out there in the event it changes any suggestions being made :-)

Again, thank you to EVERYONE for all the comments and suggestions :-D
 
pete stein
Bartender
Posts: 1561
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
By the way, your "end of file error I don't really understand" is not an error. This is how ObjectInputStreams determine that they've finished reading a file: by throwing an EOFException. This is one of the few times where you can justifiably have an empty catch block.
 
Sebastion Hill
Ranch Hand
Posts: 48
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Pete
1) PriorityCode - you said "The priorityCode int field is always calculated based on two other fields, activityUrgency and activityImportance. What if the two properties that are used to calculate priority change but the setPriorityCode() accidently isn't called?"


I'm a bit unclear as to how this would happen. The two fields that the priority code is dependent upon can only be set at two different times (originally and when edited) and both times are followed by the setPriorityCode() method. These two times also are only registered when a button is pushed. So I am still a bit confused on this one ...

2) consider having Activity class implement the Comparable<Activity>interface

I definitely would not mind doing this but again I am a bit unclear as to what kind of value this provides, please remember I am a newbie :-) I am using the Activity objects in an ArrayList (priorities) and I thought that the comparing should be done to the list as opposed to the actual activity. Or can my Activity class contain the actual arraylist? :-/

3) Two OBJECT fields ... activityDateValue and activityTimeValue

Yeah I just didn't know what kind of "object" the value from dateSpinner.getValue() is ... I looked up the method in the API and it only shows " Object getValue()" ... could you tell me what type of object it is?

(dateSpinner = new JSpinner(dateModel);
dateSpinner.setEditor(new JSpinner.DateEditor(dateSpinner, "MM/dd/yyyy"));
dateValue = dateSpinner.getValue();)
http://java.sun.com/j2se/1.5.0/docs/api/javax/swing/JSpinner.html

4) Gridbaglayout

... the java tutorials said this was the easiest to learn

5) Make some classes stand alone, de-couple as much as possible and use interfaces

6) Incorporate more MVC concepts in my code including getting the core model - the list of activities - out of the GUI and also extracting out file IO as well


It seems like if I incorporate more MVC concepts into my code it will also address #5 ... so I am currently reading up on MVC with the following links:
http://java.sun.com/developer/technicalArticles/javase/mvc/
http://java.sun.com/products/jfc/tsc/articles/architecture/
http://java.sun.com/blueprints/patterns/MVC-detailed.html
http://www.javadude.com/articles/vaddmvc1/mvc1.htm


Fred
7) learn more about layout managers
8) high level containter class to be just a shell


Thanks for both comments Fred :-) I definitely need to learn more about layout managers and I think my high level container class is just a shell like you mentioned.

Rob
9) readability of the code ... consistency, indention


That's is not nit-picking ... i just have issues. I was really confused as to how much one should indent for each section ... i think I'm going to go with 3 spaces ... is there a way to change the indent size so I can just press the tab key?

10) fix resizing (text fields, spinners, buttons "should not" grow ... only components like scroll panes with tables, text areas, etc. should grow)

Will fix this as well. Thanks for letting me know which ones should and should not grow :-)

11) consider non-resizable popup dialogs

I tried using popup dialogs but I don't know how to include the JSpinner in them ... do you have any code example that shows the proper way to include the info like on my "create new"?

12) fix duplicate code and use a shared panel to enter/change the Activity information

Again, still don't know how to use a shared panel but I think that's what the Model-View-Component (MVC) is talking about correct?

13) 3 buttons on the "Current List of Activities" take up way to much space

I have it this way specifically for when I port it to cell phones ... won't be too much space on those screens

14) Add a "cancel" button for the "delete" button in case someone changes their mind

Will do :-) I think I'll be able to do that with the JOptionsPane right?

15) "Description" type field is usually a text area to allow the user more room to provide a detailed description

Again, just thinking about the screen real-estate size on cellphones :-)

16) Need multiple source files ... shrink the size of the main class a great deal

That's the MVC again right? I'm pretty scared of multiple source files since I don't know how to really access certain things from within ... methods seem easy but listeners scare me

17) learn to copy the LAF of exisiting applications. The general look of an application is:
a) title bar
b) menu bar
c) toolbar
d) application area
e) status bar


This definitely makes sense for Java programs so I guess I'll just look at Android programs as well and find out what their LAF is ... what is LAF by the way? :-) Thanks so much for the pointers

Pete
Quick and dirty JTable looks awesome! :-) For my complete activities I wanted to send them to a seperate file so that you can track what you've taken care of. This will be perfect :-D
 
Sheriff
Posts: 22781
131
Eclipse IDE Spring VI Editor Chrome Java Windows
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Sebastion Hill wrote:4) Gridbaglayout

... the java tutorials said this was the easiest to learn


Are you kidding me? GridBagLayout is probably the hardest to learn.
 
Sebastion Hill
Ranch Hand
Posts: 48
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I guess I made that up :-) ... flexible and easy ... must have been tired that day :-)

"Note: This lesson covers writing layout code by hand, which can be challenging. If you are not interested in learning all the details of layout management, you might prefer to use the GroupLayout layout manager combined with a builder tool to lay out your GUI. One such builder tool is the NetBeans IDE. Otherwise, if you want to code by hand and do not want to use GroupLayout, then GridBagLayout is recommended as the next most flexible and powerful layout manager."
 
Fred Hamilton
Ranch Hand
Posts: 686
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
A GUI layout can have different levels. At the highest level, your GUI may be laid out in a few sections according to one simple layout manager. Then within each section the individual components can be laid out according to another layout manager. By breaking it down this way, you can use a combination of simple layouts to achieve an overall effect that looks sophisticated.

catch my drift?

The article I linked in my first post is very good at describing this strategy of "layouts within layouts"
 
Rob Camick
Rancher
Posts: 3324
32
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
2) consider having Activity class implement the Comparable<Activity>interface

> I definitely would not mind doing this but again I am a bit unclear as to what kind of value this provides

The "natural" sort order should be part of the class. Additional sort orders can be applied by using Comparators.

See http://forums.sun.com/thread.jspa?forumID=256&threadID=598401

3) Two OBJECT fields ... activityDateValue and activityTimeValue

> Yeah I just didn't know what kind of "object" the value from dateSpinner.getValue() is

Well, since you are using a DateEditor, the obvious guess would be "Date". Try it, the worst that will happen is you get a ClassCastException, in which case the message will tell you the class. Or you can always use the getClass() method to find the class of any Object.

9) readability of the code ... consistency, indention

> I was really confused as to how much one should indent for each section ... i think I'm going to go with 3 spaces

Well, you generally learn by example. Look at the examples in the Sun tutorial. Look at the examples in your text book.
Look at the examples in the forums. The first hit when you use "java coding standards" on Google is: http://java.sun.com/docs/codeconv/. It addresses this issue among others.

11) consider non-resizable popup dialogs

> I tried using popup dialogs but I don't know how to include the JSpinner

Sorry, bad terminoligy. I meant just use a JDialog. You build a dialog the same way you build a frame.

12) fix duplicate code and use a shared panel to enter/change the Activity information

> Again, still don't know how to use a shared panel but I think that's what the Model-View-Component (MVC) is talking about correct?

Not even that complicated. Just create an ActivityPanel that extend JPanel. The panel simply allows you to display and edit all the fields of your activity class. To use it you just do something like:

JDialog dialog = new JDialog("Edit Activity");
ActivityPanel activityPanel = new ActivityPanel();
dialog.add( activityPanel );

You would probably want to add setActivity() and getActivity() methods to the Activity panel class so you can display an Activity for editing and then retrieve the values after editing. The you add a couple of buttons to the dialog like "Save" and "Cancel" and your all set.

As I intially said, I don't believe this "Create Activity" should be a tab, but if you do leave it as one, then you can just add the ActivityPanel to the tabbed panel as well.

13) 3 buttons on the "Current List of Activities" take up way to much space

> I have it this way specifically for when I port it to cell phones ... won't be too much space on those screens

It will take up 2 more lines of space then it needs to. Extra space would be better allocated to the description field.

17) learn to copy the LAF of exisiting applications.

> I'll just look at Android programs as well and find out what their LAF is ... what is LAF by the way?

LAF - "Look and Feel". The original question didn't mention the "cell phone" requirement. I don't know what the LAF is in this case so some suggestions may be off.
 
Sebastion Hill
Ranch Hand
Posts: 48
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
thanks again Rob, btw it was date for the .getValue() like you said :-)

I'm working on rewriting my code using principles/design from MVC ... should have something to post tomorrow and hopefully it addresses you and everyone else's suggestions thus far. thanks again! :-D
 
Sebastion Hill
Ranch Hand
Posts: 48
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I still haven't finished getting everything fixed but I did change my program up so that it is HOPEFULLY more ModelViewComponent-like ... I have run into a problem of saving and loading the activities I'm recording, not really sure how to fix it? Could someone help point me in the right direction please? :-D (I think the problem is somewhere in my PlannerAppDemo class) [Note: I have the two loading procedures blocked off by the "//" sign in the PlannerAppDemo class]









 
Sebastion Hill
Ranch Hand
Posts: 48
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Well I think it's working fine now, and hopefully it still follows MVC design philosophy ... I noticed there the problem was coming from me trying to read in the code in the TabView class, from the PlannerAppDemo class. The actionlistener events weren't working because of the way it was setup. I switched up the read in procedure to read in from the PlannerAppDemo class and then have tabs inserted using the instance of the TabView class ... anyways it works now :-)

I think I also addressed a good portion of the suggestions but in case I didn't please let me know. Thanks again for everyone's help!



 
A day job? In an office? My worst nightmare! Comfort me tiny ad!
a bit of art, as a gift, the permaculture playing cards
https://gardener-gift.com
reply
    Bookmark Topic Watch Topic
  • New Topic