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

Beginning calculator program

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

Problem

I am a beginner in Java and programming at all. I decide to try to write a calculator, as a my first "complete" program. It works for me, however I am curious is it worth anything in reality. I would be very grateful for any comments and advices.
I am especially concerned about:

  • usage of access modifiers - am I doing it correctly?



  • code structure - is it make sense in this case, to base it on inner classes?



  • readability of code



  • length of code - is code not too long as for a calculator?



```
import javax.swing.*;
import java.awt.*;
import java.awt.event.*;
import java.math.*;
import java.util.*;
import java.math.BigDecimal;
import java.io.*;

public class Kalkulator {
JFrame frame;
JPanel mainPanel;
JPanel displayPanel;
JPanel sidePanel;
static JTextArea mainDisplay;
// main display, holds only recently introduced value
static JTextArea secDisplay;
// secondary display, holds whole operation date, until use of "="
static JTextArea sideTextArea;
static JTextField sideTextField;

private static String numValue;
private static ArrayList memory; // working memory of calculator
private static ArrayList operation;
private static BigDecimal result;
private BigDecimal memoryStore; // function of memory, holds one value
private static Integer prec;
private static boolean root;
ImageIcon img;

JCheckBoxMenuItem wrap;
JCheckBoxMenuItem history;

public static void main(String[] args) {
Kalkulator calc = new Kalkulator();
calc.go();
}

public void go() {

numValue = ""; // String with currently introduced values
memory = new ArrayList();
// holds String with values for operations
operation = new ArrayList(); // holds symbols of operations
prec = 15; // decimal point accuracy,
memoryStore = new BigDecimal(0); // BigDecimal keeps one value in memory
root = false; // controls blocking of buttons

/*

Solution


  • usage of access modifiers - am I doing it correctly?




It's a good rule of thumb to first make everything private as much as possible, and relax some of those restrictions later as needed and well justified.

All those static fields... Look bad. Static fields are good for immutable constants. They should not be used to keep track of state. I suggest to make all the static fields non static.



  • code structure - is it make sense in this case, to base it on inner classes?




It's OK to use inner classes, but you should not copy-paste classes but instead generalize them with parametrized constructors when possible. For example you could replace all the B0ActList to B9ActList classes with a single more general class that takes the button label as constructor parameter. Some of the other inner classes can be generalized too.



  • readability of code




The code is well formatted, well indented, nicely readable.



  • Length of code - is code not too long as for a calculator?




Yes, I think it's a bit long for what it is. There are many repeated elements and duplicated logic. For example the way you add the buttons and their action listeners is overly repetitive. Keep in mind that duplicated elements are not only boring to read, they quickly become a maintenance nightmare. When you want to change something later, you have to remember to make the same change everywhere, which is prone to errors. I suggest to refactor, and little by little eliminate the repeated elements.

Context

StackExchange Code Review Q#82778, answer score: 6

Revisions (0)

No revisions yet.