tags:

views:

206

answers:

6

I'm reading a HTTP POST and the body of the HTTP request can be either JSON or XML. Now I've delegated the reading to a special utility class.

interface HttpUtils
{
    BodyWrapper parseBody( HttpServletRequest req );
}

interface BodyWrapper
{
    boolean isXML();  // 1
    boolean isJSON(); // 2
    String body();    // 3
}
  • I hate the fact that BodyWrapper has methods (1 & 2) to identify its type. Perhaps I should use inheritance. If I do that, I will need to do an instanceof to find out what is being returned by HttpUtils.parseBody(..)
  • Ideally I would also want the body() method to return either a JSONObject or an DOM node. How would I do that ?

Thanks

+3  A: 

First off, HttpUtils is way to generic a name. I'd go for HttpRequestParser or something. You'll also need a factory which will create appropriate implementation based on content-type of a request (XmlRequestParser or JsonRequestParser).

As far as parsing goes, I'd recommend to parse both XML and JSON to some arbitrary internal representation (IR), so that your code higher up the stack will not be concerned with such details. An IR can be an XML document, or some object graph.

Anton Gogolev
+1. Use a factory to get the object of the right type. Do not use inheritance in this case.
Gishu
+2  A: 

JSONObject and DOM nodes are unrelated to each other, inheritance wise. This means that in order to overload on the return type it would have to return object. This is a nasty code smell since you would probably then have to use introspection to figure out what was returned. Generally in this situation, you should be using a virtual method on the object (the body) which is able to act on the body in the correct fashion depending on what it actually is.

1800 INFORMATION
+3  A: 

What on earth is BodyWrapper for? Shouldn't parseBody just return the de-seriealized object? This could be a model object or it could just be a bag of values (dictionary / map / hashtable).

So, parseBody will need to check the type of the POST and then deserialize. What data are you expecting? The result should be a type that represents the actual data you want the client to post in a java sort of way, irregardless of how it was posted (jason / xml)

Daren Thomas
I like the "bag of values" idea. The data varies depending on the request. It is not possible to come up with an object model for it.
Jacques René Mesrine
+1  A: 

Abstraction seems difficult alt this level. What do you do with the JSONObject or DOM returned as body? Often it is more easy to go one step further. Is it possible to transform both to the same Java structure? Depending on the content type, create the JSOn or DOM implementation of your body parser and use the resulting java structure created by the parser in the code working on the body. If neeeded (to create the right answer format), you can make the original content type available from the answer (getMimeType() or something like that).

Arne Burmeister
+8  A: 

Don't ask your objects for information, and then make decisions on what they tell you. Make your objects do the work for you. That is, don't do this:

if (body.isXML()) {
  // do XML stuff
}
else if (body.isJSON()) {
  // do JSON stuff
}

It's a maintenance headache. Do something like this instead (the BodyWrapper implementations would created using an abstract factory method or similar)

public interface BodyWrapper {
   Object doStuff();
}

public class DOMBodyWrapper implements BodyWrapper {
   public Object doStuff() {
   }
}

public class JSONBodyWrapper implements BodyWrapper {
   public Object doStuff() {
      // do something and return a success/failure result. I've
      // deliberately not defined what this object is....
   }
}

and then:

// get the body via a factory or similar
body.doStuff();

That way, something creates the appropriate BodyWrapper implementation, and then instead of asking it what type it is, you just use it. Note that the BodyWrapper isn't returning different types of internal structures, because it (perhaps an abstract base class) is doing the work for you.

Brian Agnew
And still doStuff() returns type Object. :-(
bwalliser
I've not said what that object is. It could be a result/return value or similar. I shall edit accordingly. Thanks.
Brian Agnew
or, generify the BodyWrapper interface to BodyWrapper<T>, and have doStuff() return T.
Chii
Or have a StuffDoneResult object. The return isn't the issue in this example
Brian Agnew
+2  A: 

Just to give you some more food for thought, a variation of the Visitor pattern where the data types are not related by interitance could help you out here.

To be honest though, it could also turn out to be overkill in this particular case depending on your actual use case(s).

Here's some pseudo-code:

interface BodyTypesVisitor
{
   void visit( DOMNode domNode );
   void visit( JSONObject jsonObject );
}

interface BodyWrapper
{
    void accept( BodyTypesVisitor );
}

interface HttpUtils
{
    BodyWrapper parseBody( HttpServletRequest req );
}

class DOMVisitor implements BodyTypesVisitor
{
   void visit( DOMNode domNode ) { /* do something useful with domNode */ }
   void visit( JSONObject jsonObject ) { /* ignore */ }
}

class DOMBody implements BodyWrapper
{
    ...

    void accept( BodyTypesVisitor visitor )
    { visitor.visit( this->domNode ); }

    private DOMNode domNode;
}

...
// Process DOM
BodyWrapper wrapper = <some HttpUtils implementation that creates a DOMBody>
DOMVisitor visitor = new DOMVisitor();

wrapper.accept(visitor);

The Visitor pattern is generally useful if you have a distinct and relatively static set of "data types" that you want to process in several different ways.

Cwan