8
\$\begingroup\$

I have to create an XML body for a web service call. I want to use:

private void createProductBody(Product product, StringBuilder body) {
    body.append(WSConstants.WS_PRODUCT_ID_START_TAG)
            .append(product.getId())
            .append(WSConstants.WS_PRODUCT_ID_END_TAG)
            .append(WSConstants.WS_SIZE_TEXT_START_TAG)
            .append(product.getSize())
            .append(WSConstants.WS_SIZE_TEXT_END_TAG);
}

But some colleagues state that this writing is hard to read and they want us to use this:

private void createProductBody(Product product, StringBuilder body) {
    body.append("<ProductId>").append(product.getId()).append("</ProductId>")
        .append("<Size>").append(product.getSize()).append("</Size>");
}

Which are the pro and cons for using hardcoded values?

\$\endgroup\$
  • \$\begingroup\$ Is WSConstants a class that you wrote, or is it part of some library that you are using? \$\endgroup\$ – 200_success Dec 8 '14 at 11:08
  • \$\begingroup\$ WSConstants is a class I wrote for keeping constants. \$\endgroup\$ – Laura Dec 8 '14 at 12:13
  • 2
    \$\begingroup\$ Better to use an XmlBuilder. You need to be careful constructing XML by hand because the values may need to be escaped. E.g. if a product id were changed to be something like this "Mills & Boon"your XML would be invalid. The ampersand needs to be escaped as &amp;. In your example, it's probably OK because the values are probably only numbers. \$\endgroup\$ – ᴇʟᴇvᴀтᴇ Dec 8 '14 at 12:46
13
\$\begingroup\$

They're right. While "</Whatever>" is hard to read itself (YMMV), the constants make it even worse.

Magical constants in a code are bad and should usually be extracted as you did. But if there are many of them and each gets used just once, it's counterproductive.

The best solution would be something like

new XmlBuilder(body)
    .start("ProductId").append(product.getId()).end()
    .start("Size").append(product.getSize()).end();

or some of the million existing XML tools. If the constants get used multiple times, then something like

new XmlBuilder(body)
    .start(WSConstants.WS_PRODUCT_ID).append(product.getId()).end()
    .start(WSConstants.WS_SIZE).append(product.getSize()).end();

makes more sense. Then you may consider using static imports to keep it shorter.

The whole XmlBuilder is rather trivial, it mostly delegates to the StringBuilder and remembers the opened tags using a stack. As stated by aetheria, it should also escape the appended values when needed.


For redundancy lovers, something like

new XmlBuilder(body)
    .start(WS_PRODUCT_ID).append(product.getId()).end(WS_PRODUCT_ID)
    .start(WS_SIZE).append(product.getSize()).end(WS_SIZE);

may be more "cool". Anyway, it's more XML-like (just don't ask me what's good for).


Note the inconsistency of "PRODUCT_ID" vs. "SIZE_TEXT", where the former means indeed "productId" while the latter means just "size". This should be fixed (like I did in my snippets).

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for your answer and suggestions. They are really helpful. \$\endgroup\$ – Laura Dec 8 '14 at 12:18

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.