HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Ballistics and arithmetic calculator

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
andballisticscalculatorarithmetic

Problem

I'm a beginner to Java and OOP, and I would really like to learn how to better organize my code! Over the course of a month or two, I made a calculator program with little functions I thought of here and there with a few small jokes built into it. After looking at it a second time, I realized that it is extremely poorly formatted and almost incomprehensible. I would like to ask some more experienced programmers to point me in the right direction on what I should do to fix it (for example, what things can I turn into objects, where can I compartmentalize, etc).

Also, before anyone asks, NO this is NOT homework, it is the product of my own crack at teaching myself Java (probably why it is not working too well).

```
// This is the original Calculator code without objects in a single class. not really efficient...

package randomClasses;

import java.awt.*;
import java.awt.event.*;
import java.text.DecimalFormat;
import javax.swing.*;

@SuppressWarnings("serial")
public class CalcClass extends JFrame implements ActionListener {

JPanel[] row = new JPanel[6];
JButton[] button = new JButton[21];
String[] buttonString = {"7", "8", "9", "+",
"4", "5", "6", "-",
"1", "2", "3", "*",
".", "/", "C", "v",
"+/-", "=", "0",
"Parabola", "x^y"};
int[] dimW = {300, 45, 100, 90, 180};
int[] dimH = {35, 40};
Dimension displayDimension = new Dimension(dimW[0], dimH[0]);
Dimension regularDimension = new Dimension(dimW[1], dimH[1]);
Dimension rColumnDimension = new Dimension(dimW[2], dimH[1]);
Dimension zeroButDimension = new Dimension(dimW[3], dimH[1]);
Dimension parabolaDimension = new Dimension(dimW[4], dimH[0]);

//formatting variables
int var = 0;
double x = 0;
String stor = "";
boolean initial = true;

//variables for Parabola function
int countEquals_parab = 0;
double Angle = 0;
double Vi = 0;
double Vx = 0;
double Vy = 0;
double T_max = 0;
double Y_displ

Solution

Just a few tips:

  • Let your IDE format the code, it's zero work and the result is consistent.



  • You should always separate the UI and the computation. Here, it may be a bit pointless as the computation is much simpler than the UI, but still it allows you to separate things. And when you want to make an Android (or console) version, you have a basis. It also allows you to test your logic (testing UI is possible, but hard and rarely done).



 

import java.awt.*;


Don't use star imports. let you IDE take care of it.

public class CalcClass extends JFrame implements ActionListener {


This is fine, but a separate ActionListener would allow you to structure the code more.

JPanel[] row = new JPanel[6];


Consider using miglayout which can do it without the panels.

String[] buttonString = {"7", "8", "9", "+", ...}


Good, but consider adding more information here. Your buttons have not only a string, but also some action and you may want to use an enum or a class to specify it.

Concerning naming, how long can you remember what rColumnDimension, zeroButDimension, stor, and x mean? Names like countEquals_parab, Angle, Vi, T_max violate Java conventions (starting lowercase, no underscores). Other names are mostly fine.

for(int i = 0; i < 5; i++)
    function[i] = false;


This should look like

for (int i = 0; i < function.length; i++) {
    function[i] = false;
}


Note the spacing, brackets, and the eliminated magic constant.

for(int i = 0; i < 14; i++)
    button[i].setPreferredSize(regularDimension);
for(int i = 14; i < 18; i++)
    button[i].setPreferredSize(rColumnDimension);
button[18].setPreferredSize(zeroButDimension);
button[19].setPreferredSize(parabolaDimension);
button[20].setPreferredSize(rColumnDimension);


I'm throwing a TooManyMagicNumbersException. What about

private Dimension buttonDimension(int index) {
     if (index < REGULAR_BUTTONS_COUNT) {
         return regularDimension;
     } else if (index < WHATEVER) {
         return rColumnDimension;
     }
     switch (index) {
         case ZERO_BUTT_INDEX: return zeroButDimension;
         ...
     }
     throw new IllegalArgumentException("Unknown index: " + index);
}


and

button[i].setPreferredSize(buttonDimension(i));


added to the other loop?

Then even more magic numbers... you really should define constants for this.

public void getSqrt() {


Anything called "getWhatever" should return some "whatever".

format(Double.toString(value)); //Sets display to new value


If a method call needs a comment, it's a clear sign that the method was misnamed. What about format -> setDisplay?

} catch(NumberFormatException e){
}


Ignoring exceptions is the deadliest Java sin. Just don't do it.

becoming lazy and skipping quite a lot

public void actionPerformed(ActionEvent ae) {
    if(ae.getSource() == button[0]) numberButtons("7");

    if(ae.getSource() == button[1]) numberButtons("8");


Couldn't button[0] correspond with "0"? This would make some things clearer (and other possible worse). However, you should do something like

String text = ((Button) ae.getSource()).getText();
  if (text.length() == 1) {
      char c = text.charAt(0);
      if ('0' <= c && c <= '9') {
          numberButtons(c);
      } else {
          operatorButtons(c);
      }
  } else {
      ...
  }


This line

if(ae.getSource() == button[7]) operatorButtons(1); //subtract function[1]


could use a constant and save you a comment. However, calling operatorButtons('-') need neither a constant nor a comment. But as said above, it could look like operatorButtons(c).

becoming even more lazy and stopping for now or forever

Code Snippets

import java.awt.*;
public class CalcClass extends JFrame implements ActionListener {
JPanel[] row = new JPanel[6];
String[] buttonString = {"7", "8", "9", "+", ...}
for(int i = 0; i < 5; i++)
    function[i] = false;

Context

StackExchange Code Review Q#92308, answer score: 10

Revisions (0)

No revisions yet.