• Post Reply Bookmark Topic Watch Topic
  • New Topic

I would really appreciate if someone could give me a short code review  RSS feed

 
Alex Matthews
Greenhorn
Posts: 7
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi. I've been learning java for over a year and I'm looking for better ways to learn. I have heard that code reviews is a good way of improving your code so I decided to ask here if somebody who is fluent in java could review my calculator code. I'd really appreciate it if someone could have a look and point out any issues I may have missed. Thanks a lot. Here is the github repository: https://github.com/AlexM9200/Java-Swing-Calculator

Alex from Ireland
 
Carey Brown
Saloon Keeper
Posts: 3314
46
Eclipse IDE Firefox Browser Java MySQL Database VI Editor Windows
  • Likes 2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Don't call setVisible() in frameSetup. You want to wait until entire GUI is configured, so, perhaps at the end of makeGUI().

Is better written as
Creates less intermediate String objects.

Do this with a loop.

Call to toString() not necessary.

Be more conservative with your use of blank lines.
 
Carey Brown
Saloon Keeper
Posts: 3314
46
Eclipse IDE Firefox Browser Java MySQL Database VI Editor Windows
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
and Welcome to Code Ranch.
 
Alex Matthews
Greenhorn
Posts: 7
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks a lot for the suggestions and for the welcome. I really appreciate it. Anyone else feel free to have a look and tell me what you think. I'm just trying to get a better idea of where I am as a programmer. I honestly have no clue if my code is good or bad.
 
Carey Brown
Saloon Keeper
Posts: 3314
46
Eclipse IDE Firefox Browser Java MySQL Database VI Editor Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Overall I think you have done well. Excellent style, indentation, braces, naming. Easy to read.
Keep it up.
 
Alex Matthews
Greenhorn
Posts: 7
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Carey Brown wrote:Overall I think you have done well. Excellent style, indentation, braces, naming. Easy to read.
Keep it up.


Thank you. That's good to know.
 
Knute Snortum
Sheriff
Posts: 4276
127
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Good job!  Some notes:

1) Even with a small project like this, I'd create a package.  you should get used to not using the default package.

2) In Calculator#MyOperationButtonListener you have a lot of "magic numbers".  These are numbers that have significance to you but their meaning is not readily apparent.  I would do something like this:

3) There are a couple of things about Calculator#screenIsEmpty:

  3a) You don't need .toString in display = screen.getText().toString();
  3b) This is safer: if ("".equals(display))
  3c) Anytime you want to write if cond return true; else return false; turn it into return cond;  So the method could be just

4) I'd like to see more JavaDoc comments, one on every class and public member.
 
Alex Matthews
Greenhorn
Posts: 7
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Knute Snortum wrote:Good job!  Some notes:

1) Even with a small project like this, I'd create a package.  You should get used to not using the default package.

2) In Calculator#MyOperationButtonListener you have a lot of "magic numbers".  These are numbers that have significance to you but their meaning is not readily apparent.  I would do something like this:

3) There are a couple of things about Calculator#screenIsEmpty:

  3a) You don't need .toString in display = screen.getText().toString();
  3b) This is safer: if ("".equals(display))
  3c) Anytime you want to write if cond return true; else return false; turn it into return cond;  So the method could be just

4) I'd like to see more JavaDoc comments, one on every class and public member.

]

Thanks for the tips. Very much appreciated.
 
Junilu Lacar
Sheriff
Posts: 11481
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I only looked at the Calculator class but overall, I'd agree with the comments made so far. Good organization, breakdown, assigning of responsibilities, style, etc.

The only thing that smells pretty bad is the huge actionPerformed method of the private MyOperationButtonListener. I suggest refactoring that some more. You seem a very able programmer so I'll just give you a little hint: That method can delegate to objects that it can look up in a Map.  You can get rid of that huge if-else-if statement and you can make your program more extensible with the Map. That is, if you decide to add more operations, you won't have to change actionPerformed. You'd just add objects to the Map.
 
Junilu Lacar
Sheriff
Posts: 11481
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Looking at your code more closely, I realize now why I was smelling something in your CalcUtils and MemoryStore classes. Your implementation's design revolves around the GUI instead of the other way around. If I were to ask you to make this program support a text mode, where I could just give it an entire expression as a String and it would calculate and display the result, you'd have to make significant changes to the core code instead of just tacking on a method or two.

Pretty much the same thing if I asked you to make the calculator support reverse Polish notation expressions. For either GUI or text-based mode.

The red flags that led me to this were the displayHasDecimal() and changeToPlusOrMinus() methods. I'll explain... (see my next reply)
 
Alex Matthews
Greenhorn
Posts: 7
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu Lacar wrote:I only looked at the Calculator class but overall, I'd agree with the comments made so far. Good organization, breakdown, assigning of responsibilities, style, etc.

The only thing that smells pretty bad is the huge actionPerformed method of the private MyOperationButtonListener. I suggest refactoring that some more. You seem a very able programmer so I'll just give you a little hint: That method can delegate to objects that it can look up in a Map.  You can get rid of that huge if-else-if statement and you can make your program more extensible with the Map. That is, if you decide to add more operations, you won't have to change actionPerformed. You'd just add objects to the Map.


Thanks a lot for the reply's. Yea I realised that the actionPerformed method of the private MyOperationButtonListener was the worst part of the code but I struggled to see how to fix it since that was the way I was thought to do it in college. Im currently in second year learning GUI dev, although I'm fairly ahead of the course material, we were being thought about how to add a button only last week. Anyway,  I asked on another forum about the problem and I got this suggestion (which I also think is not the best because it adds ~19 extra methods but I dont know). Here it is:



What do you think of that? As for the suggestion about using a map I have no clue what you mean about delegating to objects that it can look up in a map. Any small example or further explanation. Sorry for the questions but I'm just trying to learn as much as I can.


Junilu Lacar wrote:Looking at your code more closely, I realize now why I was smelling something in your CalcUtils and MemoryStore classes. Your implementation's design revolves around the GUI instead of the other way around. If I were to ask you to make this program support a text mode, where I could just give it an entire expression as a String and it would calculate and display the result, you'd have to make significant changes to the core code instead of just tacking on a method or two.

Pretty much the same thing if I asked you to make the calculator support reverse Polish notation expressions. For either GUI or text-based mode.

The red flags that led me to this were the displayHasDecimal() and changeToPlusOrMinus() methods. I'll explain... (see my next reply)


Ok, when you say the implementation's design revolves around the GUI instead of the other way around, do you mean I should have one class just for the GUI and then have another class that takes care of all the logic and the GUI class calls methods from the logic class? Am I understanding that correctly? Thanks I look forward to your next reply. Thanks a lot for the help.
 
Junilu Lacar
Sheriff
Posts: 11481
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Some background first - a calculator program is usually an exercise in programming of data structures, parsing, and finite state machines. The reason your displayHasDecimal() method smells of GUI is that it deals with none of those. Instead, it checks the state of the output. The method name also points to this problem in that it includes "display" in it.  If you were writing a proper FSM, you'd check something like isParsingDecimalPart() instead and then have your display be updated accordingly.  Your program works the other way around, checking the display and then performing some action accordingly.

The changeToPlusOrMinus() method has the same smell.  I would have expected something like toggleSign() instead. That is, when you detect the ± button in the input stream, you'd toggle the sign of the value you have parsed and then update the display. Instead, you are again checking the display for a "-" and then changing the sign of the value accordingly.

The other thing that clued me in that your implementation was GUI-centric instead of being model-centric (you should have a Calculator object that is independent of the GUI) was that it's hard to write automated tests for your code because of the tight coupling with the GUI. Also, I didn't see any sign of a stack, which is usually the central data structure of a calculator program.

For reference, see the following

Finite State Machines and Calculator programs: https://www.clear.rice.edu/comp212/06-spring/labs/13/

Stacks and Reverse Polish Notation (RPN) calculators
 
Junilu Lacar
Sheriff
Posts: 11481
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Likes 2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Alex Matthews wrote:Ok, when you say the implementation's design revolves around the GUI instead of the other way around, do you mean I should have one class just for the GUI and then have another class that takes care of all the logic and the GUI class calls methods from the logic class?

Yes, that's the idea. The design principle involved here is that of separation of concerns.

Presentation/View deals with the external representation of the state of the program. The model is where most of the calculator logic would be. Right now, your program has those two concerns tightly coupled to each other. This may seem fine to start with, as you focus on just getting your program to work. However, many real-world programs are long-lived and it's impractical to rewrite everything every time a new set of features are needed. Programs spend most of their lifetimes in maintenance and enhancement mode. It's important that they are maintainable and extensible.

A program like this can be a microcosm of larger, more complex applications so it's a good way to practice applying good design principles, detecting code/design smells, refactoring, testing, and extending programs.
 
Alex Matthews
Greenhorn
Posts: 7
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks a lot for the explanation. I'm going to refactor the code and try to reduce coupling. Im going to use a stack and im going to separate the logic from the GUI. But first I'm going to read the articles you linked me. Thanks again.
 
Junilu Lacar
Sheriff
Posts: 11481
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I would suggest that you drive your refactoring with tests. Unit tests provide a great way to rethink your program's design and they help you slowly morph one design into another. Remember, refactoring is not rewriting. Refactoring is a slow, incremental series of changes that don't change the outward behavior of your application, only its internal design. One of the first things I would try to do is write a CalculatorTest and test methods that would help me think about the API for a calculator class that would work for any kind of user interface, whether that's a graphical or text-based UI. Some questions you might ask in doing this:

Would the main calculator input method accept a String? A stream of characters? One character? What would the UI-Model interaction look like if it were a text-based UI? if it were a GUI?

A unit test would help you experiment with various options. Then you'd ask the same kind of questions regarding output/display. How does the UI interact with the model to update itself when the model changes state?

As I said, this could be a very good exercise in that it gives you the opportunity to go through the thought processes involved in the refactoring, testing, and designing of a non-trivial program.
 
Junilu Lacar
Sheriff
Posts: 11481
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Regarding the suggestion to use a Map, that was just off the top of my head and kind of an instinctive reaction to seeing a big if-else-if-else-if statement like that. Your conditionals all check ActionEvent.getSource() which then dictates some action to be performed. This suggests that the source of the event could be used as a key to a Map and the associated value/object for that key can be the delegate that performs the necessary action.  You could certainly go to those lengths or you could make your Listener objects also implement a second interface, say ActionDelegate interface. I might try making that a functional interface so I could even use lambda expressions or method references. You can then cast the object returned by ActionEvent.getSource() to an ActionDelegate and invoke the appropriate action.  But then again, going back to the principle of separation of concerns, I might think twice about making a GUI-related object implement an interface that has more affinity to the model.

All of the above are just options. To see how and if they work when coded out, I'd write unit tests. Designs have a funny way of looking good in your head but looking really ugly when they actually get codified.

I'll try to give you some examples of what I've written about in the last few replies. In fact, I've challenged my son, who is also taking college-level Java programming classes to fork your Github project (I already have done so) and try to refactor it the way I suggested here.
 
Campbell Ritchie
Marshal
Posts: 56536
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Knute: does OP know why
"text".equals(otherText)
is better than
otherText.equals("text")?
Is there any chance that getText() will cause the problem you are trying to avoid?
 
Knute Snortum
Sheriff
Posts: 4276
127
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

It's possible that screen will be null and cause a NullPointerException (NPE), which is what I was trying to avoid.  But if screen is null, the program has a serious problem and probably should throw an NPE.
 
Alex Matthews
Greenhorn
Posts: 7
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu Lacar wrote:Regarding the suggestion to use a Map, that was just off the top of my head and kind of an instinctive reaction to seeing a big if-else-if-else-if statement like that. Your conditionals all check ActionEvent.getSource() which then dictates some action to be performed. This suggests that the source of the event could be used as a key to a Map and the associated value/object for that key can be the delegate that performs the necessary action.  You could certainly go to those lengths or you could make your Listener objects also implement a second interface, say ActionDelegate interface. I might try making that a functional interface so I could even use lambda expressions or method references. You can then cast the object returned by ActionEvent.getSource() to an ActionDelegate and invoke the appropriate action.  But then again, going back to the principle of separation of concerns, I might think twice about making a GUI-related object implement an interface that has more affinity to the model.

All of the above are just options. To see how and if they work when coded out, I'd write unit tests. Designs have a funny way of looking good in your head but looking really ugly when they actually get codified.

I'll try to give you some examples of what I've written about in the last few replies. In fact, I've challenged my son, who is also taking college-level Java programming classes to fork your Github project (I already have done so) and try to refactor it the way I suggested here.


Thank you. I really appreciate all the suggestions and ill play around with it while keeping your suggestions in mind. I'd love to see your sons code if he gets around to doing it. I'll also update the git when I get around to re coding the calculator.
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!