Design question - methods calling methods

R

Rhino

Is it bad design for any reason for methods in one class to call other
methods in the same class to do some of their work for them? I'm pretty
sure the answer is no but this is just a sanity check to make sure I'm not
forgetting an important factor.

The situation that inspired the question is getLocales() method I've been
discussing in another thread. In addition to the getLocales() method, I
have a displayLocales() method. The displayLocales() method has no input
parameters and returns nothing, it simply writes to the console. It begins
by executing getLocales(), which returns a TreeMap, and then displays the
contents of the TreeMap on the console.

It seems absolutely reasonable to me that the getLocales() method gets
called to obtain the actual Locales, rather than repeating the code from
getLocales() again in displayLocales. If there is a problem with getLocales
(), it obviously affects BOTH methods but it only needs to be fixed in one
place to solve the problem for both methods which seems sensible to me.

I'm not nearly as familiar with source code of the Java API as some of you
seem to be but it seems to me that I've seen methods calling other methods
in some of my debugging in Eclipse too. So this is not a problem, right?
 
A

Arne Vajhøj

Is it bad design for any reason for methods in one class to call other
methods in the same class to do some of their work for them? I'm pretty
sure the answer is no but this is just a sanity check to make sure I'm not
forgetting an important factor.

It is very bad design never to do it.

Calling other methods is the standard way to keep the size of methods
down (and thereby improving readability).

Arne
 
E

Eric Sosman

Is it bad design for any reason for methods in one class to call other
methods in the same class to do some of their work for them? I'm pretty
sure the answer is no but this is just a sanity check to make sure I'm not
forgetting an important factor.

It's perfectly normal for one method to "re-use" another method's
code by calling that other method.

There is, however, one thing to watch out for. If the called
method can be overridden in a subclass, then the calling method
might not execute the code it wanted, but the subclass' code instead.
This may be all right (if a superclass calls hashCode() on an
instance of a subclass, it usually *wants* the subclass' hashCode()
and not its own), but can be surprising and perhaps troublesome if
you weren't expecting it. It's *especially* bad in a constructor,
where the superclass' constructor may wind up calling subclass code
before the subclass' constructor has finished!

Get a copy of "Effective Java" by Joshua Bloch, and read what
he says about designing for inheritance. Read the rest of the book,
too: It's good for the soul.
The situation that inspired the question is getLocales() method I've been
discussing in another thread. In addition to the getLocales() method, I
have a displayLocales() method. The displayLocales() method has no input
parameters and returns nothing, it simply writes to the console. It begins
by executing getLocales(), which returns a TreeMap, and then displays the
contents of the TreeMap on the console.

This seems reasonable. A subclass that overrides getLocales()
presumably does so because it wants to do something special, like
filter out Locales for which you don't have fonts installed. If so,
you'd presumably want to get the restricted Locale list rather than
the one the superclass would provide, so you'd want to use the
subclass' version of getLocales(). All seems well here -- but other
circumstances might be less benign.
 
R

Rhino

It's perfectly normal for one method to "re-use" another method's
code by calling that other method.
Good. At least I don't have to feel guilty for doing that :)
There is, however, one thing to watch out for. If the called
method can be overridden in a subclass, then the calling method
might not execute the code it wanted, but the subclass' code instead.
This may be all right (if a superclass calls hashCode() on an
instance of a subclass, it usually *wants* the subclass' hashCode()
and not its own), but can be surprising and perhaps troublesome if
you weren't expecting it. It's *especially* bad in a constructor,
where the superclass' constructor may wind up calling subclass code
before the subclass' constructor has finished!
Darn, always a gotcha lurking somewhere in the forest ;-)
Get a copy of "Effective Java" by Joshua Bloch, and read what
he says about designing for inheritance. Read the rest of the book,
too: It's good for the soul.
I've heard a lot of good things about that book. Unfortunately, money is
tight right now so it's going to have to wait a bit. But I will try to
get it - and read it - when circumstances permit.....
This seems reasonable. A subclass that overrides getLocales()
presumably does so because it wants to do something special, like
filter out Locales for which you don't have fonts installed. If so,
you'd presumably want to get the restricted Locale list rather than
the one the superclass would provide, so you'd want to use the
subclass' version of getLocales(). All seems well here -- but other
circumstances might be less benign.

Okay, fair enough. Even the most reasonable of actions can have
unexpected consequences at times. I suppose that's a big part of the
reason we test as thoroughly as we can, to catch that kind of thing. You
can't anticipate EVERYTHING.
 
R

Rhino

It is very bad design never to do it.

Calling other methods is the standard way to keep the size of methods
down (and thereby improving readability).

Good! I will do so whenever I can, in good conscience!
 
L

Lew

Good! I will do so whenever I can, in good conscience!

Better would be to do so whenever it makes sense. Just because you can is not
a valid engineering justification.
 
L

Lew

Rhino said:
... getLocales(), which returns a TreeMap, and then displays the
contents of the TreeMap on the console.

It's mostly a good idea to declare variables as an interface type rather than
a concrete type. The rule of thumb is that you declare the variable with the
loosest possible type that has the behavioral contract you need.

Thus, you probably want 'getLocales()' to return a 'SortedMap', as someone
suggested in another thread, unless there's something specific about 'TreeMap'
in particular that requires you use only that type or a subtype thereof.
 
A

Arne Vajhøj

Good! I will do so whenever I can, in good conscience!

Pick a good balance.

500 lines methods are not good.

But no methods > 3 lines is not good either.

Arne
 
R

Robert Klemme

It's mostly a good idea to declare variables as an interface type rather
than a concrete type. The rule of thumb is that you declare the variable
with the loosest possible type that has the behavioral contract you need.

Thus, you probably want 'getLocales()' to return a 'SortedMap', as
someone suggested in another thread, unless there's something specific
about 'TreeMap' in particular that requires you use only that type or a
subtype thereof.

Adding to that: Depending on what DisplayLocales does it might be
reasonable to give it an argument of Iterable<Locale> so it can display
Locale instances regardless from where they were obtained. If
displayLocales() does extensive formatting of Locale instances for
display, you might even cut your methods differently. If, on the
contrary you just need this one way to obtain and display Locales it is
most reasonable to have it like OP did it.

It's not that there is the one and only reasonable solution to
distribution of functionality across methods but it always depends on
circumstance. Often this means that during the life time of an
application things need to be refactored to adjust to changed requirements.

Kind regards

robert
 
E

Eric Sosman

Eric Sosman said:
[...]
Get a copy of "Effective Java" by Joshua Bloch, and read what
he says about designing for inheritance. Read the rest of the book,
too: It's good for the soul.
I've heard a lot of good things about that book. Unfortunately, money is
tight right now so it's going to have to wait a bit. But I will try to
get it - and read it - when circumstances permit.....

Excellent. Shouldn't take more than two or three years to
scrape together the thirty bucks.
 
R

Robert Klemme

Eric Sosman said:
[...]
Get a copy of "Effective Java" by Joshua Bloch, and read what
he says about designing for inheritance. Read the rest of the book,
too: It's good for the soul.
I've heard a lot of good things about that book. Unfortunately, money is
tight right now so it's going to have to wait a bit. But I will try to
get it - and read it - when circumstances permit.....

Excellent. Shouldn't take more than two or three years to
scrape together the thirty bucks.

1st edition is even cheaper:

http://www.amazon.com/gp/offer-listing/0201310058/ref=tmm_pap_used_olp_0?ie=UTF8&condition=used

robert
 
L

Lew

On 05/22/2010 08:14 AM,
On 22.05.2010 13:45,
Robert said:

I wonder if the public library near Rhino has a copy, or if he knows *any*
other Java programmers.

Amazon lists used copies of the current edition from $36.92 used, up to $42.89
new. That'll break the bank!

Now all Rhino has to do is get together with three-Java programming friends
and share the cost. At under $13.25 each I feel quite sure Rhino can afford
to read the book, right, Rhino? Hm?

Or he could go to one of those bookstores with a coffee shop and read it in
the store for free.

The excuse "I can't afford it" is quite weak. Come on, Rhino. If you're
serious about Java programming you'll find a way to read this book, if no
other on the topic, and right soon.

Otherwise it's back to, "Would you like fries with that?"
 
R

Rhino

Lew said:
On 05/22/2010 08:14 AM,
On 22.05.2010 13:45,
I wonder if the public library near Rhino has a copy, or if he knows
*any* other Java programmers.

Amazon lists used copies of the current edition from $36.92 used, up
to $42.89 new. That'll break the bank!

Now all Rhino has to do is get together with three-Java programming
friends and share the cost. At under $13.25 each I feel quite sure
Rhino can afford to read the book, right, Rhino? Hm?

Or he could go to one of those bookstores with a coffee shop and read
it in the store for free.

The excuse "I can't afford it" is quite weak. Come on, Rhino. If
you're serious about Java programming you'll find a way to read this
book, if no other on the topic, and right soon.

Otherwise it's back to, "Would you like fries with that?"
That's not as improbable as you probably think. And that's no joke. I am
indeed serious about Java but I have to eat and pay rent before I can
indulge the luxury of buying a book that I don't absolutely need right
now.... Once I'm gainfully employed again, things will look a lot
different.
 
R

Rhino

Lew said:
Better would be to do so whenever it makes sense. Just because you
can is not a valid engineering justification.

Of course. I didn't mean to do it slavishly to the exclusion of common
sense....
 
L

Lew

Rhino said:
That's not as improbable as you probably think. And that's no joke. I am
indeed serious about Java but I have to eat and pay rent before I can
indulge the luxury of buying a book that I don't absolutely need right
now.... Once I'm gainfully employed again, things will look a lot
different.

That's why I suggested free alternatives.
 
R

Rhino

Lew said:
It's mostly a good idea to declare variables as an interface type
rather than a concrete type. The rule of thumb is that you declare
the variable with the loosest possible type that has the behavioral
contract you need.

Thus, you probably want 'getLocales()' to return a 'SortedMap', as
someone suggested in another thread, unless there's something specific
about 'TreeMap' in particular that requires you use only that type or
a subtype thereof.

For a second there, I had no idea what you were proposing.

But I reviewed the Collection Classes in the Java Tutorial and I think I
get it now.

In all honesty, I hadn't even thought of SortedMap when I wrote the code
again the other day. I just knew that I wanted the result to be in
alphabetical order, not random the way that Locales.getAvailableLocales()
provides them. The article on the Map interface pointed out that TreeMap
would assure that I had alphabetical order so I went with that. Now that
you've reminded me about SortedMap, I can see the merit of it. It's not
much different than TreeMap but it does give those additional features,
like range operations. I don't see those as being _necessary_ for my
humble little getLocales() method, which I'm really just writing for
myself, but some future user of the class could conceivably benefit from
those extra features. Or maybe _I_ will get a benefit from those features
a little further down the road! I've modified the code to produced a
SortedMap - just replaced all "Map" with "SortedMap", dead easy! - and
reran my unit tests. Naturally, they still worked fine.


Hmm. Did I do this right?

I had this:

=========================================================================
public Map<String, Locale> getLocales() {

Map<String, Locale> sortedLocales = new TreeMap<String, Locale>();
for (Locale oneLocale : Locale.getAvailableLocales()) {
sortedLocales.put(oneLocale.getDisplayName(locale), oneLocale);
}

return sortedLocales;
}
========================================================================

and changed it to:

========================================================================
public SortedMap<String, Locale> getLocales() {

SortedMap<String, Locale> sortedLocales = new TreeMap<String, Locale>();
for (Locale oneLocale : Locale.getAvailableLocales()) {
sortedLocales.put(oneLocale.getDisplayName(locale), oneLocale);
}
return sortedLocales;
}
========================================================================

The first line of the revised method looks a bit funny:
SortedMap ... = TreeMap ....

Did I do what you meant?

Sigh! I still struggle with understanding what I am doing sometimes.... I
still don't REALLY understand the difference between these:

- Map<String, Locale> sortedLocales = new TreeMap<String, Locale>();

- SortedMap<String, Locale> sortedLocales = new TreeMap<String, Locale>
();

- Map<String, Locale> sortedLocales = new SortedMap<String, Locale>();

- and so forth

Or, by the same token:

- Calendar cal = Calendar.getInstance();
- Calendar cal = new GregorianCalendar();
- GregorianCalendar gcal = new GregorianCalendar();

If I had to write an exam, clearly articulating the distinction between
those and what the implications of each is, I'd surely make a hash of
it....

I can tell if I'm getting a compile error and I can tell if my code is
doing what I want it to do but I still don't always understand WHY one
thing is better than an other or what the real difference is between
things that look very very similar.....

That doesn't fill me with optimism about my ability to persuade an
employer that I would be a useful addition to their team. I imagine
they're going to want people that know that stuff backwards and
forwards....
 
R

Roedy Green

Is it bad design for any reason for methods in one class to call other
methods in the same class to do some of their work for them? I'm pretty
sure the answer is no but this is just a sanity check to make sure I'm not
forgetting an important factor.

As a general rule, avoid ever cloning code, unless it is just to get a
first cut before you brutally rehash it. You don't want snippets of
code repeated in your project. Sooner or later you will want to change
them, and it is impossible to ensure all are kept in sync.

If later you decide some of the clients need a variant and others do
not, it is lot easier to find all the occurrences if they have been
encapsulated.

The smaller your methods and the greater the reuse, the easier it is
to maintain code. You will only have to change one method. This is the
great truth you learn coding in FORTH.

--
Roedy Green Canadian Mind Products
http://mindprod.com

Beauty is our business.
~ Edsger Wybe Dijkstra (born: 1930-05-11 died: 2002-08-06 at age: 72)

Referring to computer science.
 
T

Tom Anderson

Rhino, read that sentence again.

And that one.

And again.

Now proceed with the rest of this post.
For a second there, I had no idea what you were proposing.

But I reviewed the Collection Classes in the Java Tutorial and I think I
get it now.

In all honesty, I hadn't even thought of SortedMap when I wrote the code
again the other day. I just knew that I wanted the result to be in
alphabetical order, not random the way that Locales.getAvailableLocales()
provides them.

You make that desire concrete by the type of the object holding the
locales - you want them to be sorted, so it's a SortedMap.
The article on the Map interface pointed out that TreeMap would assure
that I had alphabetical order so I went with that. Now that you've
reminded me about SortedMap, I can see the merit of it. It's not much
different than TreeMap but it does give those additional features, like
range operations. I don't see those as being _necessary_ for my humble
little getLocales() method, which I'm really just writing for myself,
but some future user of the class could conceivably benefit from those
extra features. Or maybe _I_ will get a benefit from those features a
little further down the road!

You're on the wrong track here. The extra features are not what SortedMap
is about - it's fundamentally about that first paragraph in its javadoc:

A Map that further provides a total ordering on its keys. The map is
ordered according to the natural ordering of its keys [...] This order is
reflected when iterating over the sorted map's collection views (returned
by the entrySet, keySet and values methods). Several additional
operations are provided to take advantage of the ordering.

Yes, there are several additional operations. But the heart of the matter
is that the map has an order, which governs iteration over its contents.
I've modified the code to produced a SortedMap - just replaced all "Map"
with "SortedMap", dead easy! - and reran my unit tests. Naturally, they
still worked fine.

Hmm. Did I do this right?

I had this:

=========================================================================
public Map<String, Locale> getLocales() {

Map<String, Locale> sortedLocales = new TreeMap<String, Locale>();
for (Locale oneLocale : Locale.getAvailableLocales()) {
sortedLocales.put(oneLocale.getDisplayName(locale), oneLocale);
}

return sortedLocales;
}
========================================================================

and changed it to:

========================================================================
public SortedMap<String, Locale> getLocales() {

SortedMap<String, Locale> sortedLocales = new TreeMap<String, Locale>();
for (Locale oneLocale : Locale.getAvailableLocales()) {
sortedLocales.put(oneLocale.getDisplayName(locale), oneLocale);
}
return sortedLocales;
}
========================================================================

Spot on.
The first line of the revised method looks a bit funny:
SortedMap ... = TreeMap ....

Did I do what you meant?

Yes. You did *exactly* what Lew meant when he said:

It's mostly a good idea to declare variables as an interface type rather
than a concrete type.

SortedMap is an interface type - it says 'this map is sorted somehow'.
TreeMap is a concrete type - it says 'this map is implemented as a
red-black tree, which incidentally results in it being sorted'.
Sigh! I still struggle with understanding what I am doing sometimes.... I
still don't REALLY understand the difference between these:

- Map<String, Locale> sortedLocales = new TreeMap<String, Locale>();

It happens to be a TreeMap, but all the type declares is that it's a map.
- SortedMap<String, Locale> sortedLocales = new TreeMap<String, Locale>();

It happens to be a TreeMap, but all the type declares is that it's a
sorted map.
- Map<String, Locale> sortedLocales = new SortedMap<String, Locale>();

Illegal. SortedMap is an interface.
Or, by the same token:

- Calendar cal = Calendar.getInstance();
- Calendar cal = new GregorianCalendar();
- GregorianCalendar gcal = new GregorianCalendar();

Those last two examples are an exact analogue. The first one is a little
different - but a natural extension of the idea.

The third version says "I care that this is object is specifically a
GregorianCalendar. I am going to do things with it which specifically
would not work with any other kind of calendar.". There are times when you
might want to say that, but you would strive to be more general - in which
case you would use the second version, which says "I care that this object
is a Calendar, but i don't care which kind of calendar - it could be
Gregorian, Islamic, Japanese Imperial, Darian Jovian, etc". However, in
the second form, although you will proceed to write your code
calendar-agnostically, you are actually hardcoding the choice of
GregorianCalendar there. You could change it later, but you'd have to go
through your code and change the constructions. In the first form, you
push the calendar-agnosticism even into the creation itself - rather than
explicitly using a GregorianCalendar, you let Calendar decide what kind of
calendar should be used. Right now, that will presumably always be a
GregorianCalendar, but if at some point someone boots up your software on
Ganymede (the moon, not the Eclipse release), it should probably be
Darian, and by leaving the decision to Calendar.getInstance, you let that
happen without having to alter your code. You delegate the decision to
someone in a better position to make it.

Now, in this particular case, there is a major caveat: different types of
Calendar are not interchangeable in the way that different types of Map
are, because they represent different ideas. If you're using this Calendar
object to handle dates which are definitively in Gregorian (January,
February and all that), then you need an actual GregorianCalendar, and you
should declare your variables accordingly. To paraphrase Einstein,
"everything should be made as generic as possible, but no more so".
If I had to write an exam, clearly articulating the distinction between
those and what the implications of each is, I'd surely make a hash of
it....

That would be a real mistake. You should be making a tree. Have you
learned NOTHING?
I can tell if I'm getting a compile error and I can tell if my code is
doing what I want it to do but I still don't always understand WHY one
thing is better than an other or what the real difference is between
things that look very very similar.....

The choice between Map and SortedMap here is not one of correctness. It's
one of design and style. Good design and style doesn't change the way code
runs, but it makes it easier to understand and modify. A huge part of the
business of software is understanding and modifying existing code, and so
good design and style are valuable. They're investment for the future,
rather than the here and now.

tom
 
L

Lew

Rhino said:
In all honesty, I hadn't even thought of SortedMap when I wrote the code
again the other day. I just knew that I wanted the result to be in
alphabetical order, not random the way that Locales.getAvailableLocales()
provides them. The article on the Map interface pointed out that TreeMap
would assure that I had alphabetical order so I went with that. Now that
you've reminded me about SortedMap, I can see the merit of it. It's not
much different than TreeMap but it does give those additional features,

WTF are you talking about? I got lost in your antecedent-free pronouns. Are
you referring to the methods in 'TreeMap' that are not implementations of
'SortedMap' methods?

As for 'SortedMap' not being "much different [from] TreeMap', well that makes
perfect sense since 'TreeMap' implements 'SortedMap'. OTOH, some might say
that an interface in some ways is always "much different" from a concrete
class, but that difference is the basis of the recommendation to move to a
wider ("looser") type.
like range operations. I don't see those as being _necessary_ for my
humble little getLocales() method, which I'm really just writing for
myself, but some future user of the class could conceivably benefit from
those extra features. Or maybe _I_ will get a benefit from those features
a little further down the road! I've modified the code to produced a
SortedMap - just replaced all "Map" with "SortedMap", dead easy! - and
reran my unit tests. Naturally, they still worked fine.

You're the API writer. YOU dictate what "some future user" gets to do.

It's a best practice to add a lot of "potentially useful" cruft to a type.
Implement what you need now, and refactor later if the need arises.

If you need access the 'TreeMap' methods that aren't part of the 'SortedMap'
interface, then by all means declare the variable that needs that access as
'TreeMap', otherwise don't.
Hmm. Did I do this right?

I had this:

=========================================================================
public Map<String, Locale> getLocales() {

Map<String, Locale> sortedLocales = new TreeMap<String, Locale>();
for (Locale oneLocale : Locale.getAvailableLocales()) {
sortedLocales.put(oneLocale.getDisplayName(locale), oneLocale);
}

return sortedLocales;
}
========================================================================

and changed it to:

========================================================================
public SortedMap<String, Locale> getLocales() {

SortedMap<String, Locale> sortedLocales = new TreeMap<String, Locale>();
for (Locale oneLocale : Locale.getAvailableLocales()) {
sortedLocales.put(oneLocale.getDisplayName(locale), oneLocale);
}
return sortedLocales;
}
========================================================================
Yes.

The first line of the revised method looks a bit funny:
SortedMap ... = TreeMap ....

LOL.

Since 'TreeMap' /is-a/ 'SortedMap', that's perfectly legit. You can, and
often should upcast without danger.
Did I do what you meant?
Yes.

Sigh! I still struggle with understanding what I am doing sometimes.... I
still don't REALLY understand the difference between these:

- Map<String, Locale> sortedLocales = new TreeMap<String, Locale>();

This variable 'sortedLocales' only has access to features promised by the
'Map' interface, not the additional methods of the 'TreeMap' or 'SortedMap' types.

It's also named with a lie, because the code could change to assign an
unsorted 'Map' to the variable without causing a compiler error, thus creating
a problem at run time. You never want to push compile-time-avoidable mistakes
to run time.
- SortedMap<String, Locale> sortedLocales = new TreeMap<String, Locale>
();

The variable 'sortedLocales' has access to the methods of 'SortedMap' but not
those of 'TreeMap' not in the 'SortedMap' type. The name is not a lie,
because any refactoring of the code must use a 'SortedMap' subtype to assign
to the variable.
- Map<String, Locale> sortedLocales = new SortedMap<String, Locale>();

This is "codecrap" as Eric likes to call it. It will not compile.

Are you familiar with the difference between interfaces and classes?
- and so forth

"So forth"? What "so forth"? What do you mean?
Or, by the same token:

These examples are by no means "the same token".
- Calendar cal = Calendar.getInstance();

This does not guarantee that you will get a 'GregorianCalendar' or any other
particular type of 'Calendar'.
- Calendar cal = new GregorianCalendar();

This guarantees that 'cal' points to a 'GregorianCalendar', but the behaviors
specific to 'GregorianCalendar' but not 'Calendar' cannot be accessed through it.
- GregorianCalendar gcal = new GregorianCalendar();

This guarantees that 'gal' points to a 'GregorianCalendar', and the behaviors
specific to 'GregorianCalendar' can be accessed through it.
If I had to write an exam, clearly articulating the distinction between
those and what the implications of each is, I'd surely make a hash of
it....
cal.hashCode()

That doesn't fill me with optimism about my ability to persuade an
employer that I would be a useful addition to their team. I imagine
they're going to want people that know that stuff backwards and
forwards....

No.

Employers hire people at all levels of experience and knowledge, with
appropriate adjustments to compensation, depending on the mix they need for
their team. I know of very few projects that hire only virtuosos, and I've
worked on many that actually hired incompetent programmers.

Which last you aren't, BTW.
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

Forum statistics

Threads
473,962
Messages
2,570,134
Members
46,690
Latest member
MacGyver

Latest Threads

Top