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 10 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 Sep 19 '14 at 13:03

3 Answers 3

up vote 19 down vote accepted

  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? –  lining Sep 19 '14 at 6:41
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 Sep 19 '14 at 6:57
    
Thanks @MadaraUchiha, important clarification, I forgot about that. –  thriqon Sep 19 '14 at 7:24
    
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 Sep 19 '14 at 13:03
up vote 3 down vote

thriqon's Answer is a good one, you should move your style code somewhere, but the table headers aren't far enough, you should move all your style into a CSS file that would look like this

table th {
    background: #333; 
    color: white; 
    font-weight: bold; 
    padding: 6px; 
    border: 1px solid #ccc; 
    text-align: left;
}

table td {
    padding: 6px; 
    border: 1px solid #ccc; 
    text-align: left;
}

If you have more than one table than you should look at either creating an ID for each Table or classes for the Tables that have the same Style.

By Doing this, you can change the theme of the page with having to go through the code, you just change the Stylesheet.

After the CSS(styling) is all sorted out you can change your code a little bit, cleaning it up by removing lines of code completely.

StringBuilder sb = new StringBuilder();
sb.append("<html>");
sb.append("<head>");
sb.append("</head>");
sb.append("<table>");
sb.append("<th> ClientName </th>");
sb.append("<th> SyncCount </th>");
sb.append("<th> SyncPercentile </th>");
sb.append("<th> SyncAvg </th>");
sb.append("<th> SyncMax </th>");
sb.append("<th> AsyncCount </th>");
sb.append("<th> AsyncPercentile </th>");
sb.append("<th> AsyncAvg </th>");

for (DataMetrics metrics : dataMetricsList) {
    sb.append("<tr>");
    sb.append("<td> " + metrics.getName() + " </td>");
    sb.append("<td> " + metrics.getSyncCall() + " </td>");
    sb.append("<td> " + metrics.getSyncPercent() + " </td>");
    sb.append("<td> " + metrics.getSyncAvg() + " </td>");
    sb.append("<td> " + metrics.getSyncMax() + " </td>");
    sb.append("<td> " + metrics.getAsyncCall() + " </td>");
    sb.append("<td> " + metrics.getAsyncPercent() + " </td>");
    sb.append("<td> " + metrics.getAsyncAvg() + " </td>");
    sb.append("</tr>");
}
sb.append("</table>");
sb.append("</body>");
sb.append("</html>");

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

you really don't need to have a line just for the end tags (th and td) they can share.

This looks a lot cleaner already.

I do agree that a tag helper would be really nice, and the Template Engine looks like a really nice way to go as well.

share|improve this answer
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.