Krdytkyu

I am a beginner programmer. How would you refactor this code to abide by good coding practices? As it stands the program has 4 classes and its aim is to compute the area and perimeter of a specific shape by inputting its length from a JSlider. The MyFrame, MyShape, MyTriangle, MyCircle and MySquare. The initComponents() method is autogenerated by NetBeans autobuilder so I didn't include it below. Also the program is fully functionable as of now, just want to know how to better refactor the code, splitting it in different classes etc..

package mainpackage;





    import java.awt.event.ActionEvent;

    import java.awt.event.ActionListener;

    import java.text.DecimalFormat;

    import javax.swing.JFrame;

    import javax.swing.event.ChangeEvent;

    import javax.swing.event.ChangeListener;



public class MyFrame extends JFrame

   {



       MyShape myShape;

   int value;



   public MyFrame() 

      {



      // Create frame, panels and items within

      initComponents();

      shapeSlider.setEnabled(false);



      // Create instances of shapes when chosen in Menu

      initShapes();    



      // Adds Changelistener to slider and updates dimensionOutputField

      linkEventHandler();



      exitButton2.addActionListener(new CloseListener());



      }                        



   private void initShapes()

      {

      ShapeHandler shapeHandler = new ShapeHandler();

      squareMenuItem.addActionListener(shapeHandler);

      circleMenuItem.addActionListener(shapeHandler);

      triangleMenuItem.addActionListener(shapeHandler);



      }



   public class ShapeHandler implements ActionListener 

      {

      public void actionPerformed(ActionEvent e)

         {



            if(e.getSource() == squareMenuItem)

            {



               myShape = new MySquare();



            } else if (e.getSource() == circleMenuItem)

            {



               myShape = new MyCircle();



            } else if (e.getSource() == triangleMenuItem)

            {



               myShape = new MyTriangle();



            }



            shapeSlider.setEnabled(true);



         }



      }



   public class MyChangeAction implements ChangeListener

      {



      public void stateChanged(ChangeEvent e)

         {



         int currentValue = shapeSlider.getValue() ;

         dimensionOutputField.setText(String.valueOf(currentValue));



         DecimalFormat numberFormat = new DecimalFormat("#.00");



         double bLength = myShape.calculatePerimeter(currentValue);

         String bLengthString = numberFormat.format(bLength);

         bLengthOutputField.setText(bLengthString);



         double bArea = myShape.calculateArea(currentValue);

         String areaString = numberFormat.format(bArea);

         areaOutputField1.setText(areaString);



         }



      }



   private void linkEventHandler()

      {



         MyChangeAction sliderAction = new MyChangeAction() ;

         shapeSlider.addChangeListener(sliderAction);



      }



   private class CloseListener implements ActionListener

      {



       @Override

       public void actionPerformed(ActionEvent e) 

       {

           System.exit(0);

       }

      }



   private void exitButtonActionPerformed(java.awt.event.ActionEvent evt) {                                           



   }                                          



   public static void main(String args) 

      {

      /* Set the Nimbus look and feel */

      //<editor-fold defaultstate="collapsed" desc=" Look and feel setting code (optional) ">

      /* If Nimbus (introduced in Java SE 6) is not available, stay with the default look and feel.

         * For details see http://download.oracle.com/javase/tutorial/uiswing/lookandfeel/plaf.html 

       */

      try 

         {

         for (javax.swing.UIManager.LookAndFeelInfo info : javax.swing.UIManager.getInstalledLookAndFeels()) 

         {

            if ("Nimbus".equals(info.getName())) 

            {

               javax.swing.UIManager.setLookAndFeel(info.getClassName());

               break;

            }

         }

         } catch (ClassNotFoundException ex) 

         {

         java.util.logging.Logger.getLogger(MyFrame.class.getName()).log(java.util.logging.Level.SEVERE, null, ex);

         } catch (InstantiationException ex) 

         {

         java.util.logging.Logger.getLogger(MyFrame.class.getName()).log(java.util.logging.Level.SEVERE, null, ex);

         } catch (IllegalAccessException ex) 

         {

         java.util.logging.Logger.getLogger(MyFrame.class.getName()).log(java.util.logging.Level.SEVERE, null, ex);

         } catch (javax.swing.UnsupportedLookAndFeelException ex) 

         {

         java.util.logging.Logger.getLogger(MyFrame.class.getName()).log(java.util.logging.Level.SEVERE, null, ex);

         }

      //</editor-fold>

      //</editor-fold>



      /* Create and display the form */

      java.awt.EventQueue.invokeLater(new Runnable() 

         {

         public void run() {

            new MyFrame().setVisible(true);

         }

         });

      }  



   // Variables declaration - do not modify                     

   private javax.swing.JPanel MyControlPanel;

   private javax.swing.JLabel areaLabel1;

   private javax.swing.JTextField areaOutputField1;

   private javax.swing.JLabel bLengthLabel;

   private javax.swing.JTextField bLengthOutputField;

   private javax.swing.JMenuItem circleMenuItem;

   private javax.swing.JLabel dimensionLabel;

   private javax.swing.JTextField dimensionOutputField;

   private javax.swing.JButton exitButton2;

   private javax.swing.JPanel mainPanel2;

   private javax.swing.JLabel mainTitle;

   private javax.swing.JMenu shapeMenu;

   private javax.swing.JMenuBar shapeMenuBar;

   private javax.swing.JSlider shapeSlider;

   private javax.swing.JMenuItem squareMenuItem;

   private javax.swing.JMenuItem triangleMenuItem;

   private org.jdesktop.beansbinding.BindingGroup bindingGroup;

   // End of variables declaration                   

   }

For Java the Code style conventions are quite rigorous. I would adhere to the code style that is followed by 99% of the community. The firms you will be working for will determine the coding style.

When Java was launched, C++/C generally had an indentation of 3. And the
code had often a large number of nested { }.
It was decided to opt for more Methods (academic style) and indent to 4 in order to support "good style." The alternative explanation is that 4 is the half of 8, the normal tab size.

The NetBeans IDE's GUI builder let's set properties and code Fragments. You can use that to disable the slider, add Action listeners and such.

Since Java 8 one can use lambas, representations of anonymous functions, that under the hood will become an interface implementation. This abbreviates the Code, introducing parameter names:

component.addActionListener(new ActionListener() {

    @Override

    public void actionPerformed(ActionEvent evt) {

        myShape = new MyCircle();

    }

});

can be shortened to

component.addActionListener(evt -> myShape = new MyCircle());

If not, always use @Override on overriden Methods. Should the method have a typo, that would be reported with a compile error.

The fields may be private.

    private MyShape myShape;

    private int value;



    public MyFrame() {



        // Create frame, panels and items within

        initComponents();

        shapeSlider.setEnabled(false);



        // Create instances of shapes when chosen in Menu

        initShapes();    



        // Adds Changelistener to slider and updates dimensionOutputField

        shapeSlider.addChangeListener(new MyChangeAction());



        exitButton2.addActionListener(actionEvent -> MyFrame.this.dispose());

        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);

    }                        

The ShapeHandler could not use if-else-if chains but be object-oriented:

    public class ShapeHandler implements ActionListener {

        private final Supplier<MyShape> shapeCreator;



        public ShapeHandler(Supplier<MyShape> shapeCreator) {

            this.shapeCreator = shapeCreator;

        }



        public void actionPerformed(ActionEvent e)  {

            myShape = shapeCreator.get();

            shapeSlider.setEnabled(true);

        }

    }



    private void initShapes() {

        squareMenuItem.addActionListener(new ShapeHandler(MySquare::new));

        circleMenuItem.addActionListener(new ShapeHandler(MyCircle::new));

        triangleMenuItem.addActionListener(new ShapeHandler(MyTriangle::new));



        // Or:

        squareMenuItem.addActionListener(evt  -> myShape = new MySquare());

        circleMenuItem.addActionListener(evt -> myShape = new MyCircle());

        triangleMenuItem.addActionListener(evt -> myShape = new MyTriangle());

    }

But as you see, the class is not really needed. Though nice to not "repeat one self".

I would use @Override and possible an other Name, as Action is a known interface in swing.

    public class MyChangeAction implements ChangeListener {

        @Override

        public void stateChanged(ChangeEvent e) {

The generated main method is conservative: compilable for very old versions of Java < 7. Shorter would be:

public static void main(String args) {

    try {

        for (UIManager.LookAndFeelInfo info : UIManager.getInstalledLookAndFeels()) {

            if ("Nimbus".equals(info.getName())) {

                UIManager.setLookAndFeel(info.getClassName());

                break;

            }

        }

     } catch (ClassNotFoundException | InstantiationException

             | IllegalAccessException | UnsupportedLookAndFeelException ex) {

         Logger.getLogger(MyFrame.class.getName()).log(Level.SEVERE,

                 "Error setting theme", ex);

     }



     EventQueue.invokeLater(() -> new MyFrame().setVisible(true));

}  

Create CalculateCallback which is responsible for managing the action after the calculation is done. So perform any calculations there necessary once calculation is done.

   @FunctionalInterface

    public interface CalculateCallback {

        public void perform(double value);

    }

AreaCalculator is responsible for doing calculations and performing the calculate call back. Once the slider changes the calculation should go through AreaCalculator.

public class AreaCalculator {



    private CalculateCallback perimeterCalculatorCallback;

    private CalculateCallback areaCalculatorCallback;

    private MyShape myShape;



    public AreaCalculator(CalculateCallback perimeterCalculatorCallback, CalculateCallback areaCalculatorCallback) {

        this.perimeterCalculatorCallback = perimeterCalculatorCallback;

        this.areaCalculatorCallback = areaCalculatorCallback;

    }



    public void doCalculate(int currentValue) {

        if (null != myShape) {



            double bLength = myShape.calculatePerimeter(currentValue);

            perimeterCalculatorCallback.perform(bLength);



            double bArea = myShape.calculateArea(currentValue);

            areaCalculatorCallback.perform(bArea);

        }

    }



    public void setMyShape(MyShape myShape) {

        this.myShape = myShape;

    }

}

ShapeFactory is responsible to provide MyShape according to parameter. I didn't implement that. Please makes sure to implement this.

public class ShapeFactory {



    public static MyShape getMyShape(String item) {



        //Here you have to return MyShape according to parameter.

    }

}

ShapeHandler is responsible for handling change. It will perform calculation through AreaCalculator by providing MyShape according to action.

public class ShapeHandler implements ActionListener {



    private AreaCalculator areaCalculator;

    private ActionDoneCallback actionDoneCallback;



    public ShapeHandler(AreaCalculator areaCalculator, ActionDoneCallback doneCallback) {

        this.areaCalculator = areaCalculator;

        this.actionDoneCallback = doneCallback;

    }



    @Override

    public void actionPerformed(ActionEvent e) {

        MyShape shape = ShapeFactory.getMyShape(e.getActionCommand());

        areaCalculator.setMyShape(shape);

        actionDoneCallback.perform();

    }

}

Your MyFrame constructor should like below.

Its responsible to create callback since its the owner of component and this need to modify according to requirement.

public MyFrame() {



        // Create frame, panels and items within



        DecimalFormat numberFormat = new DecimalFormat("#.00");

        AreaCalculator areaCalculator = new AreaCalculator(

                (value) -> bLengthOutputField.setText(numberFormat.format(value)),

                (value -> areaOutputField1.setText(numberFormat.format(value))));



        ShapeHandler shapeHandler = new ShapeHandler(areaCalculator, () -> shapeSlider.setEnabled(true));

        squareMenuItem.addActionListener(shapeHandler);

        triangleMenuItem.addActionListener(shapeHandler);

        circleMenuItem.addActionListener(shapeHandler);



        shapeSlider.setEnabled(false);

        shapeSlider.addChangeListener(e -> areaCalculator.doCalculate(shapeSlider.getValue()));



        exitButton2.addActionListener(new CloseListener());



    }

Once a shape is selected ShapeHandler will set MyShape in AreaCalculator.

Once the Jslider is changed it will do the calculation base on the value.

I design this under SOLID principles and base on TDD.
so you can write unit test easy for this design.

Let me to know if you need some clarification.