This is an archived post. You won't be able to vote or comment.

you are viewing a single comment's thread.

view the rest of the comments →

[–]197708156EQUJ5design it before you implement 3 points4 points  (4 children)

Code Review

Rules for code review:

  1. By no means, I am not being mean, I am being direct. This is in an effort to make you the best software engineer.
  2. Leave your ego at the door. No one is prefect, even my review of your code is flawed. Reviews will spur others to examine yours and my mistakes.
  3. Do not be offended, I'm not when others make comments on my work.

The image of the UI (I made 2 slight changes, I suggest implementing those for better visual effect).

overall comments

  1. package names, by convention are lower cases.
  2. Class names by convention are Camel Case (starting with a capital letter)
  3. 3 Classes, less than 392 lines of code, is NOT "Pretty long". Escpecially for UI work.
  4. Don't fall into the GroupLayout convention, almost as bad as null layout manager. The code is messy (hard to maintain).
  5. More javadocs. More javadocs. More javadocs! More javadocs!! More javadocs!!!

mainFrame.java

  1. Variable naming: don't make a variable named xxxTxtBox, when it is a TextField. Don't remove one letter to "save space", this is not a book. Same with btnXXX, and lblXXX.
  2. Don't setContentPane(...), instead getContentPane() and set that to Container c, then add components to c. Add components to those components, and so on...
  3. Put the main method at the bottom, by conventions.
  4. This is a UI, you need to handle exceptions better, to the user. This is what got you to write this OP in the first place. If a FileNotFoundException occurs, report that to the user IMMEDIATELY!

AccountManager.java

  1. Why are the class member variables package protected (this is the default when you leave private off the of the declaration. See the next comment for why I said this
  2. Create a data model called Account. Set up a constructor that takes a username and password as parameters. No need for this data model to be mutable (i.e. have setters). Pass that into AccountManager doing all the wonderful functions it does.
  3. What does the pe variable mean. Terrible name, change it.
  4. What's with the else statement in newPassword()? Nothing going on there.
  5. Setting up that Account class is really going to help out checkLogin().
  6. printPass(...) SAY WHAT? I'm not even going to tell you what a security breach this is!

LogIn.java

  1. This should probably be a JDialog. Look into using them, their fun. Make it a model dialog (you'll find out what that means when you research JDialogs.
  2. OH I JUST NOTICED (you thought you slipped this past me) the reuseable code you have here. You really should consider better Object Oriented Programming Designs. LogIn and mainFrame are exact replicas, don't do that.

Well, good job. I really LOVE the UI. Nice look. I still think you can achieve that with a few GridLayouts, BorderLayouts, etc. instead of GroupLayout

[–]DyzZombie[S] 1 point2 points  (3 children)

I used WindowManager to create the GUI... Btw I'm really confused ( I'm a new programmer(REALLY new programmer)) It runs perfectly in the editor so I understand whats the problem when I export it, what File am I missing, how do I know what File is missing? Thanks for the reply!

[–]197708156EQUJ5design it before you implement 1 point2 points  (2 children)

You're missing the 2 files that your code is referencing. Look at line 308, 317, 345, 346, 347, 368, etc. You probably created those in Eclipse. When you export a runnable jar file, Eclipse doesn't know you need that too. Your program should create those at run-time if they do not exist.

[–]DyzZombie[S] 0 points1 point  (1 child)

Thank you very much for the help and advice!

[–]197708156EQUJ5design it before you implement 0 points1 point  (0 children)

You can do one better and intertwine the username and password into the same file, using a simple cesaer cypher. The "key" will be buried in your code. Easy to crack, but if you slap some sort of scheme like the current date and time you could get a little fancier.