current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

I am trying to generate an HTML table using Java. I have an object which I am iterating to make an HTML table.

StringBuilder sb = new StringBuilder();
sb.append("<html>");
sb.append("<head>");
sb.append("</head>");
sb.append("<table>");
sb.append("<th style = \"background: #333; color: white; font-weight: bold; padding: 6px; border: 1px solid #ccc; text-align: left;\"> ClientName");
sb.append("</th>");
sb.append("<th style = \"background: #333; color: white; font-weight: bold; padding: 6px; border: 1px solid #ccc; text-align: left;\"> SyncCount");
sb.append("</th>");
sb.append("<th style = \"background: #333; color: white; font-weight: bold; padding: 6px; border: 1px solid #ccc; text-align: left;\"> SyncPercentile");
sb.append("</th>");
sb.append("<th style = \"background: #333; color: white; font-weight: bold; padding: 6px; border: 1px solid #ccc; text-align: left;\"> SyncAvg");
sb.append("</th>");
sb.append("<th style = \"background: #333; color: white; font-weight: bold; padding: 6px; border: 1px solid #ccc; text-align: left;\"> SyncMax");
sb.append("</th>");
sb.append("<th style = \"background: #333; color: white; font-weight: bold; padding: 6px; border: 1px solid #ccc; text-align: left;\"> AsyncCount");
sb.append("</th>");
sb.append("<th style = \"background: #333; color: white; font-weight: bold; padding: 6px; border: 1px solid #ccc; text-align: left;\"> AsyncPercentile");
sb.append("</th>");
sb.append("<th style = \"background: #333; color: white; font-weight: bold; padding: 6px; border: 1px solid #ccc; text-align: left;\"> AsyncAvg");
sb.append("</th>");

for (DataMetrics metrics : dataMetricsList) {
    sb.append("<tr>");
    sb.append("<td style = \"padding: 6px; border: 1px solid #ccc; text-align: left;\"> " + metrics.getName());
    sb.append("</td>");
    sb.append("<td style = \"padding: 6px; border: 1px solid #ccc; text-align: left;\"> " + metrics.getSyncCall());
    sb.append("</td>");
    sb.append("<td style = \"padding: 6px; border: 1px solid #ccc; text-align: left;\"> " + metrics.getSyncPercent());
    sb.append("</td>");
    sb.append("<td style = \"padding: 6px; border: 1px solid #ccc; text-align: left;\"> " + metrics.getSyncAvg());
    sb.append("</td>");
    sb.append("<td style = \"padding: 6px; border: 1px solid #ccc; text-align: left;\"> " + metrics.getSyncMax());
    sb.append("</td>");
    sb.append("<td style = \"padding: 6px; border: 1px solid #ccc; text-align: left;\"> " + metrics.getAsyncCall());
    sb.append("</td>");
    sb.append("<td style = \"padding: 6px; border: 1px solid #ccc; text-align: left;\"> " + metrics.getAsyncPercent());
    sb.append("</td>");
    sb.append("<td style = \"padding: 6px; border: 1px solid #ccc; text-align: left;\"> " + metrics.getAsyncAvg());
    sb.append("</td>");
    sb.append("</tr>");
}
sb.append("</table>");
sb.append("</body>");
sb.append("</html>");

System.out.println(sb.toString());

Somehow, the code looks pretty ugly to me in the way HTML and CSS are being used within StringBuilder. I am opting for code review to see whether we can improve anything here. Is there any better way of writing this code?

share|improve this question
    
I am sure there are plenty of libraries that do this, but you could do it yourself: Write objects that are abstractions of the HTML-elements you are creating. That makes everything much easier and safer, at the cost of some performance. The first thing a google search found was "Gagawa" for example. –  Traubenfuchs 2 days ago

3 Answers 3

up vote 15 down vote

  1. How about extracting the style=""s into the head?

    sb.append("<style>" +
"td { padding: 6px; border: 1px solid #ccc; text-align: left; }" + 
"th { background: #333; color: white; font-weight: bold; padding: 6px; border: 1px solid #ccc; text-align: left;}" +
"</style>");

  2. You should write helper methods, that do the 'heavy lifting' for you:

    void appendTag(StringBuilder sb, String tag, String contents) {
    sb.append('<').append(tag).append('>');
    sb.append(contents);
    sb.append("</").append(tag).append('>');
}
void appendDataCell(StringBuilder sb, String contents) {
    appendTag(sb, "td", contents);
}
void appendHeaderCell(StringBuilder sb, String contents) {
    appendTag(sb, "th", contents);
}

Please note, that if you are using StringBuilder you should not concatenate strings with +. You want to get rid of string concatenation, not do it.

If you want to get even more expressive you should take a look at existing template engines. They are optimized and mostly easy to use.

Suggesting mustache:

If you are using mustache, you can write a template like this:

<html>
  <head>
    <title>Table</title>
    <style>...</style>
  </head>
  <body>
    <table>
     <tr><th>....</th></tr>
    {{#metrics}}
      <tr>
        <td>{{getName}}</td>
        ...
      </tr>
    {{/metrics}}
  </body>
</html>

and then render it via e.g.

public static void main(String[] args) throws IOException {
    HashMap<String, Object> scopes = new HashMap<String, Object>();
    scopes.put("metrics", dataMetricsList);

    Writer writer = new OutputStreamWriter(System.out);
    MustacheFactory mf = new DefaultMustacheFactory();
    Mustache mustache = mf.compile(new StringReader(getTemplateCodeFromAbove()), "example");
    mustache.execute(writer, scopes);
    writer.flush();
}

It depends on your use case, of course. If all you ever want is to render a single table, you are fine with StringBuilder. But if you find yourself repeating this, get an external library for it.

share|improve this answer
    
Thanks a lot thriqon. I was not able to understand what do you mean by existing template engines. Can you provide an example what do you mean by that and how it will be useful to me? –  Webby 2 days ago
4  
A templating language like Mustache allows you to have a pre-written template, with placeholders for information, and then fill that information from Java, and render the result. It's a form of separation between logic and presentation which is invaluable when scaling. –  Madara Uchiha 2 days ago
    
Thanks @MadaraUchiha, important clarification, I forgot about that. –  thriqon 2 days ago
    
Or, failing a templating engine, some library to write HTML in Java - like jmesa. +100 for excellent suggestions to remove duplication and separate concerns. –  Boris the Spider 2 days ago
up vote 11 down vote

Try to get yourself into the habit of deeply hating duplicated elements in your code. 2 lines like these would compel me to refactor immediately:

sb.append("<th style = \"background: #333; color: white; font-weight: bold; padding: 6px; border: 1px solid #ccc; text-align: left;\"> ClientName");
sb.append("<th style = \"background: #333; color: white; font-weight: bold; padding: 6px; border: 1px solid #ccc; text-align: left;\"> SyncCount");

And you don't just have 2 lines, you have dozens. This is criminal behavior, and unacceptable in code for any purpose.

DOWN with copy-paste coding!

share|improve this answer
8  
weknowmemes.com/wp-content/uploads/2012/03/… –  Madara Uchiha 2 days ago
1  
While I deeply agree - copy-pasta-coding is the root of all evil - the issue with this "answer" is simply that it's not an answer. Paraphrased without funny comic: Don't do that. Refactor. Why has this answer gotten so many up-votes? Or am I totally misunderstanding this site? Shouldn't a real answer have more actual advice? –  WernerCD 2 days ago
    
I think it's an appropriate answer, but you're free to disagree. I added the graphics for fun, especially since today is Friday. Does it reduce the value of the answer? I don't think so. Did it boost unusually high popularity? Probably, yes! I had a feeling this might happen, and believe it or not, I was following this question during the day, ready to delete my answer in case it overtakes the better answer. The fact that it didn't, is a good sign about the sense of value of the community. I'd like to think we all had fun, and that more of us will think twice before copy-pasting again. –  janos 2 days ago
2  
This answer would be much better if you showed an example of how to refactor the highlighted code. –  Brian S 2 days ago
    
@WernerCD - this answer scores high ... because pictures!!! –  rolfl♦ 2 days ago
up vote 1 down vote

I have solved this problem by creating a few trivial helper classes called TableData, TableRow and TableCell. I then use those to build a table with Java objects. Those objects have javax.xml.bind.annotation.XmlValue and such annotations in them, so that the table can easily be serialized into XML using JAXB. That XML can then be further processed into any kind of HTML by an XSLT transformation, which is surprisingly simple to do in Java.

I've got an open source project at Bitbucket that uses this method. The HtmlFormatter class takes in a TableData object and outputs an HTML string after the process I just described (using the xmlToHtml.xsl XSL file).

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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