Win a copy of The Java Performance Companion this week in the Performance forum!
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic

a challenge; contest if you will

 
Randall Twede
Ranch Hand
Posts: 4439
3
Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
a lot of you think the best way to handle action events is a separate anonymous inner class for each button. i challenge this. since java 7 i can have a switch statement that uses Strings. i think my code is shorter, more readable and more maintainable. i also contend it is just as object oriented. as for the ButtonListener class or the actionPerformed() method being too long, it is only two local variables and a switch statement.

so here is the challenge. i will post my code. if someone wants to rewrite it to use anonymous inner classes, we can let JavaRanch decide which they prefer.

 
Rob Spoor
Sheriff
Pie
Posts: 20546
57
Chrome Eclipse IDE Java Windows
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I definitely wouldn't use anonymous inner classes for each buttons here. A lot of the buttons share functionality, so I'd definitely use that. A quick untested example:
Unfortunately, 1/x and sqrt are the only ones remaining that actually share code, although you'd need an abstract method for that.

Some hints about your existing code:
1) Change isNegative = isNegative ? false : true; into isNegative = !isNegative;
2) If you decide to stick with the large switch statement, you can group "+", "-", "*", "%" and "/" like this:
 
Randall Twede
Ranch Hand
Posts: 4439
3
Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
i take that as 1/0 in favor of my code.
thanks rob for the help optimizing my code.
i changed it because i thought the binary() method and the operator variable should be in the inner class not the outer class.
i will also use Rob's changes. i will post the new when i can get online with my laptop. im using the state's computer now. the challenge still stands.
 
Matthew Brown
Bartender
Posts: 4567
8
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Randall Twede wrote:i take that as 1/0 in favor of my code.
thanks rob for the help optimizing my code.
i changed it because i thought the binary() method and the operator variable should be in the inner class not the outer class.
i will also use Rob's changes. i will post the new when i can get online with my laptop. im using the state's computer now. the challenge still stands.

How do you interpret Rob posting an alternative that doesn't have all the different responses in a single event handler - that, crucially, follows the important principle of cohesion (unlike your code - which is why I'd say it's not as object-oriented) - as 1/0 in your favour?
 
Randall Twede
Ranch Hand
Posts: 4439
3
Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
it sounded like he agreed.

i'll rewrite it taking rob's observations in mind

 
Paul Clapham
Sheriff
Posts: 21126
32
Eclipse IDE Firefox Browser MySQL Database
  • Likes 3
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Randall Twede wrote:it sounded like he agreed.


Yes, it did. And yet if you look at the code that Rob posted after he "agreed", there are two ActionListeners there.

You're making this into a contest between "15 ActionListeners" and "1 ActionListener". In my opinion (like what Rob posted) neither is the winner. The right answer is a number around 3 or 4.
 
Matthew Brown
Bartender
Posts: 4567
8
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Paul Clapham wrote:You're making this into a contest between "15 ActionListeners" and "1 ActionListener". In my opinion (like what Rob posted) neither is the winner. The right answer is a number around 3 or 4.

3 or 4 ActionListener classes, but there would be about 15 instances in Rob's version.

The point I'd come back to is that ActionListeners, like any other class, should be cohesive. They should do one thing. A single class handling 10 events that differ only in the number they print is great - just parameterise the class. I don't think anyone is arguing for separate anonymous listeners in that case, as you'd have needless repetion (Don't Repeat Yourself). I think the original challenge is misleading.
 
Randall Twede
Ranch Hand
Posts: 4439
3
Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
cohesion...i fail to see how my code is not cohesive. perhaps you could point it out to me. i really should read code people post. Rob's code addresses the one thing i didn't like about mine. the repeated code "if (!text.equals(""))"

ActionListeners, like any other class, should be cohesive. They should do one thing.

i argue that it does do one thing. it handles the action events

i might change it to two event handlers though to get rid of the repeated if's

the only other "problem" i see is, from a MVC point of view, i am mixing the model, view, and controller in my event handler. however it is a very small program.
 
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic