• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Jeanne Boyarsky
  • Ron McLeod
  • Paul Clapham
  • Liutauras Vilda
Sheriffs:
  • paul wheaton
  • Rob Spoor
  • Devaka Cooray
Saloon Keepers:
  • Stephan van Hulst
  • Tim Holloway
  • Carey Brown
  • Frits Walraven
  • Tim Moores
Bartenders:
  • Mikalai Zaikin

a challenge; contest if you will

 
Ranch Hand
Posts: 4716
9
Scala Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • 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.

 
Sheriff
Posts: 22783
131
Eclipse IDE Spring VI Editor Chrome Java Windows
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • 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: 4716
9
Scala Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • 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.
 
Bartender
Posts: 4568
9
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • 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: 4716
9
Scala Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
it sounded like he agreed.

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

 
Marshal
Posts: 28193
95
Eclipse IDE Firefox Browser MySQL Database
  • Likes 3
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • 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: 4568
9
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • 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: 4716
9
Scala Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • 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.
 
With a little knowledge, a cast iron skillet is non-stick and lasts a lifetime.
reply
    Bookmark Topic Watch Topic
  • New Topic