Is it poor design to have a class used only by one other class?

Question

I have a QueueProcessor class that processes a work queue. I want to send an email out of a summary of all work done with a bunch of statistics. I am already using a mail class (Zend_Mail), so most of the email work is abstracted away for me, but I still have to figure out where to put the code to generate the summary information. I'm basically collecting data during the processing and then I have to transform the data into a format suitable for email.

My question is, should I create a separate class called QueueProcessorSummaryEmail, or is that considered bad design since it's extremely tightly coupled to the original class? If I did make it it's own class, I'd have to pass a ton of data to it for it to generate the email, but it'd be nice to have that kind of separation. If I don't make it a class, all the data is available from inside the QueueProcessor class, but it just feels weird mixing the processing logic and the reporting generating logic in one class.

And for the record, when I say "report generating logic", I don't mean I'm generating HTML inline; I'm using a view for that. By formatting I mean taking the data and aggregating it into something usable by the email report.

Answer 1

ryeguy,

i'd be tempted to make the QueueProcessor class the base abstract class and then Extend that to create the QueueProcessorSummaryMail class.

ok, basic example:

class QueueProcessor
{
     // all your methods and properties
}

class QueueProcessorSummaryMail extends QueueProcessor
{
     // add your custom properties and methods here
}

Hopefully, this gives you the main idea.

jim

[edit] that of course is for php. for c#, you'd still use either abstract classes or an interface.

Answer 2

is that considered bad design since it's extremely tightly coupled to the original class?

No, that's not a problem. Subclass-superclass design is also tightly coupled.

it just feels weird mixing the processing logic and the reporting generating logic in one class.

Delegation of reporting/summary often makes sense. Reporting changes and expands more quickly than the processing does.

Answer 3

One way to help de-couple your design is by using interfaces. For example, you could create an interface called ISummaryDataProvider and have QueueProcessor implement that interface with the properties and methods that would be needed to provide the data for another class that handles the summary data.

Then, you can have your constructor for QueueProcessorSummaryEmail take in an ISummaryDataProvider as a parameter. That way, if you ever decide to get your summary data from something other than QueueProcessor, you just need to make sure that your new data source implements ISummaryDataProvider so that it can be properly consumed by QueueProcessorSummaryEmail.

---

I explored similar object-oriented-design questions in my question How should I model my code to maximize code re-use in this specific situation?. One of the answers suggested using Dependency Injection, which I didn't really understand before. You can see at the bottom of my question how I implemented this solution in a loosely-coupled way. You may find the information there helpful for your situation.

Answer 4

Structure matters. If there is a set of logic that doesn't really belong in a class then refactoring it elsewhere can really help mainatainability in the future.

In a languages that support inner classes you could just make it an inner class. May be later you separate it and reuse it and then need to pass data to it at that point you might define an Interface that the Mail class accepts, and which Queue Processor implements.

Answer 5

Ryeguy,

My suggestion would be to create a decorator class (like say SummarizedQueryProcessor) to extend the functionality of the QueueProcessor. This will allow for a higher degree of separation of concerns and will simplify unit testing.

Ted

Answer 6

What about something like this?

public interface WorkProcessorI {
    public void process( WorkQueue queue ); // could also be something like "process( Task[] queue )"
}

public interface SummaryGeneratorI () {
    public String[][] generateSummary(); // returns subscripted string array of name/values or whatever
}

public class QueueProcessor implements WorkProcessorI, SummaryGeneratorI {
    public void process( WorkQueue queue ) {
        // process queue populating member values that will be used to generate the summary.
     }

     public String[][] generateSummary() {
        // read values that were populated by the process( WorkQueue ) method.
     }
}

public class SummaryMail {
    private String[][] summary = null;
    public void setSummary( String[][] argSummary ) {
        this.summary = argSummary;
    }
    // to, from, server, cc, bcc, subject, Additional body text, ...

    public void send() {
        // sends email via whatever api you want.
    }
}