Win a copy of Kotlin in Action this week in the Kotlin forum!
  • Post Reply Bookmark Topic Watch Topic
  • New Topic

Rate my program  RSS feed

 
Al Hobbs
Greenhorn
Posts: 3
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The fractioncalc.java is the starting point.  This is program is a fraction calculator.  you can put in a whole number or fraction and get your answer as a fraction or decimal.

It compiles fine I just want to know if there is something I'm doing wrong in terms of efficiency or style.  Instead of using the GUI wizard I did it manually with gridbag layout.  Do people generally just use the GUI wizard of their IDE?
Also, should I make all my methods throw exceptions and have more try/catch statements?

Thanks!! ><






 
Stephan van Hulst
Saloon Keeper
Posts: 7808
142
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I tend to craft my Swing applications by hand, using BorderLayout and BoxLayout if possible, but for complex forms GridBagLayout certainly is most flexible. I'm glad to see you're getting practice by doing it by hand as well.

Here are some remarks:

  • Don't extend JFrame. Your calculator uses a frame, it is not one.
  • Your Fraction, CalcPanel and FractionCalc classes can be final.
  • Don't call pack() or setVisible() from the constructor. They can start new threads and you shouldn't start threads from constructors.
  • Use whole words. la, instru, fc, a, b, calc, dec, nu, deno, etc. don't mean anything. Characters don't cost anything, so write identifiers out.
  • Why are you resetting gridx, fill and gridwidth for the second component? They already had the intended values after setting them the first time.
  • You should interact with Swing components from the Event Dispatch Thread. Use SwingUtilities.invokeLater() to construct your calculator from the EDT.
  • You're not consequent with your imports. Why do you use the star import for java.awt and javax.swing, but not for java.awt.event?
  • Why does CalcPanel contain protected members?
  • Don't reuse listeners in multiple components if you're going to differentiate their behavior based on getSource().
  • If you only need a partial implementation of a listener, override the adapter instead. For instance, create an anonymous FocusAdapter instead of a FocusListener.
  • Instead of using switch statements, bind an instance of an operator class to your choiceBox, and then let the currently selected item perform the operation on the operands.
  • Use Action instead of ActionListener. You can easily reuse them in multiple components, and they will all get the same name.
  • You can probably lay out the components of your CalcPanel using some sort of loop.
  • Remove commented-out code from your programs.
  • Your Fraction class doesn't use a normalized internal representation. The values of zero, one and whole can be derived from nu and deno.
  • You're not performing parameter checking in your constructors.
  • I recommend giving Fraction only one private constructor, and then provide a factory method for integers and a factory method for other fractions.
  • Don't overload the equals method. Instead, override equals(Object) and hashCode().
  • If you make sure that instances of fractions only exist in their simplified form, you can implement Comparable<Fraction> easily.
  • Don't perform parameter checking in your gcd() method. It's private, you may assume it will be called correctly.


  • When you've made changes, post your updated code and I will give you more pointers. If you're interested, I can share how I would write this application.
     
    Campbell Ritchie
    Marshal
    Posts: 55717
    163
    • Likes 1
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Stephan van Hulst wrote:I tend to craft my Swing applications by hand, using BorderLayout and BoxLayout if possible, but for complex forms GridBagLayout certainly is most flexible. I'm glad to see you're getting practice by doing it by hand as well. . . .
    Find out about Cay Horstmann's GBC class, which makes grid bag easier to use. There are more details in his Book Core JavaII (vol I). This grid bag tutorial is only suitable for drinking beer to.
  • Don't extend JFrame. Your calculator uses a frame, it is not one.
  • . . . .
    I would go further and say that the calculator shou‍ld be in a different class from all the display components.
     
    Al Hobbs
    Greenhorn
    Posts: 3
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Hey Stephan,

    I really appreciate your feedback.        I just finished changing most of it if I could figure it out.  I'll let you know what I didn't understand even after researching.      It's only a few things.  I'm not sure if I did the instructions right on some of them, but they should all be about right.

    Stephan van Hulst wrote:

  • Don't extend JFrame. Your calculator uses a frame, it is not one.

  • Your Fraction, CalcPanel and FractionCalc classes can be final.

  • Don't call pack() or setVisible() from the constructor. They can start new threads and you shouldn't start threads from constructors.

  • Use whole words. la, instru, fc, a, b, calc, dec, nu, deno, etc. don't mean anything. Characters don't cost anything, so write identifiers out.

  • Why are you resetting gridx, fill and gridwidth for the second component? They already had the intended values after setting them the first time.

  • You should interact with Swing components from the Event Dispatch Thread. Use SwingUtilities.invokeLater() to construct your calculator from the EDT.

  • You're not consequent with your imports. Why do you use the star import for java.awt and javax.swing, but not for java.awt.event?

  • Why does CalcPanel contain protected members?

  • Use Action instead of ActionListener. You can easily reuse them in multiple components, and they will all get the same name.

  • Remove commented-out code from your programs.

  • I recommend giving Fraction only one private constructor, and then provide a factory method for integers and a factory method for other fractions.

  • Don't overload the equals method. Instead, override equals(Object) and hashCode().

  • Your Fraction class doesn't use a normalized internal representation. The values of zero, one and whole can be derived from nu and deno.

  • Don't perform parameter checking in your gcd() method. It's private, you may assume it will be called correctly.






  • Stephan van Hulst wrote:
  • Don't reuse listeners in multiple components if you're going to differentiate their behavior based on getSource().
  • If you only need a partial implementation of a listener, override the adapter instead. For instance, create an anonymous FocusAdapter instead of a FocusListener.


  • For this I made CalcPanel implement Focuslistener.  I originally changed what I had into focusadapter, but I changed it to implement instead because I couldn't figure how to give them all the same listener without reusing the listener.

    Stephan van Hulst wrote:
  • Instead of using switch statements, bind an instance of an operator class to your choiceBox, and then let the currently selected item perform the operation on the operands.

  • I didn't really know exactly what you were talking about.  I had made my own operator methods but I wasn't sure what you meant by binding a class to the box.
    Stephan van Hulst wrote:
  • You can probably lay out the components of your CalcPanel using some sort of loop.
  • You're not performing parameter checking in your constructors.
  • If you make sure that instances of fractions only exist in their simplified form, you can implement Comparable<Fraction> easily.


  • I'm not really sure how I would use a loop to layout the components if I only have one of each to do.
    I wasn't really sure what kind of parameter checking to do.  I added some checks in the actions of the buttons to check whether they will work without returning an error.
    Not really sure what I'd do with comparable.

    Overall I added most of the stuff.  I'd definitely like to hear more of your critique and your thoughts on how you'd do it.

    Here's the updated code:


     
    Campbell Ritchie
    Marshal
    Posts: 55717
    163
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Al Hobbs wrote:. . . I made CalcPanel implement Focuslistener.   . . .
    Please explain why. Whenever I see a display component implementing a listener, I start to get suspicious about the design. Does the Listener apply to the panel itself?

    Have you read the documentation for Comparable? Do you know what a total ordering means? This recent post of mine tells you how to implement Comparable wrongly (‍), and has other useful in formation in.
     
    Stephan van Hulst
    Saloon Keeper
    Posts: 7808
    142
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Al Hobbs wrote:For this I made CalcPanel implement Focuslistener.  I originally changed what I had into focusadapter, but I changed it to implement instead because I couldn't figure how to give them all the same listener without reusing the listener.

    Don't let components implement listeners. Usually you would create a separate listener object per control that you need to. I misinterpreted your initial code though. This is one of the few instances where using getSource() and applying the same listener to multiple controls is actually warranted. It would be better to make it into a reusable class though:

    I had made my own operator methods but I wasn't sure what you meant by binding a class to the box.

    This ties in to what Campbell said about the calculator being a different class from all the display components. Right now you're letting your view decide which calculations to perform. Instead, you could create a Calculator class that models all the operations that you can perform, and the view just passes calls through to the calculator. Right now, this calculator would be nothing more than a collection of operators, so maybe all you need is a custom RationalOperator class.

    JComboBox is a generic class. You can add items of a specific type, and JComboBox will use the toString() method to display them. This gives you the opportunity to add instances of RationalOperator. When you hit a button, you can just let the view retrieve the currently selected operator and pass the operands to it, and it will then perform the calculation. No hard-coded decision trees!

    I'm not really sure how I would use a loop to layout the components if I only have one of each to do.

    I'll show how I would write the GUI later, and I'll see if I can address this point then.

    I wasn't really sure what kind of parameter checking to do.  I added some checks in the actions of the buttons to check whether they will work without returning an error.

    Your Fraction class doesn't check its constructor parameters. What if a denominator is negative? What if the denominator is zero? I can imagine you would want to allow negative values, and just normalize them by negating the numerator, but is zero really a valid value for the denominator? Check it and throw an exception. Debugging an application becomes MUCH easier when every non-private method checks that its parameters are within a valid range, and throws an exception when they aren't. Check out the RationalOperator class. It checks that no null arguments are passed, and that strings are trimmed and non-empty.

    Not really sure what I'd do with comparable.

    This was mostly a suggestion rather than strong advice. Classes that represent values should override equals(Object) and hashCode(), and if they can be compared to each other, they should implement Comparable. There's little use to writing code if you're not going to use it, though, so it's not a hard requirement.

    Some comments about your new code:

  • What is a FractionCalc? did you mean FractionCalculator? It isn't actually a calculator though, it's a class that just initializes and runs your form. Call it something like CalculatorApplication or CalculatorForm.
  • The fractionCalc field should probably be named frame, and it should be private and final.
  • Why does the addComponents() method require a container parameter? It can directly operate on the field.
  • Instead of writing an anonymous class to create and turn on your application, you can use a lambda expression.
  • Don't let components implement listeners. Keep an instance of OnFocusEraser in your panel class.
  • Your panel class doesn't need the num1, num2 and operation fields. Their values are already represented by the firstNumberField, secondNumberField and choiceBox.
  • Class names should always begin with capital letters, so your abstract actions as well.
  • Make your abstract actions anonymous, and assign an instance of them to a field of your panel. I'll give an example shortly.
  • Don't verify fields in your actions. Disable actions when the fields they are supposed to work with hold invalid values. You can also add instances of InputVerifier to your text fields.
  • The pattern you use to verify input isn't actually all that robust. What if I enter a Chinese ideogram?
  • You could make your "Decimal" button toggle between decimal and fractional notation. You can do this with the setAction() method.
  • Your Fraction class still doesn't have a normalized state. The zero, one and whole fields are redundant. You can calculate them on the fly from the numerator and denominator.
  • Numer doesn't mean anything. Deno doesn't mean anything. Personally, I also prefer to drop the get prefix in the case of simple properties, so I'd just call them numerator() and denominator().
  • Your toString() method doesn't need the zero and one properties. If the denominator is one, just return the numerator by itself. Zero and one are not interesting special cases.
  • Don't use double to implement your equals() method. Floating point values are not exact. Instead, find the least common multiple of the two fractions.
  • You don't need a gcd() method. You can use BigInteger.gcd(). You may want to consider basing your entire Fraction class on BigInteger rather than int.

  • Here's an example of how to use actions as fields:
     
    Al Hobbs
    Greenhorn
    Posts: 3
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Hi Stephan,

    Sorry for the late reply.  I needed some time to wrap my head around some of the things you were saying.  I was able to implement about all of your suggestions.

    Stephan van Hulst wrote:
  • What is a FractionCalc? did you mean FractionCalculator? It isn't actually a calculator though, it's a class that just initializes and runs your form. Call it something like CalculatorApplication or CalculatorForm.
  • The fractionCalc field should probably be named frame, and it should be private and final.
  • Why does the addComponents() method require a container parameter? It can directly operate on the field.
  • Don't let components implement listeners. Keep an instance of OnFocusEraser in your panel class.
  • Your panel class doesn't need the num1, num2 and operation fields. Their values are already represented by the firstNumberField, secondNumberField and choiceBox.
  • Class names should always begin with capital letters, so your abstract actions as well.
  • Don't verify fields in your actions. Disable actions when the fields they are supposed to work with hold invalid values. You can also add instances of InputVerifier to your text fields.
  • The pattern you use to verify input isn't actually all that robust. What if I enter a Chinese ideogram?
  • You could make your "Decimal" button toggle between decimal and fractional notation. You can do this with the setAction() method.
  • Your Fraction class still doesn't have a normalized state. The zero, one and whole fields are redundant. You can calculate them on the fly from the numerator and denominator.
  • Numer doesn't mean anything. Deno doesn't mean anything. Personally, I also prefer to drop the get prefix in the case of simple properties, so I'd just call them numerator() and denominator().
  • Your toString() method doesn't need the zero and one properties. If the denominator is one, just return the numerator by itself. Zero and one are not interesting special cases.
  • Don't use double to implement your equals() method. Floating point values are not exact. Instead, find the least common multiple of the two fractions.



  • Stephan van Hulst wrote:
  • Make your abstract actions anonymous, and assign an instance of them to a field of your panel. I'll give an example shortly.
  • Instead of writing an anonymous class to create and turn on your application, you can use a lambda expression.
  • You don't need a gcd() method. You can use BigInteger.gcd(). You may want to consider basing your entire Fraction class on BigInteger rather than int.


  • I had written the reusable class Handlers and it worked great except that the useDecimalNotationAction had an illegal reference to the useFractionalNotationAction.  The IDE solved it by basically writing what was in the Handler class.  I wasn't sure the appropriate way to respond to it. 

    I wasn't sure about the anonymous class part of starting the application.  Did you mean erasing the FractionCalculatorForm and just putting it's details into an anonymous class in CalcPanel or Fraction?
    Wow, I had no idea there would already be a method built into the standard API already for finding gcd.   I just kept it because it's only one method, and I like my method I wrote because it's recursive.
    I had alot of trouble with disabling the buttons.  At first I had it disable both buttons from shouldyieldfocus, but there was a problem because the buttons could only get re enabled by clicking a textfield.  When I used your way of making the actions, I changed the action to do nothing and change the label when the input was incorrect, so that it could be clicked again.  I had tried using documentlistener before I went the current route but I couldn't get it to work.

    I was also wondering if there's a good book that has stuff about making reusable classes like the Handlers class. I honestly didn't understand it at first.  Also, is there anyway to make classes like that but for different things like an inputverifier?
    thanks again for your help.        









     
    Stephan van Hulst
    Saloon Keeper
    Posts: 7808
    142
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Al Hobbs wrote:I had written the reusable class Handlers and it worked great except that the useDecimalNotationAction had an illegal reference to the useFractionalNotationAction.  The IDE solved it by basically writing what was in the Handler class.  I wasn't sure the appropriate way to respond to it.

    Ah that's my bad. The illegal forward reference is because a field is being referenced before it's declared. You can more elegantly solve this by qualifying the fields with the this keyword:

    While we're here, please don't write down lambda parameters with single letters for anything but the most simple expressions. Like I said before, characters cost nothing, and writing out words will make your code read more fluently. Change t back to event.

    I wasn't sure about the anonymous class part of starting the application.  Did you mean erasing the FractionCalculatorForm and just putting it's details into an anonymous class in CalcPanel or Fraction?

    No, I meant that instead of using an anonymous Runnable to initialize the application, you can use a lambda expression:

    Wow, I had no idea there would already be a method built into the standard API already for finding gcd.   I just kept it because it's only one method, and I like my method I wrote because it's recursive.

    Yes, hobby projects are great for writing your own implementations of functions, and to showcase your prowess. Remember though, for professional applications it's best to use as much as possible from the standard API, or nice third party libraries. You will spend a lot less time maintaining your application that way, because other people are responsible for fixing possible bugs.

    I had alot of trouble with disabling the buttons.  At first I had it disable both buttons from shouldyieldfocus, but there was a problem because the buttons could only get re enabled by clicking a textfield.

    InputVerifier should probably just be used to prevent focus from leaving the text field until the user has entered a valid value. If that's too annoying, you can disable your buttons using a DocumentListener.

    When I used your way of making the actions, I changed the action to do nothing and change the label when the input was incorrect, so that it could be clicked again.

    Personally, I think disabling the action is better. That way the look of your interface won't change a lot, and it's clear to the user that something must change before they are allowed to click the button. Now it looks as if the button will fix the input for you, but nothing actually happens.

    I had tried using documentlistener before I went the current route but I couldn't get it to work.

    I'll show you. First, let's add another factory method to the Handlers class:

    We add some methods to the CalcPanel class:

    Finally we update the constructor:

    The explicit call to verifyInputFieldsAndEnableCalculation() is necessary to make sure that the buttons have the correct state when the application has just started.

    I was also wondering if there's a good book that has stuff about making reusable classes like the Handlers class. I honestly didn't understand it at first. Also, is there anyway to make classes like that but for different things like an inputverifier?

    I'm sorry, I haven't actually read a lot of books on the topic. Most of what I know comes from practice. Check out the other method we added to Handlers for another example of how you can create a factory method for other event handlers.

    Other remarks on your code:

  • Put classes in appropriate packages. Fraction and RationalOperator should be in a domain model package. CalcPanel, FractionCalculatorForm, Handlers and OnFocusEraser should be in the gui package. I used com.example.calculator.model and com.example.calculator.view respectively.
  • The Fraction constructor still doesn't perform parameter checking. Zero is NOT a valid denominator.
  • Fraction.toString() still uses special cases for when the fraction's value is 0 or 1. These are not special cases. The only special case is if the denominator is 1.
  • Instead of simplifying your fractions in the equals() method, you can make sure that they are always in their simplified form to begin with.
  • Don't add useless parameters to methods just to overload them.
  • Your two simplify() methods basically do the same thing. Reuse code.
  • You're still using meaningless names, such as ya, ye, newNu, newDeno and CalcPanel. Write identifiers out.
  • Use imports statements consistently, and remove unused imports.
  • CalcPanel doesn't require the num1, num2, answerField, calculate or decimal fields.
  • You can initialize most of CalcPanel's fields when you declare them.
  • All of CalcPanel's fields (except useFractionalNotation) can be final.
  • Instead of using lambda expressions to create your operators, you can use method references.
  • Add getLeftHandFraction() and getRightHandFraction() to CalcPanel.
  • Instead of adding a process() method to CalcPanel, you can add a static parse() method to Fraction.
  • The updateAnswer() method duplicates a lot of code. First parse the two fractions, then perform the calculation, and then format them based on what notation must be used.
  • Instead of formatting your fractions in the updateAnswer() method, add a toString(boolean useFractionalNotation) method to your Fraction class.

  • Here's how you can declare and initialize all your fields in one go:
     
    • Post Reply Bookmark Topic Watch Topic
    • New Topic
    Boost this thread!