- "Dirty" Code Is Evil - or at Least Unprofessional
- A Simple Example
- Let's Try That Again
- Now We Need a Helper
- It Works, But It Sure Isn't Pretty
- How This Technique Helps
Like this article? We recommend
Like this article? We recommend
Like this article? We recommend
It Works, But It Sure Isn't Pretty
This compiles and passes the tests, but it's ugly. Complex expressions are hard to read. It's easy to miss subtle errors inside them. I want to refactor this expression to make it easier to read:
public static Element
toXML(Customer c) { Element customer =
new Element("customer"); customer.addContent
(makeElement("name", c.name)); return customer; } private static Element
makeElement(String tag, String content) { return new Element(tag).setText(content); }
That's a little better. Now let's do the same for the test:
Element e = CustomerXMLConverter.toXML(c);
assertEquals("customer", e.getName());
assertEquals("John Smith", getElementText(e, "name"));
}
private String getElementText(Element e, String tag)
{
return e.getChild(tag).getTextTrim();
}
Again, that's a little better. Let's continue to add tests:
public void testToXML() throws Exception
{
Customer c = new Customer();
c.name = "John Smith";
c.address = "55 Somewhere, City, State Zip";
c.email = "jsmith@somewhere.com";
c.phone = "111-222-3333";
c.fax = "444-555-6666";
c.cellPhone = "777-888-9999";
Element e = CustomerXMLConverter.toXML(c);
assertEquals("customer", e.getName());
assertEquals("John Smith", getElementText(e, "name"));
assertEquals("55 Somewhere, City, State Zip",
getElementText(e, "address"));
assertEquals("jsmith@somewhere.com", getElementText(e, "email"));
}
This fails, as expected. It's also pretty messy. I've shown the whole test above so you can see that there's lots of duplication and confusion in there. But before I fix that, I'd better make it work. Remember the rule: First make it work, then make it right. (A software proverb often attributed to Kent Beck. Another common rendition is "Make it work, make it right, make it fast.")
public static Element toXML(Customer c)
{
Element customer = new Element("customer");
customer.addContent(makeElement("name", c.name));
customer.addContent(makeElement("address", c.address));
customer.addContent(makeElement("email", c.email));
return customer;
}
The test passes. Now let's clean up the test a bit. What bothers me most is the duplication of the strings. We need to pull those strings out into variables and clean things up a little.
public void testToXML() throws Exception
{
final String NAME = "John Smith";
final String ADDRESS = "55 Somewhere, City, State Zip";
final String EMAIL = "jsmith@somewhere.com";
final String BUS_PHONE = "111-222-3333";
final String FAX = "444-555-6666";
final String CELL = "777-888-9999";
Customer c = new Customer();
c.name = NAME;
c.address = ADDRESS;
c.email = EMAIL;
c.phone = BUS_PHONE;
c.fax = FAX;
c.cellPhone = CELL;
Element e = CustomerXMLConverter.toXML(c);
assertEquals("customer", e.getName());
assertEquals(NAME, getElementText(e, "name"));
assertEquals(ADDRESS, getElementText(e, "address"));
assertEquals(EMAIL, getElementText(e, "email"));
}
This looks pretty good.
I'm also bothered by the structure of the toXML method, though. All those lines that add elements to customer look repetitive and confusing. As an improvement, let's create a new function that just adds a tag and value to an element:
public class CustomerXMLConverter
{
public static Element toXML(Customer c)
{
Element customer = new Element("customer");
addElement(customer, "name", c.name);
addElement(customer, "address", c.address);
addElement(customer, "email", c.email);
return customer;
}
private static Element makeElement(String tag, String content)
{
return new Element(tag).setText(content);
}
private static void addElement(Element e, String tag, String content)
{
e.addContent(makeElement(tag, content));
}
}
The test still passes, and the structure isn't too bad, although I don't like the fact that the customer argument is repeated so many times. We might be better off turning CustomerXMLConverter into an object instead of a class with lots of static methods. Let's try that.
public class CustomerXMLConverter
{
private Element customerElement;
public Element toXML(Customer customer)
{
customerElement = new Element("customer");
addToCustomer("name", customer.name);
addToCustomer("address", customer.address);
addToCustomer("email", customer.email);
return customerElement;
}
private static Element makeElement(String tag, String content)
{
return new Element(tag).setText(content);
}
private void addToCustomer(String tag, String content)
{
customerElement.addContent(makeElement(tag, content));
}
}
This removes some of the repetition, but at the expense of extra complexity. Let's see what happens when we add the next test:
Element e = new CustomerXMLConverter().toXML(c);
assertEquals("customer", e.getName());
assertEquals(NAME, getElementText(e, "name"));
assertEquals(ADDRESS, getElementText(e, "address"));
assertEquals(EMAIL, getElementText(e, "email"));
Element phones = e.getChild("phones");
assertEquals(BUS_PHONE, getElementText(phones, "business"));
This fails, as expected. We can make it pass by adding the business phone element in the toXML method.
public Element toXML(Customer customer)
{
customerElement = new Element("customer");
addToCustomer("name", customer.name);
addToCustomer("address", customer.address);
addToCustomer("email", customer.email);
Element phones = new Element("phones");
phones.addContent(makeElement("business", customer.phone));
customerElement.addContent(phones);
return customerElement;
}
This makes the test pass, but isn't very gratifying. We've got that addContent/makeElement pair back again. We were probably better off before we made the addToCustomer change. Let's change it back:
public static Element toXML(Customer customer)
{
Element customerElement = new Element("customer");
addToElement(customerElement, "name", customer.name);
addToElement(customerElement, "address", customer.address);
addToElement(customerElement, "email", customer.email);
Element phones = new Element("phones");
addToElement(phones, "business", customer.phone);
customerElement.addContent(phones);
return customerElement;
}
The test passes, and this looks pretty good. The addToElement function is going to be useful beyond just adding elements to the customerElement. We can use it to add elements to the phones.
Okay, let's finish out the test:
Element phones = e.getChild("phones");
assertEquals(BUS_PHONE, getElementText(phones, "business"));
assertEquals(FAX, getElementText(phones, "fax"));
assertEquals(CELL, getElementText(phones, "cell"));
And then make it pass:
Element phones = new Element("phones");
customerElement.addContent(phones);
addToElement(phones, "business", customer.phone);
addToElement(phones, "fax", customer.fax);
addToElement(phones, "cell", customer.cellPhone);
The tests run.
Now let's look the whole batch of code over one last time to see if there is any cleanup to be done. Yes, I found one thing: The two lines that add the phones sub-element aren't clear. I'll create a function to make it obvious what's going on. Here's the final version of the code:
public class CustomerXMLConverter
{
public static Element toXML(Customer customer)
{
Element customerElement = new Element("customer");
addToElement(customerElement, "name", customer.name);
addToElement(customerElement, "address", customer.address);
addToElement(customerElement, "email", customer.email);
Element phones = addSubElement(customerElement, "phones");
addToElement(phones, "business", customer.phone);
addToElement(phones, "fax", customer.fax);
addToElement(phones, "cell", customer.cellPhone);
return customerElement;
}
private static Element addSubElement(Element customerElement, String tag)
{
Element subElement = new Element(tag);
customerElement.addContent(subElement);
return subElement;
}
private static Element makeElement(String tag, String content)
{
return new Element(tag).setText(content);
}
private static void addToElement(Element e, String tag, String content)
{
e.addContent(makeElement(tag, content));
}
}
public class CustomerTest extends TestCase
{
public void testToXML() throws Exception
{
final String NAME = "John Smith";
final String ADDRESS = "55 Somewhere, City, State, Zip";
final String EMAIL = "jsmith@somwhere.com";
final String BUS_PHONE = "111-222-3333";
final String FAX = "444-555-6666";
final String CELL = "777-888-9999";
Customer c = new Customer();
c.name = NAME;
c.address = ADDRESS;
c.email = EMAIL;
c.phone = BUS_PHONE;
c.fax = FAX;
c.cellPhone = CELL;
Element e = CustomerXMLConverter.toXML(c);
assertEquals("customer", e.getName());
assertEquals(NAME, getElementText(e, "name"));
assertEquals(ADDRESS, getElementText(e, "address"));
assertEquals(EMAIL, getElementText(e, "email"));
Element phones = e.getChild("phones");
assertEquals(BUS_PHONE, getElementText(phones, "business"));
assertEquals(FAX, getElementText(phones, "fax"));
assertEquals(CELL, getElementText(phones, "cell"));
}
private String getElementText(Element e, String tag)
{
return e.getChild(tag).getTextTrim();
}
}