Overloading abstract methods

[Rhino]

I have a question about abstract classes but I'm going to need to give you
some of the background before the question makes much sense. Please bear
with me.

I've decided that my project, which writes my resume in a variety of
different formats - HTML, ASCII, PDF for starters - would benefit from an
abstract class. I'm calling this abstract class ResumeWriter. There will be
one class for each each of the resume formats and each of these will extend
ResumeWriter so I'm calling them ResumeWriterHtml, ResumeWriterAscii and
ResumeWriterPdf. In other words, ResumeWriterHtml extends the abstract
ResumeWriter and writes the HTML version of the resume and so on.

The ResumeWriter abstract class includes:
- a number of concrete getters, one for each of the things that the
ResumeWriterHtml, ResumeWriterAscii, and ResumeWriterPdf classes will get
from the ResourceBundle that supplies the data to all of the classes whose
names start with 'ResumeWriter'
- several abstract write methods, one for each portion of the resume, e.g.
writeCandidateInformation(), writeEmploymentHistory(),
writeSoftwareSkills(). In the subclasses, ResumeWriterHtml et. al., each of
the abstract methods will be implemented to write that portion of the resume
in the appropriate format, e.g. the writeSoftwareSkills() method will write
software skills information in HTML format in ResumeWriterHtml and in PDF
format in ResumeWriterPdf and so on.

I'm reasonably confident that this is a sensible way to do this from a
design point of view and the code already works so I know that this is a
workable approach.

Now, here's where I run into problems. In some of the ResumeWriter
subclasses, the write() methods display slightly different data than in
others. For instance, the ResumeWriterAscii class displays only the heading
"EmploymentHistory" and then an array called "EmploymentHistoryData" but the
ResumeWriterHtml displays additional information, specifically some text
that serve as column titles for the different aspects of each job, namely
"From/To", "Employer", "Role", and "Main Tools Used" for those columns of
the HTML table that displays the employment history.

This suggests to me that I need _two_ abstract methods called
writeEmploymentHistory(); one has only two arguments, EmploymentHistoryText
and EmploymentHistoryData, while the other version also has the same two
arguments plus four additional arguments, FromToText, EmployerText,
RoleText, and MainToolsText. Now, I know that this is simply overloading the
writeEmploymentHistory() method and that this is perfectly fine in an OO
program.

The problem for me is that I apparently have to implement _both_ versions of
writeEmploymentHistory() in each class that subclasses ResumeWriter, even
though only one of the two methods will be used in any given subclass of
ResumeWriter. And that makes me uneasy: I find myself wondering if I am
doing the right thing when I implement multiple versions of the same method
in a given subclass even though I will only use one of them. Implementing
methods that I'm not going to use doesn't seem right somehow.

Can someone who is stronger in OO Analysis and Design help me figure out if
I am doing the right thing here? If not, what should I be doing differently?

One other small issue related to the main problem. In the version of the
overloaded method that I am NOT using, it seems to me that the easiest thing
to do is just leave the method body empty with a comment that it is not
being implemented because it is not appropriate for this subclass. However,
if I do that, I get compiler warnings that the parameters for that method
are never used. I know I could disable those warnings in the compiler but
I'd rather avoid that and just put some sort of do-nothing code in the
method body to satisfy the compiler that the parameters are used, even if
the use is trivial. What is the most trivial thing I can do with each
parameter to satisfy the compiler that the parameter is being used? I'm
looking for something that would be harmless if it were executed and that
would be more-or-less harmless looking to anyone who sees the code, i.e.
something that seems to be obvious do-nothing code.

Rhino

 

[Chris Smith]

I've decided that my project, which writes my resume in a variety of
different formats - HTML, ASCII, PDF for starters - would benefit from an
abstract class. I'm calling this abstract class ResumeWriter. There will be
one class for each each of the resume formats and each of these will extend
ResumeWriter so I'm calling them ResumeWriterHtml, ResumeWriterAscii and
ResumeWriterPdf.

Okay, you're only the seventeen gazillionth person to do this, but I'm
picking on you.

Why?!? Would it have killed you to call them HtmlResumeWriter,
AsciiResumeWriter, and PdfResumeWriter? Have you ever asked a neighbor
to borrow her mower-lawn?

Anyway, just ignore me. Had to get that off my chest.

This suggests to me that I need _two_ abstract methods called
writeEmploymentHistory(); one has only two arguments, EmploymentHistoryText
and EmploymentHistoryData, while the other version also has the same two
arguments plus four additional arguments, FromToText, EmployerText,
RoleText, and MainToolsText. Now, I know that this is simply overloading the
writeEmploymentHistory() method and that this is perfectly fine in an OO
program.

The problem for me is that I apparently have to implement _both_ versions of
writeEmploymentHistory() in each class that subclasses ResumeWriter, even
though only one of the two methods will be used in any given subclass of
ResumeWriter.

There's no problem with giving an object more than it needs. What you
want is, most fundamentally, one method called writeEmploymentHistory,
which takes as parameters all of the information that might be useful
for writing an employment history. Which information you choose to make
use of in each subclass is up to you.

One other small issue related to the main problem.

Actually, it turns out I think this is more related to the problem than
you may think.

In the version of the
overloaded method that I am NOT using, it seems to me that the easiest thing
to do is just leave the method body empty with a comment that it is not
being implemented because it is not appropriate for this subclass.

If you decide on this design against my recommendations, the better
thing to do would be to throw an exception... an unchecked one, since
this is a programming error.

However, if I do that, I get compiler warnings that the parameters for
that method are never used. I know I could disable those warnings in
the compiler but I'd rather avoid that

Why? Disable that warning. That warning is counterproductive to good
programming technique. It will cause you to do a worse job than if you
leave it enabled.

If the warning only warned about unused parameters for an entire
hierarchy of types in unpublished APIs, then I could see its uses. What
it's doing, though -- warning you for not using a parameter in one
method when you ARE using it within that method signature in the type
hierarchy -- is counterproductive, and encourages poor code... like this
throw away "trick the compiler" nonsense you're asking for.

I know it sounds like rehashing the same thing, but you're really
providing a perfect example now of why you should NOT turn on random
warnings just because some compiler vendor saw fit to give you the
option (and leave it OFF by default). I imagine I could really make
your life miserable by patching your copy of Eclipse to add an optional
warning if you use more than 16 characters in an identifier.

and just put some sort of do-nothing code in the
method body to satisfy the compiler that the parameters are used, even if
the use is trivial. What is the most trivial thing I can do with each
parameter to satisfy the compiler that the parameter is being used?

Is this Java 1.5? If so, @SuppressWarnings("unused").

If it's not Java 1.5, and if you can't be dissuaded from doing this,
then I guess you could declare a temporary variable of the same type,
assign the value of the parameter to the parameter to that temporary
variable, and then assign the temporary variable back to the parameter.
Like this:

public void myMethod(int unusued)
{
int tmp = unused;
unused = tmp;
}

(The second bit is to prevent another warning about an unread local
variable.)

Then again, with any luck Eclipse will soon gain a warning for assigning
to a parameter (which WOULD be a good one to leave on) and you'll
trigger that one. Or its control flow will get smarter and you'll have
to get more complex to trick it. Eventually, someone might get wise
enough to use escape analysis for that warning, and then you'll be
really snookered.

Do you care about the standard output and error streams? I guess you
could print the values there, and that would sure take care of it... but
you'd get weird output if you ran the app from the command line.

--
www.designacourse.com
The Easiest Way To Train Anyone... Anywhere.

Chris Smith - Lead Software Developer/Technical Trainer
MindIQ Corporation

 

[VisionSet]

Okay, you're only the seventeen gazillionth person to do this, but I'm
picking on you.

Why?!? Would it have killed you to call them HtmlResumeWriter,
AsciiResumeWriter, and PdfResumeWriter? Have you ever asked a neighbor
to borrow her mower-lawn?

Anyway, just ignore me. Had to get that off my chest.

LOL, I felt that from this side of the pond!
They would at least sort nicely in (my) IDEs list's of classes.

 

[Raymond DeCampo]

I have a question about abstract classes but I'm going to need to give you
some of the background before the question makes much sense. Please bear
with me.

I've decided that my project, which writes my resume in a variety of
different formats - HTML, ASCII, PDF for starters - would benefit from an
abstract class. I'm calling this abstract class ResumeWriter. There will be
one class for each each of the resume formats and each of these will extend
ResumeWriter so I'm calling them ResumeWriterHtml, ResumeWriterAscii and
ResumeWriterPdf. In other words, ResumeWriterHtml extends the abstract
ResumeWriter and writes the HTML version of the resume and so on.

The ResumeWriter abstract class includes:
- a number of concrete getters, one for each of the things that the
ResumeWriterHtml, ResumeWriterAscii, and ResumeWriterPdf classes will get
from the ResourceBundle that supplies the data to all of the classes whose
names start with 'ResumeWriter'
- several abstract write methods, one for each portion of the resume, e.g.
writeCandidateInformation(), writeEmploymentHistory(),
writeSoftwareSkills(). In the subclasses, ResumeWriterHtml et. al., each of
the abstract methods will be implemented to write that portion of the resume
in the appropriate format, e.g. the writeSoftwareSkills() method will write
software skills information in HTML format in ResumeWriterHtml and in PDF
format in ResumeWriterPdf and so on.

I'm reasonably confident that this is a sensible way to do this from a
design point of view and the code already works so I know that this is a
workable approach.

Now, here's where I run into problems. In some of the ResumeWriter
subclasses, the write() methods display slightly different data than in
others. For instance, the ResumeWriterAscii class displays only the heading
"EmploymentHistory" and then an array called "EmploymentHistoryData" but the
ResumeWriterHtml displays additional information, specifically some text
that serve as column titles for the different aspects of each job, namely
"From/To", "Employer", "Role", and "Main Tools Used" for those columns of
the HTML table that displays the employment history.

This suggests to me that I need _two_ abstract methods called
writeEmploymentHistory(); one has only two arguments, EmploymentHistoryText
and EmploymentHistoryData, while the other version also has the same two
arguments plus four additional arguments, FromToText, EmployerText,
RoleText, and MainToolsText. Now, I know that this is simply overloading the
writeEmploymentHistory() method and that this is perfectly fine in an OO
program.

The problem for me is that I apparently have to implement _both_ versions of
writeEmploymentHistory() in each class that subclasses ResumeWriter, even
though only one of the two methods will be used in any given subclass of
ResumeWriter. And that makes me uneasy: I find myself wondering if I am
doing the right thing when I implement multiple versions of the same method
in a given subclass even though I will only use one of them. Implementing
methods that I'm not going to use doesn't seem right somehow.

I think the way to go is to have your writeXxx() methods either accept
no parameters and access the data via protected methods/attributes of
the base class or accept one universal parameter that contains all the
data a write method could ever want.

Can someone who is stronger in OO Analysis and Design help me figure out if
I am doing the right thing here? If not, what should I be doing differently?

One other small issue related to the main problem. In the version of the
overloaded method that I am NOT using, it seems to me that the easiest thing
to do is just leave the method body empty with a comment that it is not
being implemented because it is not appropriate for this subclass. However,
if I do that, I get compiler warnings that the parameters for that method
are never used. I know I could disable those warnings in the compiler but
I'd rather avoid that and just put some sort of do-nothing code in the
method body to satisfy the compiler that the parameters are used, even if
the use is trivial. What is the most trivial thing I can do with each
parameter to satisfy the compiler that the parameter is being used? I'm
looking for something that would be harmless if it were executed and that
would be more-or-less harmless looking to anyone who sees the code, i.e.
something that seems to be obvious do-nothing code.

As another poster pointed out, don't let compiler warnings bully you
into making busy work.

HTH,
Ray

 

[Roedy Green]

Implementing
methods that I'm not going to use doesn't seem right somehow.

what you can do is implement the method and throw an
UnsupportedOperationException just in case you or someone else
accidentally uses it.

 

[Roedy Green]

Okay, you're only the seventeen gazillionth person to do this, but I'm
picking on you.

Why?!? Would it have killed you to call them HtmlResumeWriter,
AsciiResumeWriter, and PdfResumeWriter? Have you ever asked a neighbor
to borrow her mower-lawn?

Anyway, just ignore me. Had to get that off my chest.

the advantage of doing that weird way is javadoc listings will put the
three classes side by side.

 

[Rhino]

the advantage of doing that weird way is javadoc listings will put the
three classes side by side.

And that was my basic motivation for naming things that way, as it turns
out.

One other reason is something I heard in school many years ago: a teacher
described the ISO standards as putting things like dates, which comprised
several parts, into order on the basis of largest things to smallest things,
e.g. the ISO standard is to put years first, then months, then days. To me,
the abstract class, ResumeWriter is the "larger" thing and the specific type
of document being produced is the "smaller" think; therefore, ResumeWriter
goes first, followed by the type of document, thus giving us things like
ResumeWriterHtml or ResumeWriterAscii.

Then again, I could persuade myself that I've really got three parts in the
name, not two, and by that reasoning, the names should be WriterResumeHtml
and WriterResumeAscii. Something tells me that would cause a lot of raised
eyebrows too

Well, I'm not going to please everyone so I'm going to concentrate on
pleasing myself.

Rhino

 

[Rhino]

Okay, you're only the seventeen gazillionth person to do this, but I'm
picking on you.

Why?!? Would it have killed you to call them HtmlResumeWriter,
AsciiResumeWriter, and PdfResumeWriter? Have you ever asked a neighbor
to borrow her mower-lawn?

Anyway, just ignore me. Had to get that off my chest.

See my explanation in a followup to Roedy's first reply. It probably won't
persuade you but, then again, maybe it will

There's no problem with giving an object more than it needs. What you
want is, most fundamentally, one method called writeEmploymentHistory,
which takes as parameters all of the information that might be useful
for writing an employment history. Which information you choose to make
use of in each subclass is up to you.

Okay, I guess I can buy that as a valid approach. But is it good design? If
I applied that same argument to other classes in the J2SE, then I'd probably
never overload _any_ method: I'd just combine all overloaded methods into a
single big method that had all possible parameters. I'd expect to get
criticized on the grounds that I'm discarding the benefits of overloaded
methods by doing that. Isn't that what you're proposing here? Or is this a
"special case" where that is okay?

Actually, it turns out I think this is more related to the problem than
you may think.


If you decide on this design against my recommendations, the better
thing to do would be to throw an exception... an unchecked one, since
this is a programming error.


Why? Disable that warning. That warning is counterproductive to good
programming technique. It will cause you to do a worse job than if you
leave it enabled.

If the warning only warned about unused parameters for an entire
hierarchy of types in unpublished APIs, then I could see its uses. What
it's doing, though -- warning you for not using a parameter in one
method when you ARE using it within that method signature in the type
hierarchy -- is counterproductive, and encourages poor code... like this
throw away "trick the compiler" nonsense you're asking for.

I know it sounds like rehashing the same thing, but you're really
providing a perfect example now of why you should NOT turn on random
warnings just because some compiler vendor saw fit to give you the
option (and leave it OFF by default). I imagine I could really make
your life miserable by patching your copy of Eclipse to add an optional
warning if you use more than 16 characters in an identifier.


Is this Java 1.5? If so, @SuppressWarnings("unused").

Yes, it is Java 1.5.

If it's not Java 1.5, and if you can't be dissuaded from doing this,
then I guess you could declare a temporary variable of the same type,
assign the value of the parameter to the parameter to that temporary
variable, and then assign the temporary variable back to the parameter.
Like this:

public void myMethod(int unusued)
{
int tmp = unused;
unused = tmp;
}

(The second bit is to prevent another warning about an unread local
variable.)

Then again, with any luck Eclipse will soon gain a warning for assigning
to a parameter (which WOULD be a good one to leave on) and you'll
trigger that one. Or its control flow will get smarter and you'll have
to get more complex to trick it. Eventually, someone might get wise
enough to use escape analysis for that warning, and then you'll be
really snookered.

I'm actually not as determined to write do nothing code as you may think. I
was hoping someone could suggest a way to avoid that, first and foremost.
But I wanted a suggestion for the least offensive way of doing it if I had
to do it.

You make a good case for using the compiler switch to simply ignore unused
variables, at least in this case. I suppose I was looking for some bragging
rights in saying that I am not ignoring any warnings or errors and have an
absolutely warning-free compile. But I agree that this is just going to lead
to poor code in this case. And, as you say, the next version of the compiler
will probably find ways to detect any little tricks that I play on the
current version of the compiler and make me change the code eventually.

Do you care about the standard output and error streams? I guess you
could print the values there, and that would sure take care of it... but
you'd get weird output if you ran the app from the command line.

Interesting. That's exactly what I came up with on my own: I just did a
System.out.println() of each parameter. I'm not even worried about the code
executing because the unused method simply isn't called anywhere within its
class. Of course, if I subclassed that class, there'd be the risk that the
subclass could execute it.

Rhino

 

[Chris Smith]

Well, I'm not going to please everyone so I'm going to concentrate on
pleasing myself.

As I said, you can ignore that.

Hope the rest of the response was helpful.

--
www.designacourse.com
The Easiest Way To Train Anyone... Anywhere.

Chris Smith - Lead Software Developer/Technical Trainer
MindIQ Corporation

 

[Chris Smith]

Okay, I guess I can buy that as a valid approach. But is it good design? If
I applied that same argument to other classes in the J2SE, then I'd probably
never overload _any_ method: I'd just combine all overloaded methods into a
single big method that had all possible parameters. I'd expect to get
criticized on the grounds that I'm discarding the benefits of overloaded
methods by doing that. Isn't that what you're proposing here? Or is this a
"special case" where that is okay?

Actually, you can overload or not... it doesn't matter all that much.
The change in design is that you provide one point of implementation for
subclasses to customize the behavior of the superclass. And yes, it is
good design. For example, here's something with overloads.

public abstract class ResumeWriter
{
public abstract void writeEmploymentHistory(
String employmentHistoryText,
String employmentHistoryData,
String fromToText,
String employerText,
String roleText,
String mainToolsText);

public final void writeEmploymentHistory(
String employmentHistoryText,
String employmentHistoryData)
{
writeEmploymentHistory(
employmentHistoryText, employmentHistoryData,
null, null, null, null);
}
}

You still need only override the method that receives all the data, and
just use what you need.

I'm not exactly clear on how you call this method, though. I would
think that from the calling context, you'd just forget about what kind
of resume you're writing, and pass all the information you've got. Let
the subclass sort out what it uses and what it doesn't use. In that
case, you'd never use the overload anyway. The change here isn't just
to reuse the same method from two contexts... it's to design your code
as much as possible to stop caring about the context in the first place.

Yes, it is Java 1.5.

So that's the solution to the unused parameter warning, then, eve if you
do leave it on. You'll have to deal with that regardless of which
design decision you've made.

--
www.designacourse.com
The Easiest Way To Train Anyone... Anywhere.

Chris Smith - Lead Software Developer/Technical Trainer
MindIQ Corporation

 

[Rhino]

As I said, you can ignore that.

I wasn't trying to be defensive, I just wanted to make the case for the
approach I was using

Hope the rest of the response was helpful.

It certainly was!

Rhino

 

[Rhino]

Actually, you can overload or not... it doesn't matter all that much.
The change in design is that you provide one point of implementation for
subclasses to customize the behavior of the superclass. And yes, it is
good design. For example, here's something with overloads.

public abstract class ResumeWriter
{
public abstract void writeEmploymentHistory(
String employmentHistoryText,
String employmentHistoryData,
String fromToText,
String employerText,
String roleText,
String mainToolsText);

public final void writeEmploymentHistory(
String employmentHistoryText,
String employmentHistoryData)
{
writeEmploymentHistory(
employmentHistoryText, employmentHistoryData,
null, null, null, null);
}
}

You still need only override the method that receives all the data, and
just use what you need.

I'm not exactly clear on how you call this method, though. I would
think that from the calling context, you'd just forget about what kind
of resume you're writing, and pass all the information you've got. Let
the subclass sort out what it uses and what it doesn't use. In that
case, you'd never use the overload anyway. The change here isn't just
to reuse the same method from two contexts... it's to design your code
as much as possible to stop caring about the context in the first place.

For what it's worth, I've decided not to overload the method after all; I'll
just have a single version of writeEmploymentHistory() and pass it all the
parameters it will ever need, even if some parameters don't get used in some
subclasses of ResumeWriter.

So that's the solution to the unused parameter warning, then, eve if you
do leave it on. You'll have to deal with that regardless of which
design decision you've made.

You sold me on using the @SuppressWarnings annotation. I'd been meaning to
start using annotations in the near future anyway and your note gave me the
impetus to read up about the basics. I really like that I can apply
annotations in a very narrow scope, such as a single method of a given
class, rather than having to turn off the unused variable detection for all
classes throughout my IDE.

Rhino

 

[Chris Smith]

You sold me on using the @SuppressWarnings annotation. I'd been meaning to
start using annotations in the near future anyway and your note gave me the
impetus to read up about the basics. I really like that I can apply
annotations in a very narrow scope, such as a single method of a given
class, rather than having to turn off the unused variable detection for all
classes throughout my IDE.

Yep. I'd like still narrower scope, though... especially for
@SuppressWarnings("unchecked"). This particular annotation ought to be
able to be applied to a statement, rather than just a method or other
declaration. Except, of course, that this isn't what annotations are
really designed for.

--
www.designacourse.com
The Easiest Way To Train Anyone... Anywhere.

Chris Smith - Lead Software Developer/Technical Trainer
MindIQ Corporation

 

[Chris Uppal]

For what it's worth, I've decided not to overload the method after all;
I'll just have a single version of writeEmploymentHistory() and pass it
all the parameters it will ever need, even if some parameters don't get
used in some subclasses of ResumeWriter.

That's not too bad, but it would be better still not to try to work out what
"all the parameters it will ever need" are. Just pass[*] it the source of
/all/ the data and let the individual implementations pull what they require
out of it.

(by "pass" I don't necessarily mean pass as a parameter -- since your raw data
is held as part of the object's state, the writeEmploymentHistory() method is
entitled to use whatever bits of that it happens to need)

-- chris

 

[Rhino]

For what it's worth, I've decided not to overload the method after all;
I'll just have a single version of writeEmploymentHistory() and pass it
all the parameters it will ever need, even if some parameters don't get
used in some subclasses of ResumeWriter.

That's not too bad, but it would be better still not to try to work out
what
"all the parameters it will ever need" are. Just pass[*] it the source of
/all/ the data and let the individual implementations pull what they
require
out of it.

(by "pass" I don't necessarily mean pass as a parameter -- since your raw
data
is held as part of the object's state, the writeEmploymentHistory() method
is
entitled to use whatever bits of that it happens to need)

This is an intriguing idea but I think it would be counterproductive in my
case.

I've gone ahead and reworked the resume classes that we discussed in a
thread just before Christmas. I was persuaded by several of the people who
responded, including you, that I should take advantage of the fact that each
version of the resume would have several common elements and create an
abstract class that recognized that; then use subclasses of the abstract
class to generate each specific format of the resume, e.g. HTML, PDF, ASCII.

All of the information for the resume is coming from a ResourceBundle.
(Currently, I am only creating an English-language resume but I thought I'd
use ResourceBundles in case I decided to generate other languages as well;
that would be dead easy since I'd only have to copy my English
ResourceBundle and then translate the relevant bits of in the second
ResourceBundle.)

I realized that each of the resumes executed _exactly_ the same code to get
the desired information, such as my name, from the ResourceBundle,
regardless of the format of the resume being generated. I also realized that
each resume _wrote_ that information in its respective file _differently_
from the other files. For example, the HTML version of the resume would
write this to display my name:

<h1>RHINO</h1>

while the ASCII version of the resume would write this left-justified:

RHINO

and the PDF version of the resume would write this centered:


RHINO


As a result I created the abstract class, ResumeWriter, with _concrete_
methods to _get_ the contents of the resume from the ResourceBundle and
_abstract_ methods to _write_ those contents in the desired format.
Therefore, ResumeWriter looks like this (I'm including only the code needed
for the first bit of the resume; that's all I need for illustrating my
point):

-------------------------------------------------------------------------------------------------------------------
public abstract class ResumeWriter{

public String getCandidateNameData(ResourceBundle locList) {

String candidateNameData = locList.getString("CandidateNameData");
return candidateNameData;
}

public String[] getCandidateAddressData(ResourceBundle locList) {

String[] candidateAddressData = (String[])
locList.getObject("CandidateAddressData");
return candidateAddressData;
}

public String getJobTargetText(ResourceBundle locList) {

String jobTargetText = locList.getString("JobTargetText");
return jobTargetText;
}

public String getJobTargetData(ResourceBundle locList) {
String jobTargetData = locList.getString("JobTargetData");
return jobTargetData;
}

public abstract void writeCandidateAndTarget(String candidateNameData,
String[] candidateAddressData, String jobTargetText, String jobTargetData);

-------------------------------------------------------------------------------------------------------------------

The ResumeWriterHtml class looks like this (with most of the irrelevancies
omitted):

-------------------------------------------------------------------------------------------------------------------
public class ResumeWriterHtml extends ResumeWriter implements
ResumeConstants, ResumeGeneratorConstants {

private final String CLASS_NAME = getClass().getName();

private PrintWriter pw = null;

/**
* Creates an HTML version of a resume.
*
* @param strOutfile the name of the file which will contain the HTML
version of the resume
* @param locList the ResourceBundle which contains the data that will
be written on the resume
*/
public ResumeWriterHtml(String strOutfile, ResourceBundle locList) {

/* Delete the old version of the output file. */
ResumeUtilities.deleteFile(strOutfile);

/* Open the output file. */
this.pw = ResumeUtilities.openOutputFile(strOutfile);

/* Write the head of the page. */
writeHead();

/* Get and write the candidate name, address, and job target
information. */
String candidateNameData = getCandidateNameData(locList);
String[] candidateAddressData = getCandidateAddressData(locList);
String jobTargetText = getJobTargetText(locList);
String jobTargetData = getJobTargetData(locList);
writeCandidateAndTarget(candidateNameData, candidateAddressData,
jobTargetText, jobTargetData);

}


/**
* Writes the resume lines that show the candidate name, address, and
job target in HTML format.
*
* @param candidateNameData the name of the candidate
* @param candidateAddressData the address of the candidate
* @param jobTargetText the job target text, typically "Job Target"
* @param jobTargetData the job target data
*/
@Override
public void writeCandidateAndTarget(String candidateNameData, String[]
candidateAddressData, String jobTargetText, String jobTargetData) {

this.pw.println("<BODY>");
this.pw.println("<table width=\"580\">");
this.pw.println("<tr><td>");
this.pw.println("<h1>" + candidateNameData.toUpperCase() + "</h1>");
this.pw.println("<p>"); //$NON-NLS-1$
for (String candidateAddressLine : candidateAddressData) {
if (candidateAddressLine.indexOf("@") > -1) {
this.pw.println("<a href=\"mailto:" + candidateAddressLine +
"\">" + candidateAddressLine + "</a><br>");
} else {
this.pw.println(candidateAddressLine + "<br>");
//$NON-NLS-1$
}
}
this.pw.println("</p>");
this.pw.println("<h4>" + jobTargetText + ": " + jobTargetData +
"</h4>");
this.pw.println("<br>");
this.pw.println("</td></tr>");
}

-------------------------------------------------------------------------------------------------------------------

See? I use the concrete methods from the abstract ResumeWriter class to get
the data from the ResourceBundle and don't need to implement them within my
subclasses at all, then I implement the abstract writeCandidateAndTarget()
method within the subclass so that it writes the information in HTML. Class
ResumeWriterPdf looks identical to ResumeWriterHtml except that its
implementation of method writeCandidateAndTarget() writes PDF format;
however, it is using the exact same concrete methods to get the data from
the ResourceBundle and those methods don't need to be overridden in the
subclasses of ResumeWriter.

If I followed your suggestion, I'd basically be eliminating those concrete
getters from the abstract class and having to implement that code in each of
the subclasses of ResumeWriter, which would mean duplicating the getter code
in each and every one of the subclasses. That would be a definite step
backwards from the current design.

The only quibble is that some implementations of some of my abstract write
methods don't need all of the data that the method wants. For instance, in
some cases, the implementations of writeEmploymentHistory() need only
EmploymentHistoryText and EmploymentHistoryData but in other
implementations, they need FromToText, EmployerText, RoleText, and
MainToolsText. That's what prompted this thread in the first place.

Chris's remarks got me thinking that I was better to define all the
parameters that writeEmploymentHistory() would ever need in a single
abstract definition of the method rather than making several overloaded
abstract methods, one for each valid combination of parameters that the
overloaded method might need. It seems easier to simply ignore superfluous
parameters than to implement each of the overloaded methods, even if all but
one of the overloaded versions of the method contains only an empty
implementation.

Going through this thought process again has made me change my mind! As I
wrote the preceding paragraph, I realized that it still bothered me to pass
parameters that were being ignored and that this bothered me more than
having overloaded versions of the writeEmploymentHistory() method. I created
an abstract two-parameter version of writeEmploymentHistory() in
ResumeWriter to complement the six-parameter version and implemented it in
each of my three subclasses of ResumeWriter. In each of the subclasses, I
did a proper implementation of the version I was actually using and an empty
implementation of the version I wasn't using. The empty implementation got
an @SuppressWarnings("unused") annotation and a "//not used in this
subclass" comment; the proper implementation got neither. Then, I made sure
that I adjusted the invocation of writeEmploymentHistory() to pass only the
appropriate number of parameters, two or six, none of which were null.
Frankly, I like this approach better overall, although I could certainly
live with the no-overloaded methods approach in a pinch.

 

[Raymond DeCampo]

Going through this thought process again has made me change my mind! As I
wrote the preceding paragraph, I realized that it still bothered me to pass
parameters that were being ignored and that this bothered me more than
having overloaded versions of the writeEmploymentHistory() method. I created
an abstract two-parameter version of writeEmploymentHistory() in
ResumeWriter to complement the six-parameter version and implemented it in
each of my three subclasses of ResumeWriter. In each of the subclasses, I
did a proper implementation of the version I was actually using and an empty
implementation of the version I wasn't using. The empty implementation got
an @SuppressWarnings("unused") annotation and a "//not used in this
subclass" comment; the proper implementation got neither. Then, I made sure
that I adjusted the invocation of writeEmploymentHistory() to pass only the
appropriate number of parameters, two or six, none of which were null.
Frankly, I like this approach better overall, although I could certainly
live with the no-overloaded methods approach in a pinch.

Why do your writeEmploymentHistory() methods require any parameters, if
all the data is available via the superclass?

HTH,
Ray

 

[Rhino]

Why do your writeEmploymentHistory() methods require any parameters, if
all the data is available via the superclass?

Good point!

I'm invoking all of the getters from the constructor of each ResumeWriter
subclass, e.g. ResumeWriterAscii. Here's a short fragment from the actual
ResumeWriterAscii:

---------------------------------------------------------------------------------------------------------
public ResumeWriter(ResourceBundle locList) {

....
String employmentHistoryText = getEmploymentHistoryText(locList);
String[][] employmentHistoryData =
getEmploymentHistoryData(locList);
writeEmploymentHistory(employmentHistoryText,
employmentHistoryData);

....
-----------------------------------------------------------------------------------------------------------

I suppose I could very easily change the class so that employmentHistoryText
and employmentHistoryData are class variables, then simply remove the
parameters from the invocation of writeEmploymentHistory().

But isn't it generally bad form to make something a class variable unless it
really needs to be a class variable because it is used all over the class?
In my case, employmentHistoryText and employmentHistoryData are used only in
the constructor and in writeEmploymentHistory().

Wouldn't that suggest the two variables really ought to be local variables?
That's my gut hunch anyway. But maybe that's just me overcompensating for my
first several years of programming! I was writing mainframe COBOL and COBOL
doesn't have local variables: everything is a global variable. Once I
discovered that local variables existed in some languages, I've tended to
try to use a local variable whenever possible, perhaps to excess.

Do you think I should be making the variables that hold my getter results
class variables rather than local variables?

Rhino

 

[Chris Smith]

public ResumeWriter(ResourceBundle locList) {

...
String employmentHistoryText = getEmploymentHistoryText(locList);
String[][] employmentHistoryData =
getEmploymentHistoryData(locList);
writeEmploymentHistory(employmentHistoryText,
employmentHistoryData);

Danger, Will Robinson! You're calling a non-final and non-private
method from a constructor, here. That's a very dangerous thing to do,
and best avoided. It means that any fields of the subclass will be
uninitialized -- even if they are compile-time constants! -- when
writeEmploymentHistory is called. For example, consider a subclass:

public class HTMLResumeWriter extends ResumeWriter
{
private String docType;

public HTMLResumeWriter(String docType, ResourceBundle locList)
{
super(locList);
this.docType = docType;
}

public void writeEmploymentHistory(String text, String[][] data)
{
...
}
}

docType will be null inside of writeEmploymentHistory.

But isn't it generally bad form to make something a class variable unless it
really needs to be a class variable because it is used all over the class?
In my case, employmentHistoryText and employmentHistoryData are used only in
the constructor and in writeEmploymentHistory().

It's good form to make something an instance field if it is logically a
property of any object of that class. Rules of thumb that rely on
things like how or where something is used are necessarily approximate,
and prone to give wrong suggestions sometimes. In this case, I see two
possible designs, and you have your choice between them:

Option 1: ResumeWriter is conceived of as a tool. You could create one
without giving it any specific information about any particular person's
resume. It has a method to write a resume, and all necessary
information is passed in there. Much like a printer, the ResumeWriter
is a general-purpose tool that may be used to create any number of
resumes, and the details of the resumes it writes depend on how it's
used, not how it was built. In that case, the resume information should
be passed around as a parameter, because it's not logically a property
of the writer.

Option 2: A ResumeWriter is a one-shot object that's used to write a
specific resume. It's created to write one resume and then discarded.
It's more like a print job than a printer, in that it is a transient
thing that is created for and works with a specific set of data, and a
new one is created for the next data set. In this case, the resume
that's being written definitely is a property of the ResumeWriter, and
it should be a field.

Both options are fine. Each gives you different tools for handling
additional requirements if the application becomes more advanced in the
future. So far, you seem to have chosen option 2, and that's fine. In
that case, you should store the information about the resume in fields,
and provide protected APIs for subclasses to access it.

--
www.designacourse.com
The Easiest Way To Train Anyone... Anywhere.

Chris Smith - Lead Software Developer/Technical Trainer
MindIQ Corporation

 

[Rhino]

public ResumeWriter(ResourceBundle locList) {

...
String employmentHistoryText = getEmploymentHistoryText(locList);
String[][] employmentHistoryData =
getEmploymentHistoryData(locList);
writeEmploymentHistory(employmentHistoryText,
employmentHistoryData);

Danger, Will Robinson! You're calling a non-final and non-private
method from a constructor, here. That's a very dangerous thing to do,
and best avoided. It means that any fields of the subclass will be
uninitialized -- even if they are compile-time constants! -- when
writeEmploymentHistory is called. For example, consider a subclass:

public class HTMLResumeWriter extends ResumeWriter
{
private String docType;

public HTMLResumeWriter(String docType, ResourceBundle locList)
{
super(locList);
this.docType = docType;
}

public void writeEmploymentHistory(String text, String[][] data)
{
...
}
}

docType will be null inside of writeEmploymentHistory.

Okay, so what are you saying I should do to prevent these problems? I know
the compiler will not let me make writeEmploymentHistory() (within
ResumeWriterAscii) a private method because it is public in the abstract
class and the implementation can't have less access than the abstract
version. I know the compiler will only let me make the method public or
protected in the abstract class. I _can_ make writeEmploymentHistory() final
within ResumeWriterAscii; is that what you are suggesting? I don't plan on
subclassing ResumeWriterAscii or its siblings any further so I don't have a
problem with making the writeXXX() implementations final.

With regards to your docType example, I wasn't aware that docType would be
uninitialized in that case. Frankly, it rather surprises me that docType
would be uninitialized in this case, given that docType is a class variable
in the class. Why exactly does Java behave that way? Would I be safe in
assuming that a compiler warning would bring this to my attention?

Am I correct in thinking that you are warning me against setting a class
variable based on a value passed in the parameter of a constructor?

My actual ResumeWriterAscii class, and its siblings, pass only two values to
their constructors: a String containing the name of the file that is to be
written and a ResourceBundle that contains the data used by the class. Both
of these values are not assigned to any class variables, are used only in
the constructor and are not passed explicitly to any other method of the
class. I also have several class variables in ResumeWriterAscii: all but two
of them are constants used throughout the class for formatting. The other
two class variables are:

private StringUtils stringUtils = null;
private PrintWriter pw = null;

They get set to useful values in the constructor and are then used
throughout the class. I'm not going to have any problems with any of that,
am I? The compiler is not giving me any errors or warnings anywhere in my
project.

Hmmm, just out of curiousity, I made a class variable called strOutfile
which was defined as a private String and initialized to null. Then, within
my constructor, I added this line:

this.strOutfile = strOutfile;

I also added this line to writeEmploymentHistory():

System.out.println("strOutfile: " + this.strOutfile); //$NON-NLS-1$

The compiler flags 'strOutfile' within my constructor signature ['public
ResumeWriterAscii(String strOutfile, ResourceBundle locList)'] and gives me
this warning: "The parameter strOutfile is hiding a field from type
ResumeWriterAscii." That's fair enough since the strOutfile value passed in
via the constructor will inevitably clobber the value of the strOutfile
class variable. But I get no warnings or errors about strOutfile being
uninitialized in writeEmploymentHistory() even though it now parallels the
docType example you gave. I assume that's why you brought that to my
attention: since the compiler gives no warning, I can easily fail to notice
this problem.

It's good form to make something an instance field if it is logically a
property of any object of that class. Rules of thumb that rely on
things like how or where something is used are necessarily approximate,
and prone to give wrong suggestions sometimes. In this case, I see two
possible designs, and you have your choice between them:

Option 1: ResumeWriter is conceived of as a tool. You could create one
without giving it any specific information about any particular person's
resume. It has a method to write a resume, and all necessary
information is passed in there. Much like a printer, the ResumeWriter
is a general-purpose tool that may be used to create any number of
resumes, and the details of the resumes it writes depend on how it's
used, not how it was built. In that case, the resume information should
be passed around as a parameter, because it's not logically a property
of the writer.

Option 2: A ResumeWriter is a one-shot object that's used to write a
specific resume. It's created to write one resume and then discarded.
It's more like a print job than a printer, in that it is a transient
thing that is created for and works with a specific set of data, and a
new one is created for the next data set. In this case, the resume
that's being written definitely is a property of the ResumeWriter, and
it should be a field.

Both options are fine. Each gives you different tools for handling
additional requirements if the application becomes more advanced in the
future. So far, you seem to have chosen option 2, and that's fine. In
that case, you should store the information about the resume in fields,
and provide protected APIs for subclasses to access it.

You're right; I do think of my ResumeWriter subclasses as Option 2.

Could you elaborate on what you mean by your last sentence? Are you saying
that I am doing the right thing in storing the values obtained by the
getters in local variables and then passing them to their respective
writers? Do the implementations of the writeXXX methods which appear in my
ResumeWriter subclasses constitute the protected APIs that you are
recommending? Or are you proposing something else?

Rhino

 

[Chris Smith]

Okay, so what are you saying I should do to prevent these problems? I know
the compiler will not let me make writeEmploymentHistory() (within
ResumeWriterAscii) a private method because it is public in the abstract
class and the implementation can't have less access than the abstract
version. I know the compiler will only let me make the method public or
protected in the abstract class. I _can_ make writeEmploymentHistory() final
within ResumeWriterAscii; is that what you are suggesting? I don't plan on
subclassing ResumeWriterAscii or its siblings any further so I don't have a
problem with making the writeXXX() implementations final.

I mean that any method you call from a constructor should be either
private or final in the class where you call it. Since you need to
override writeEmploymentHistory, that precludes the possibility of
calling writeEmploymentHistory from the constructor. An alternative
design is:

public class ResumeWriter
{
private ResourceBundle locList;

public ResumeWriter(ResourceBundle locList)
{
this.locList = locList;
}

public void writeResume()
{
String employmentHistoryText = ...;
String[][] employmentHistoryData = ...;

writeEmploymentHistory(
employmentHistoryText, employmentHistoryData);
}
}

There are now two steps to writing a resume: first, create a
ResumeWriter, and then call its writeResume method. That's better,
anyway. It's surprising when writing the resume -- which is the main
purpose of a ResumeWriter -- is done as a side-effect of creating the
object. If other people were using this API, it would take them time to
get accustomed to that little quirk, and it prevents the code from
reading well. There is no mental model of the ResumeWriter class that
makes it logical for it to write the resume in the constructor.

Note that this paves the way for the changes suggested elsewhere. What
needs to happen now is:

- your getEmploymentHistoryText and getEmploymentHistoryData methods
made protected, so that they can be accessed by the subclass, and
modified so that they get locList from the instance field rather than a
parameter.

- All parameters removed from writeEmploymentHistory. Implementations
of writeEmploymentHistory in the subclass should call
getEmploymentHistoryText and getEmploymentHistoryData directly if they
need that information.

With regards to your docType example, I wasn't aware that docType would be
uninitialized in that case. Frankly, it rather surprises me that docType
would be uninitialized in this case, given that docType is a class variable
in the class. Why exactly does Java behave that way?

It's a quirk of the implementation. The superclass constructor runs
before the subclass constructor... and a polymorphic call to a method
(like writeEmploymentHistory) from the superclass constructor can
therefore reach the subclass before its constructor is called. The
typical way to avoid this is to make sure that all methods called from a
constructor are private or final, so that they can't be overridden.

Would I be safe in
assuming that a compiler warning would bring this to my attention?

In Eclipse, I assume? I don't believe that warning exists.

Am I correct in thinking that you are warning me against setting a class
variable based on a value passed in the parameter of a constructor?

I'm confused by your use of the term "class variable". When most people
say "class variable" or "class field", they mean a static field. My
example didn't use any static fields (although it would probably be
incorrect to set them from a constructor, if you did have one).

My warning was against calling polymorphic (that is, non-final and non-
private) methods from a constructor.

I also have several class variables in ResumeWriterAscii: all but two
of them are constants used throughout the class for formatting. The other
two class variables are:

private StringUtils stringUtils = null;
private PrintWriter pw = null;

They get set to useful values in the constructor and are then used
throughout the class. I'm not going to have any problems with any of that,
am I?

Well, in the code you posted, writeEmploymentHistory is going to be
called BEFORE the constructor for ResumeWriterAscii... so you very well
might have problems. Both variables will be null -- and not because you
set them to null above, but because all reference variables will be
null.

The compiler is not giving me any errors or warnings anywhere in my
project.

As I said, I don't think Eclipse has a warning for this one.

Hmmm, just out of curiousity, I made a class variable called strOutfile
which was defined as a private String and initialized to null. Then, within
my constructor, I added this line:

this.strOutfile = strOutfile;

I also added this line to writeEmploymentHistory():

System.out.println("strOutfile: " + this.strOutfile); //$NON-NLS-1$

The compiler flags 'strOutfile' within my constructor signature ['public
ResumeWriterAscii(String strOutfile, ResourceBundle locList)'] and gives me
this warning: "The parameter strOutfile is hiding a field from type
ResumeWriterAscii." That's fair enough since the strOutfile value passed in
via the constructor will inevitably clobber the value of the strOutfile
class variable.

The warning you're getting is not related to this problem. It's just
saying that you're hding a field with a parameter. To get rid of it,
just rename the parameter.

I assume that's why you brought that to my
attention: since the compiler gives no warning, I can easily fail to notice
this problem.

Yep. I'm starting to wonder how hard it would be to write code to
detect this, and contribute the code to Eclipse. I'll look into that
today.

Could you elaborate on what you mean by your last sentence? Are you saying
that I am doing the right thing in storing the values obtained by the
getters in local variables and then passing them to their respective
writers?

No, I'm saying that all the information should be stored in instance
fields of the object. I'm given more detailed suggestions about that
may be more helpful.

--
www.designacourse.com
The Easiest Way To Train Anyone... Anywhere.

Chris Smith - Lead Software Developer/Technical Trainer
MindIQ Corporation