7
\$\begingroup\$

While learning object oriented design I'm judging my own design critically. This framework should be able to print objects in either XML, or JSON, I've stubbed in a basic implementation to avoid getting into details of XML and JSON parser APIs for now.

So I made the Formatter be the base class. But with my current design, all derivatives of this base class would need to know that they have to call: getFormattedValue() to get output. Also I don't feel comfortable with all of those if else statements in the Formatter constructor. The clients would need to know to pass in either an "XML" or "JSON" in all derivatives of this class. How can I improve this design to conform to all object oriented design principles?

public class Formatter {

    private String output;

    public Formatter(Object object, String formatType){
        if(formatType.equals("xml")){
            output = getXMLFormat(object);
        } else if(formatType.equals("json")) {
            output = getJSONFormat(object);
        }
    }

    private String getXMLFormat(Object object){

        return "<title>"+object.toString()+"<title>"; // simplified
    }

    private String getJSONFormat(Object object){
        return "{"+object.toString()+"}"; // simplified
    }

   protected  String getFormattedValue(){
        return output;
    }
}

The derivative class:

public class ItemFormatter extends Formatter {

    public ItemFormatter(Employee item, String formatOutput) {
        super(item, formatOutput);
    }

    public void printItem(){
        System.out.println(getFormattedValue());
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ Welcome to Code Review. Nice first question. \$\endgroup\$ – RubberDuck Aug 19 '14 at 10:37
  • \$\begingroup\$ @TazMan: Why do you need ItemFormatter derived class? \$\endgroup\$ – Tahir Akhtar Aug 19 '14 at 12:35
  • \$\begingroup\$ Because there are other types of derivations that need a reporting standard. There is InventoryFormatter, EmployeeFormatter, AccountsFormatter.. etc. And each derivation would have the PrintObject, printInventory and printItem methods with specific changes if needed \$\endgroup\$ – GrindMan Aug 19 '14 at 15:18
  • \$\begingroup\$ ditto to what @TahirAkhtar says of ItemFormatter. Employee is passed to the base turning into Object - calling the base constructor directly is exactly the same thing. I assume Employee et.al. overrides toString() but it doesn't matter as object.toString() is not polymorphic here. ... What is a reporting standard? It seems to have "business model" meaning so maybe this should be a class. \$\endgroup\$ – radarbob Aug 20 '14 at 20:53
5
\$\begingroup\$

This one is screaming for an enum. What you want is what you get:

public enum FormatType {
    XML, JSON, //maybe further support later
}

And your constructor then becomes:

public Formatter (Object object, FormatType formatType) {
    switch (formatType) {
        case XML:
            output = getXMLFormat(object);
            break;
        case JSON:
            output = getJSONFormat(object);
            break;
    }
}

In addition to that a small nitpick on naming:
Your get...Format() methods break the camelCase you kept until then. I suggest you ignore that these formats are acronyms and instead use following method names:

getXmlFormat (Object object);
getJsonFormat (Object object);

But I'd say that's personal preference. All else you didn't give us much to review :(

\$\endgroup\$
5
  • \$\begingroup\$ So after changing to use an enum, the design is okay? \$\endgroup\$ – GrindMan Aug 19 '14 at 10:52
  • \$\begingroup\$ @TazMan there's currently quite the meta-discussion going on about design. Additionally I am not that experienced concerning design overall. Your question is mostly missing the context to decide whether the design is okay. I don't really get what you want to achieve with it, so I refrain from any judgement here. \$\endgroup\$ – Vogel612 Aug 19 '14 at 10:54
  • \$\begingroup\$ where is the meta-discussion? Is that regarding this design? Also what should i do with making the assumption that clients would know or remember to call getFormattedValue()? \$\endgroup\$ – GrindMan Aug 19 '14 at 12:11
  • \$\begingroup\$ @TazMan check this link: meta.codereview.stackexchange.com/questions/2308/… and the related questions. Feel free to also drop by in Code Review Chat and tell us your opinion or write an answer on Code Review Meta yourself ;) \$\endgroup\$ – Vogel612 Aug 19 '14 at 12:14
  • \$\begingroup\$ Surely the Formatter should not know how to parse all of those formats (XML/JSON). The factory pattern would be best applied here, let the factory decide what implementation to respond with - this way you get all the nice separation of concerns and SRP goodness that you would want to achieve. \$\endgroup\$ – Dan Aug 20 '14 at 18:44
5
\$\begingroup\$

Disclaimer: I've been spoiled by Action<> in C# for a long time and haven't used Java for even longer. This problem would suit the factory and strategy patterns very well. Here is how I would approach the problem.

// This interface will describe a single format.
// You could allow this to return Object, or you could
// use generics. I'm going to use generics because I'm a C# guy.

// Note: you could optionally constrain this to one non-object
// input type.
public interface IFormatter<TOutput> {
  // Formats the given object into TOutput.
  // I would recommend making this a generic constraint
  // on the interface and having types of same outputs
  // implement the interface numerous times.
  TOutput format(Object input);
}

// An enumeration of the formats avaliable to you.
// This is a decent solution if you know ahead of time
// all of the formats that you want the user to have access to.
public enum Format {
  Xml,
  Json
}

// A factory that can create formatters that output
// a single type.
public interface IFormatterFactory<TOutput> {
  // Return a formatter for the given type.
  IFormatter<TOutput> getFormatter(Format format);
}

public class XmlFormatter : IFormatter<String> {
  @Override public String format(Object input) {
    return "<title>" + input.toString()+"</title>";
  }
}

public class JsonFormatter : IFormatter<String> {
  @Override public String format(Object input) {
    return "{"+object.toString()+"}";
  }
}

public class FormatterFactory<TOutput> : IFormatterFactory<TOutput> {
  private Dictionary<Format, IFormatter<TOutput>> _formatters;
  public FormatterFactory(Dictionary<Format, IFormatter<TOutput>> formatters) {
    _formatters = formatters;
  }

  public IFormatter<TOutput> getFormatter(Format format) {
    if(!_formatters.containsKey(format)) {
      throw new UnknownObjectException("no formatter for format" + format.toString());
    }
    return _formatters.get(format);
  }
}

I do not think it's a good idea for your formatters to implement a "print" method. Then you are teaching your formatters how to format to TOutput, and then teaching them how to print those items. In my opinion, this invalidates the SRP. Instead, you should have TOutput know how to print itself (by way of toString(), although you should stick to the contract of toString() if you do this), or implement the presenter pattern.

One caveat of this approach is that all of your formatters must have the same TOutput to be type-compatible with the factory. However, seeing as you are looking at "printing" objects I don't see how this is a problem. The upside of this as well means that anything that needs a formatter can simply just ask for a IFormatter<String> without having to care about whether it is an XmlFormatter or a JsonFormatter.

One thing I will stress, as I said earlier - it should not be the job of a formatter to know how to print something to the console. Or in other terms - it is not up to the producer to tell the consumer what to do with it's content. The producer should only provide the consumer with the content, the consumer should do the rest. This makes for cleaner code in the long run.

Contrary to others, I do not think the factory should be static. That's just me, though. I think the Factory should be injected into whatever is responsible for creating the object that needs the Formatter (if not further up the object graph).

Read: Do not pass the IFormatterFactory<TOutput> into the class that needs the formatter and let it decide which one you use. You should give the class that needs the formatter an IFormatter<TOutput> object and create the formatter further up the object graph, where the depending class is created.

Please make the dictionary immutable after creation. I can't remember how to do that in Java, but it should be immutable unless you want all hell to break loose or you NEED the mutability. Mutability + non-static = oops threading.

Presenter pattern as promised:

interface IReportPresenter {
  void present(Report report);
}

class ConsolePresenter : IReportPresenter {
  public void present(Report report) {
    System.out.println(report);
  }
}

class FilePresenter : IReportPresenter {
  public void present(Report report) {
    //...
    fileStream.Write(report);
  }
}

public class FormatterFactory<TOutput> : IFormatterFactory<TOutput> {
  private Dictionary<Format, IFormatter<TOutput>> _formatters;
  public FormatterFactory(Dictionary<Format, IFormatter<TOutput>> formatters) {
    _formatters = formatters;
  }

  public IFormatter<TOutput> getFormatter(Format format) {
    return getFormatter(format, new ConsolePresenter());
  }

  public IFormatter<TOutput> getFormatter(Format format, IReportPresenter presenter) {
    if(!_formatters.containsKey(format)) {
      throw new UnknownObjectException("no formatter for format" + format.toString());
    }
    // this one is a little more tricky
    // and it SHOULD be written better
    // but whatever - just set the presenter element on the formatter
    // strictly speaking we should create the formatter here and
    // return it but Java doesn't have delegates
    IFormatter<TOutput> formatter = _formatters.get(format);
    formatter.setPresenter(presenter);
    return formatter;
  }   
}

Side note: I wish we didn't have to use Enum for this, it feels like a horrible hack :(

\$\endgroup\$
9
  • \$\begingroup\$ The item formatter actually is the child class that will need the print method. I need for children to be able to customize the formatting rules defined in the parent if needed. And allow for printing. Do I need a printer specific object? Seems kind of over engineered then. \$\endgroup\$ – GrindMan Aug 20 '14 at 20:26
  • \$\begingroup\$ Do you need a class solely for printing? No, that would be over-engineering. But I personally don't feel the responsibility for printing lies with the formatter. The formatter already has the responsibility of being a data mapper. Consider using a logging framework like log4j and have that handle your printing instead? It is OK to have the formatter doing the printing, provided that it is delegating it and not actually managing the responsibility itself (and the logger is injected via constructor/property) \$\endgroup\$ – Dan Aug 20 '14 at 20:28
  • 1
    \$\begingroup\$ The balance to strike here is that you could put the printing method inside of the formatter, but then you introduce a side effect that will happen every time you use the format() method. Perhaps this is OK for your purposes, but I tend to keep everything adhering to the SRP simply because I value my sanity in larger projects. Plus, it's a lot nicer on the poor dev down the road who wonders why his JSON is being printed to std out whenever he tries to format it. \$\endgroup\$ – Dan Aug 20 '14 at 20:32
  • \$\begingroup\$ Actually this class would be better named as ReportFormatter, and the children would be, InventoryReportFormatter, EmployeeReportFormatter and AccountsReportFormatter. Your thoughts? I just had trouble finding a proper name to incorporate both formatting and printing.. :-( \$\endgroup\$ – GrindMan Aug 20 '14 at 20:35
  • \$\begingroup\$ "trouble finding a proper name" If you had trouble finding a proper name it's probably because your class is either doing too much, too little, or you are not sure on what it is doing :) I'm not sure what you mean by "This class". \$\endgroup\$ – Dan Aug 20 '14 at 20:36
4
\$\begingroup\$

I don't feel comfortable with all of those if else statements in the Formatter constructor.

Well, you asked for it! Here goes...

  1. Create FormattedItem factory.
  2. A Formatter (for a specific FormatType) is reusable
    • Build a XML | JSON specific Formatter
    • Do not pass the to-be-formatted object in the constructor
    • Formatter is stateless, which is a very good thing. It does not keep a reference to the thing it is formatting.
  3. Extensibility is more object oriented. To add a new format type:
    • Add enum member
    • Add code to factory
    • Create new IFormattedItem implementation
  4. Other OO Goodness
    • client handles all formatted items the same way.
    • factory pattern helps us adhere to the Open/Closed Principle
    • Each xxxFormattedItem class can be modified - fix bugs! - and no other classes need to change. More Open/Close Principle at work.

using enum and switch from @Vogel612 answer.

// client code
Formatter myXMLFormatter = new Formatter(FormatType.XML);
Formatter myJSONFormatter = new Formatter(FormatType.JSON);
String myXMLedObject = myXMLFormatter.getFormattedValue(object1);
String myXMLedObject2 = myXMLFormatter.getFormattedValue(object2);
String myJSONObject = myJSONFormatter.getFormattedVlaue(object1);
String myJSONObject2 = myJSONFormatter.getFormattedVlaue(object2);

// Formatter redux

// This is the only class the client should see, so all the classes below this
// should be hidden from the client - I did not do that here.
public class Formatter {
    protected FormatterFactory factory = new FormatterFactory(); // could be a static class
    protected IFormattedItem output;

    // tell the Formatter what kind to be
    public Formatter (FormatType formatType) {
        output = factory.Create(formatType);
    }

    public IFormattedItem getFormattedValue(Object object) {
        return output.FormatMe(object);
    }
}

// could be an abstract class
// can add stuff to abstract class without breaking thiings, vice an `interface`
public interface IFormattedItem {
    String FormatMe(Object thingToFormat);
}


// Formatter class gets one of these from the factory
// You may want methods that can get values from the formatted object
public class XMLFormattedItem implements IFormattedItem {
    public String FormatMe(Object object) { ... }
}

public class JSONFormattedItem implements IFormattedItem {
    public String FormatMe(Object object) { ... }
}


// Only Formatter should have access here
internal class FormatterFactory {
    IFormattedItem output;

    public IFormattedItem create (FormatType outputForm) {
        switch (outputForm) {
            case XML:
                output = new XMLFormattedItem();
                break;
            case JSON:
                output = new JSONFormattedItem();
                break;
            default:
                throw new NotImlimentedException( "FormatType not implemented: " + outputForm.toString();
        }

        return output;
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ Do you think this refactoring is worth it? It sure adds many more lines of code and the application id small \$\endgroup\$ – GrindMan Aug 20 '14 at 16:40
  • \$\begingroup\$ "It sure adds many more lines of code." "line count is too high" is an OO anti-principle if ever there was one. ... Depends on how much you want to "conform to all object oriented design principles." Also depends on how much future change you see. New formatTypes, functionality you might add for the types, etc. \$\endgroup\$ – radarbob Aug 20 '14 at 17:25
  • \$\begingroup\$ a few comments: the Factory can be a private class, what you implement then is more of a Facade than a Factory pattern. IFormattedItem is a glorified Object (--> use case??) The factory should be used in a static way (as you mention in a comment). output is a not that good name for what the Factory returns and If you want to comment a use-case do so in xml-documentary comments, all else equal, good job ;) \$\endgroup\$ – Vogel612 Aug 20 '14 at 19:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.