I need a different approach - suggestions please

[bilsch]

I'm trying to make a program that works like the calculator in Windows
Accessories - all input is from button clicks. Below I listed a
stripped down version of the GUI and a little of the processing behind
it. It is in two small files. I had intended that the program would
display a sequence of digits from button clicks, which I would then
convert into a Double precision number. The problem is I am constrained
to initialize the string within the method that accumulates the sequence
of digits. Everytime the method is called the string is reinitialized
with the result that my sequence of digits is only ever one digit long.
My plan would work if I could initialize the string outside the method,
however variable scope in Java doesn't allow it.

If you try the code below you will see my problem. Alternative
suggestions on how to make a simple calculator will be appreciated.

TIA Bill S.

HERE'S THE GUI:

import java.awt.*;
import javax.swing.*;

public class CalcGUIQ1 extends JFrame {
CrunchQ1 crunchNu = new CrunchQ1(this);

// set up row 1
JPanel row1 = new JPanel();
JTextField number1 = new JTextField(10);
// set up row 2
JPanel row2 = new JPanel();


JButton sev = new JButton("7");
JButton ate = new JButton("8");
JButton nin = new JButton("9");

JButton fou = new JButton("4");
JButton fiv = new JButton("5");
JButton six = new JButton("6");

JButton one = new JButton("1");
JButton two = new JButton("2");
JButton tre = new JButton("3");

JButton zro = new JButton("0");
JButton dot = new JButton(".");

public CalcGUIQ1() {
super();
setTitle("Calculator");
setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
GridLayout layout = new GridLayout(2, 1, 10, 10);
setLayout(layout);

//add listeners
dot.addActionListener(crunchNu);
zro.addActionListener(crunchNu);
one.addActionListener(crunchNu);
two.addActionListener(crunchNu);
tre.addActionListener(crunchNu);
fou.addActionListener(crunchNu);
fiv.addActionListener(crunchNu);
six.addActionListener(crunchNu);
sev.addActionListener(crunchNu);
ate.addActionListener(crunchNu);
nin.addActionListener(crunchNu);


FlowLayout layout1 = new FlowLayout(FlowLayout.CENTER, 10, 10);
row1.add(number1);
row1.setLayout(layout1);
add(row1);

GridLayout layout2 = new GridLayout(4, 3, 10, 10);
row2.setLayout(layout2);

row2.add (sev);
row2.add (ate);
row2.add (nin);

row2.add (fou);
row2.add (fiv);
row2.add (six);

row2.add (one);
row2.add (two);
row2.add (tre);

row2.add (zro);
row2.add (dot);

add(row2);
pack();
setVisible(true);
}
public static void main(String[] arguments) {
CalcGUIQ1 frame = new CalcGUIQ1();
}
}


HERE'S THE SECOND FILE:

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

public class CrunchQ1 implements ActionListener{
CalcGUIQ1 gui;
public CrunchQ1(CalcGUIQ1 in) {
gui = in;
}
public void actionPerformed(ActionEvent event){
String strng1 = "";
String btn = event.getActionCommand();

if (btn == ".") {strng1 += btn;}
else if (btn == "0") {strng1 += btn;}
else if (btn == "1") {strng1 += btn;}
else if (btn == "2") {strng1 += btn;}
else if (btn == "3") {strng1 += btn;}
else if (btn == "4") {strng1 += btn;}
else if (btn == "5") {strng1 += btn;}
else if (btn == "6") {strng1 += btn;}
else if (btn == "7") {strng1 += btn;}
else if (btn == "8") {strng1 += btn;}
else if (btn == "9") {strng1 += btn;}

gui.number1.setText(strng1);
}

}

 

[markspace]

I'm trying to make a program that works like the calculator in Windows
Accessories - all input is from button clicks. ...
Everytime the method is called the string is reinitialized
with the result that my sequence of digits is only ever one digit long.
My plan would work if I could initialize the string outside the method,
however variable scope in Java doesn't allow it.

First, good job on making a very reasonable SSCCE.

Second, the trick to Java's scoping rules is to change the rules!

Move the string strng1 from inside the actionPerformed to outside, right
below the CalcGUIQ1 gui; line. Now Java's scoping rules help you rather
than hinder you.

BTW, this looks like a homework problem, and it looks like you've been
getting help on it. Some if it is a bit sophisticated for someone who
doesn't understand scoping. Please try to talk to your instructor or a
TA, they need to understand when you're having problems with your lessons.

 

[bilsch]

First, good job on making a very reasonable SSCCE.

Second, the trick to Java's scoping rules is to change the rules!

Move the string strng1 from inside the actionPerformed to outside, right
below the CalcGUIQ1 gui; line. Now Java's scoping rules help you rather
than hinder you.

BTW, this looks like a homework problem, and it looks like you've been
getting help on it. Some if it is a bit sophisticated for someone who
doesn't understand scoping. Please try to talk to your instructor or a
TA, they need to understand when you're having problems with your lessons.

Thanks for the help. I could swear I tried that first but got error
messages about static and non-static conflict problem. i will be taking
Java in fall quarter. Right now I'm working from "Learn Java in 24
Hours" I thought up the calculator project myself. Thanks again.


Bill S.

 

[markspace]

Thanks for the help. I could swear I tried that first but got error
messages about static and non-static conflict problem. i will be taking
Java in fall quarter. Right now I'm working from "Learn Java in 24
Hours" I thought up the calculator project myself. Thanks again.

It was the Q1 parts of your code that really tipped me off. I suppose
you could be planning on a future Q1 class, but it did make it look like
you where taking a course over the summer.

BTW, another way to improve you code is to watch the big cascades of
if-else statements. For example this:

if (btn == ".") {strng1 += btn;}
else if (btn == "0") {strng1 += btn;}
else if (btn == "1") {strng1 += btn;}
else if (btn == "2") {strng1 += btn;}
else if (btn == "3") {strng1 += btn;}
else if (btn == "4") {strng1 += btn;}
else if (btn == "5") {strng1 += btn;}
else if (btn == "6") {strng1 += btn;}
else if (btn == "7") {strng1 += btn;}
else if (btn == "8") {strng1 += btn;}
else if (btn == "9") {strng1 += btn;}

could all just be

strng1 += btn;

because you don't do anything different for the different cases.
Copy-paste statement like this should be avoid, because it creates
relatively low value redundancy. Try to combine common cases into one
block. It's easier to go back and modify later.

 

[Lew]

Thanks for the help. I could swear I tried that first but got error
messages about static and non-static conflict problem. i will be taking
Java in fall quarter. Right now I'm working from "Learn Java in 24
Hours" I thought up the calculator project myself. Thanks again.

There are a few mistakes in your code.

You don't need to call 'super()' in the constructor explicitly.
That's what happens by default anyway.

You called the constructor directly from the 'main()' routine. That means
you called it from the primary thread of the program. You don't know this
yet, probably, unless you've already studied concurrency in Java a little bit.

The problem is that the GUI won't work right if you do that. You have to
move GUI actions onto the "Event Dispatch Thread" (EDT), a background
thread that the system creates to handle all GUI actions.

Also, you start all the action from the constructor. That's bad. As its name
implies, a constructor's purpose is to _construct_ an object, not run its logic.
Run the logic after construction completes and the instance is no longer in a
partially-built state.

And make your indentation consistent with the Java coding conventions (available
on line).

So all together, you'd do something like:

public static void main(String[] arguments) {
java.awt.EventQueue.invokeAndWait( new Runnable() {
@Override public void run() {
CalcGUIQ1 calculator = new CalcGUIQ1();
calculator.setVisible(true);
}
});
}

 

[Roedy Green]

JButton sev = new JButton("7");
JButton ate = new JButton("8");
JButton nin = new JButton("9");

you code will simplify if use use a array of JButtorns or some
subclass of JButtons.
--
Roedy Green Canadian Mind Products
http://mindprod.com
When you get stuck trying to solve a computer program:
1. Go into the kitchen and make coffee.
2. If that fails, go for a walk.
3. If that fails, take a nap.
Why? To avoid being swamped with details, to see the big picture,
to allow in some random noise to kick you out of your thinking rut.

 

[bilsch]

It was the Q1 parts of your code that really tipped me off. I suppose
you could be planning on a future Q1 class, but it did make it look like
you where taking a course over the summer.

BTW, another way to improve you code is to watch the big cascades of
if-else statements. For example this:

if (btn == ".") {strng1 += btn;}
else if (btn == "0") {strng1 += btn;}
else if (btn == "1") {strng1 += btn;}
else if (btn == "2") {strng1 += btn;}
else if (btn == "3") {strng1 += btn;}
else if (btn == "4") {strng1 += btn;}
else if (btn == "5") {strng1 += btn;}
else if (btn == "6") {strng1 += btn;}
else if (btn == "7") {strng1 += btn;}
else if (btn == "8") {strng1 += btn;}
else if (btn == "9") {strng1 += btn;}

could all just be

strng1 += btn;

because you don't do anything different for the different cases.
Copy-paste statement like this should be avoid, because it creates
relatively low value redundancy. Try to combine common cases into one
block. It's easier to go back and modify later.

Now that you mention it I see how that would work. However the actual
program has many non-numeric buttons I don't want in the string - I
better leave that alone for the present.

Thanks. Bill S.

 

[bilsch]

Thanks for the help. I could swear I tried that first but got error
messages about static and non-static conflict problem. i will be taking
Java in fall quarter. Right now I'm working from "Learn Java in 24
Hours" I thought up the calculator project myself. Thanks again.

There are a few mistakes in your code.

You don't need to call 'super()' in the constructor explicitly.
That's what happens by default anyway.

You called the constructor directly from the 'main()' routine. That means
you called it from the primary thread of the program. You don't know this
yet, probably, unless you've already studied concurrency in Java a little bit.

The problem is that the GUI won't work right if you do that. You have to
move GUI actions onto the "Event Dispatch Thread" (EDT), a background
thread that the system creates to handle all GUI actions.

Also, you start all the action from the constructor. That's bad. As its name
implies, a constructor's purpose is to _construct_ an object, not run its logic.
Run the logic after construction completes and the instance is no longer in a
partially-built state.

And make your indentation consistent with the Java coding conventions (available
on line).

So all together, you'd do something like:

public static void main(String[] arguments) {
java.awt.EventQueue.invokeAndWait( new Runnable() {
@Override public void run() {
CalcGUIQ1 calculator = new CalcGUIQ1();
calculator.setVisible(true);
}
});
}

Many things are because I mimic what I see in other programs. Your
advise is appreciated and noted. I know my indentation is wrong - it is
something I'll have to work on. I hope I can finish this project without
understanding threads because I read about them and I don't see that
they apply here. I don't really understand them.

 

[Lew]

You called the constructor directly from the 'main()' routine. That means
you called it from the primary thread of the program. You don't know this
yet, probably, unless you've already studied concurrency in Java a little bit.

The problem is that the GUI won't work right if you do that. You have to
move GUI actions onto the "Event Dispatch Thread" (EDT), a background
thread that the system creates to handle all GUI actions.

Also, you start all the action from the constructor. That's bad. As its name
implies, a constructor's purpose is to _construct_ an object, not run its logic.
Run the logic after construction completes and the instance is no longer in a
partially-built state.

And make your indentation consistent with the Java coding conventions (available
on line).

So all together, you'd do something like:

public static void main(String[] arguments) {
java.awt.EventQueue.invokeAndWait( new Runnable() {
@Override public void run() {
CalcGUIQ1 calculator = new CalcGUIQ1();
calculator.setVisible(true);
}
});
}

Many things are because I mimic what I see in other programs. Your
advise is appreciated and noted. I know my indentation is wrong - it is
something I'll have to work on. I hope I can finish this project without
understanding threads because I read about them and I don't see that
they apply here. I don't really understand them.

Thread issues do apply here, as I explained and you quoted:

The problem is that the GUI won't work right if you do that [make GUI calls
from the main thread]. You have to
move GUI actions onto the "Event Dispatch Thread" (EDT), a background
thread that the system creates to handle all GUI actions.

Mimicking bad code is not good practice.

More recent references, including particularly the (gasp!) Java Swing tutorial
give better examples.

Try
<http://docs.oracle.com/javase/tutorial/uiswing/concurrency/index.html>

 

[bilsch]

you code will simplify if use use a array of JButtorns or some
subclass of JButtons.

I realize there are better alternatives. I don't know any right
offhand. I have many more buttons than shown in the example.
Thanks for the advise. I like your website.

 

[Gene Wirchenko]

On 6/25/2012 6:10 PM, markspace wrote:
[snip]

BTW, another way to improve you code is to watch the big cascades of
if-else statements. For example this:

[snip]

because you don't do anything different for the different cases.
Copy-paste statement like this should be avoid, because it creates
relatively low value redundancy. Try to combine common cases into one
block. It's easier to go back and modify later.

Now that you mention it I see how that would work. However the actual
program has many non-numeric buttons I don't want in the string - I
better leave that alone for the present.

Actually, grouping would help. If you know that more than one
case will be handled the same way, then you just have to concern
yourself with code in one place. Suppose you later find that you have
to change that code. One place vs. many. The former is much easier.
Your code will also be shorter.

Sincerely,

Gene Wirchenko

 

[Gene Wirchenko]

[snip]

Many things are because I mimic what I see in other programs. Your

Bear in mind that there is a lot of not-so-good code out there.
There is also some very good code.

advise is appreciated and noted. I know my indentation is wrong - it is
something I'll have to work on. I hope I can finish this project without

Adopt an indentation style whether it is your own and one that
you see that you are comfortable with. Consistent indenting can help
you catch errors.

understanding threads because I read about them and I don't see that
they apply here. I don't really understand them.

Since this is your own problem, make that part of the spec.
Threads are where you have more than one path of execution. For a
calculator, you do not need that for the basic functionality. Once
you do get your calculator working, then you can add to it or start
another project in order to learn threads.

Sincerely,

Gene Wirchenko

 

[Lew]

A couple of things. First, even if true, it's better to do something
like this:

if( btn == "1" || btn == "2" || btn == "3" ... )
{
// one single case here...
}

Than it is to use many different if-blocks. Same action for different
inputs, you want to use one code block to implement that action.

One other important point I'd like to make is that Java strings don't
normally compare with ==. You have to use .equals() instead. Your code
works now because all of the strings are in a single file, but as your
program grows == will no longer work for you.

This is the normal, and more correct, way to do it:

if( btn.equals( "1" ) || btn.equals( "2" ) || ... )
{
strng1 += btn;
}

switch (btn)
{
case "1":
case "2":
doCaseOneAndTwo();
break;

case "3":
doCaseThree();
break;

default:
doAllOtherCases();
break;
}

Lastly, given your specific use case, there's a cheap quick way to cut
down on verbosity. It involves knowing the API well, but String and
Math (and a few others) are two APIs that you should memorize eventually
to be a good Java programmer. (Other APIs it's OK to have to consult
the documentation periodically.)

Also you should be very, very familiar with the collections classes
(mostly in java.util.*).

 

[Lew]

bilsch wrote:

[snip]

Many things are because I mimic what I see in other programs. Your

Bear in mind that there is a lot of not-so-good code out there.
There is also some very good code.

advise is appreciated and noted. I know my indentation is wrong - it is
something I'll have to work on. I hope I can finish this project without

Adopt an indentation style whether it is your own and one that
you see that you are comfortable with. Consistent indenting can help
you catch errors.

It is best to use the Java Coding Conventions or something very close.

There are a couple of very limited acceptable variations from those
conventions.

Since this is your own problem, make that part of the spec.
Threads are where you have more than one path of execution. For a
calculator, you do not need that for the basic functionality. Once

For a Swing program you always need them.

you do get your calculator working, then you can add to it or start
another project in order to learn threads.

They're already using Swing, requiring that at least they know how to
push GUI actions onto the EDT.

 

[Lew]

Yeah, that's an issue. When teaching someone, is it better to let them
do it the simplest, but wrong, way? Or should you teach them to write
code they don't understand, but promise too "explain it later?"

I'm actually in the latter camp at the moment. I'd rather see you copy
and paste good code, than let you get into the habit of writing bad code
that you feel you understand. It a style of learning thing, more than a
programming thing, but I think it's better to get you into the habit of
looking at correct code now, so it doesn't seem strange when you see the
correct version later.

OTOH, it's not going to adversely effect your small programs. The
chance of you hitting a threading error in such a small program is
virtually nil. Just be aware that: 1. the code really is wrong, and 2.
you'll have to learn threads eventually.

They've already hit a threading error.

How is that "virtually nil" chance?

 

[Arne Vajhøj]

switch (btn)
{
case "1":
case "2":
doCaseOneAndTwo();
break;

case "3":
doCaseThree();
break;

default:
doAllOtherCases();
break;
}

If Java version >= 7.

Arne

 

[Arne Vajhøj]

They've already hit a threading error.

How is that "virtually nil" chance?

Usually "hit [by] an error" means actually impacted by the error.

I did not see any case of such.

Arne

 

[Lew]

They've already hit a threading error.

How is that "virtually nil" chance?

Usually "hit [by] an error" means actually impacted by the error.

I did not see any case of such.

Have you run the OP's code?

On a multi-processor system?

 

[Lew]

If Java version >= 7.

Which the OP had better be using.

 

[Gene Wirchenko]

bilsch wrote:

[snip]

Many things are because I mimic what I see in other programs. Your

Bear in mind that there is a lot of not-so-good code out there.
There is also some very good code.

advise is appreciated and noted. I know my indentation is wrong - it is
something I'll have to work on. I hope I can finish this project without

Adopt an indentation style whether it is your own and one that
you see that you are comfortable with. Consistent indenting can help
you catch errors.

It is best to use the Java Coding Conventions or something very close.

There are a couple of very limited acceptable variations from those
conventions.

That is your religion.

For a Swing program you always need them.

Ah, then my advice is to use it minimally for this project then
....

They're already using Swing, requiring that at least they know how to
push GUI actions onto the EDT.

KIS for now.

Sincerely,

Gene Wirchenko