1

I've provided only simplified code as it's more of an abstract design question.

So I have many, many nested business/domain event objects, e.g.

 public class Event
 {
  //bunch of properties and standard accessors
 }

 public class ExplosionEvent extends Event
 {
  //properties and standard accessors 
 }

And many more of these at different levels. If I need information about any given object chosen, I display it in HTML like so

 private String generateHTML(Event event)
 { 
   StringBuilder sb = new StringBuilder();

   sb.append("<HTML><table>");//simplified

   sb.append("<TH>Time</TH>");   
   sb.append("<TD>" + event.getEventTime() + "</TD>");

   if (event instanceof ExplosionEvent)
   {
     //append HTML and ExplosionEvent specific data
   }

   // ...many, many more calls like the one above

   sb.append("</table></HTML>");
 }

As I have many event types, this means loads of duplicated HTML table tags and uses of instanceof so generateHTML is hundreds of lines long, split into methods of course but still, it's a lot of code that makes this hard to understand, navigate and therefore maintain.

This is ugly and I need a better design for this. I had the idea of creating a method on Event which is overridable by all sub methods

//Using LinkedHashMap to preserve order as Properties wont do that
public LinkedHashMap<String, String> getAttributes()
{
 LinkedHashMap<String, String> list = new LinkedHashMap<String, String>();

 list.put("Time", eventTime);
}

sub-classes then override this, call their parent and add date specific to them to the list meaning that no matter how many Event classes there is, the existing generateHTML method (external to Event objects) will then simply be one small loop

private String generateHTML(Event event)
{
  StringBuilder sb = new StringBuilder();

  b.append("<HTML><table>");

  for (Entry<String,String> entry : event.getAttributes().entrySet())
  {
    sb.append("<TH>" + entry.getKey() + "</TH>");   
    sb.append("<TD>" + entry.getValue() + "</TD>");
  }

  sb.append("</table></HTML>");
}

Is this putting too much logic in business/domain objects?

Is there a better way?

|improve this question
  • Nobody on Programmers have some ideas? – Ross Drew Jan 31 '14 at 15:36
3

This looks suspicious to me ...

if (event instanceof ExplosionEvent)
{
  //append HTML and ExplosionEvent specific data
}

... usually when I see that I think that Event ought to be abstract with an overridable method. That method be LinkedHashMap<String, String> getAttributes() as you suggested.

Another possibility is to store the LinkedHashMap as a member of Event (pseudo-code ahead):

class Event
{
    LinkedHashMap<String, String> map;
    protected void SetAttribute(String name, String value)
    {
        // stores in map
    }
    protected String getAttribute(String name)
    {
        // gets from map
    }
    LinkedHashMap<String, String> getAttributes()
    {
        // returns entire map
    }
}

class ExplosionEvent inherits Event
{
    String getLocation() { return getAttribute("location"); }
    void setLocation(String location) { setAttribute("location", location); }

}

You could consider using the java.utils.Properties for your implementation.

Another solution might be to use reflection.

|improve this answer
  • I hate instanceof myself, that and the duplicated table HTML. It's been suggested by colleagues that my solution is wrong because it places processing into objects which is wrong. – Ross Drew Jan 31 '14 at 16:00
  • If someone adds a new property to the ExplosionEvent subclass then they'll need to edit your generateHTML method too, which IMO is <strike>no better</strike> worse. To be 'object oriented' the base Event class should either contain (see java.utils.Properties) or abstract the list of properties; or use reflection; or (similar to refelction) use auto-generated ORM code somehow (define properties as XML and auto-gererate the Java code for the event subclasses and the generateHTML method). – ChrisW Jan 31 '14 at 16:10
1

This is a bad solution because it mixes two completely separate concerns - your objects' primary functions, and outputting its properties as HTML. On the other hand, I imagine you're trying to get access to the object's private state, which violates abstraction.

If you're only trying to print out this HTML for debugging purposes, the best solution would be to use reflection to inspect its fields/properties. If you only need to print out some fields, you can use annotations to mark which fields are meant to be inspected with reflection.

If providing access to its fields is part of the design then your classes should provide a your getAttributes as part of an interface, but delegate generateHTML to someone else. That way you don't have to change your classes just because you want to change the HTML formatting. You can decouple the code further by returning Map or Iterator from getAttributes instead of LinkedHashMap.

|improve this answer
  • This is part of the design. In my solution mentioned I have a getAttributes function per Event which returns attributes as key and value pair. In the part currently generating HTML, it will still do so, just doing it as a loop using the key and value pair from each event. – Ross Drew Feb 11 '14 at 9:46

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.