Question

I was asked to work on this back-end scheduled job that export some customers data (from an e-commerce DB) to a custom-format text file. The code that follows is what I found.

I just would like to delete it all, but I can't. Would it be possible for me to improve this without changing it so much?

public class AConverter implements CustomerConverter {

    protected final Logger LOG = LoggerFactory.getLogger(AConverter.class);

    private final static String SEPARATOR = ";";
    private final static String CR = "\n";

    public String create(Customer customer) {

        if (customer == null)
            return null;

        LOG.info("Exporting customer, uidpk: {}, userid: {}", customer.getUidPk(), customer.getUserId());

        StringBuilder buf = new StringBuilder();

        buf.append("<HEAD>");
        buf.append(SEPARATOR);
        buf.append(String.valueOf(customer.getUidPk()));
        buf.append(SEPARATOR);
        byte[] fullName = null;
        try {
            fullName = customer.getFullName().getBytes("UTF-8");
        } catch (UnsupportedEncodingException e1) {
            fullName = customer.getFullName().getBytes();
        }
        String name = null;
        try {
            name = new String(fullName, "UTF-8");
        } catch (UnsupportedEncodingException e) {
            name = customer.getFullName();
        }
        buf.append(limitString(name, 40));
        buf.append(SEPARATOR);
        final CustomerAddress preferredShippingAddress = customer.getPreferredShippingAddress();
        if (preferredShippingAddress != null) {
            final String street1 = preferredShippingAddress.getStreet1();
            if (street1 != null) {
                buf.append(limitString(street1, 40));
            }
        } else {
            buf.append(" ");
        }
        buf.append(SEPARATOR);

        final String addressStr = buildAddressString(customer);
        buf.append(limitString(addressStr, 40));
        buf.append(SEPARATOR);
        buf.append(limitString(customer.getEmail(), 80));
        buf.append(SEPARATOR);
        if (preferredShippingAddress!=null && preferredShippingAddress.getStreet2() != null) {
            buf.append(limitString(preferredShippingAddress.getStreet2(), 40));
        } else {
            buf.append(" ");
        }
        buf.append(SEPARATOR);
        buf.append(limitString(customer.getPhoneNumber(), 25));
        buf.append(SEPARATOR);
        if (preferredShippingAddress!=null) {
            if(preferredShippingAddress.getCountry()!=null) {
                buf.append(preferredShippingAddress.getCountry());
            } else {
                buf.append(" ");
            }
        } else {
            buf.append(" ");
        }
        buf.append(SEPARATOR);
        if (preferredShippingAddress!=null) {
            if(preferredShippingAddress.getCountry()!=null) {
                buf.append(preferredShippingAddress.getCountry());
            } else {
                buf.append(" ");
            }
        } else {
            buf.append(" ");
        }
        buf.append(SEPARATOR);

        String fodselsnummer = " ";
        try {
            Map<String, AttributeValue> profileValueMap = customer.getProfileValueMap();
            AttributeValue attributeValue = profileValueMap.get("CODE");
            fodselsnummer = attributeValue.getStringValue();
        } catch (Exception e) {
        }
        buf.append(fodselsnummer);
        buf.append(CR);
        final String string = buf.toString();

        return string;

    }

    private String buildAddressString(Customer customer) {
        final CustomerAddress preferredShippingAddress = customer.getPreferredShippingAddress();
        if (preferredShippingAddress != null) {
            final String zipOrPostalCode = preferredShippingAddress.getZipOrPostalCode();
            final String city = preferredShippingAddress.getCity();
            if (zipOrPostalCode != null && city != null) {
                return zipOrPostalCode + " " + city;
            } else if(zipOrPostalCode == null && city != null) {
                return city;
            } else if(zipOrPostalCode != null && city == null) {
                return zipOrPostalCode;
            }
        }
        return " ";
    }

    private String limitString(String value, int numOfChars) {
        if (value != null && value.length() > numOfChars)
            return value.substring(0, numOfChars);
        else
            return value;
    }

}
Was it helpful?

Solution

You say you want to improve it, you'd like to delete it, but you can't. I'm not sure why you can't. I also don't understand why you'd want to delete it. But it sounds to me like the kind of attitude I used to have before I read Refactoring by Martin Fowler. I would strongly suggest you read that book, if you haven't already.

It is certainly possible to improve this code (or any code) without rewriting it all. The most obvious improvements would be to eliminate some of the repetitive code in the create method by creating some utility methods, and then breaking up the create method into several smaller methods à la template methods.

Also, there is a questionable bit of code in the create method that turns the customer's name into a UTF-8 byte stream and then back into a string. I can't imagine what that's for. Finally, it returns null if the customer is null. That is unlikely to be necessary or wise.

For fun, I decided to do a little refactoring on this code. (Note that proper refactoring involves unit tests; I don't have any tests for this code and have not even compiled the code below, much less tested it.) Here is one possible way you could rewrite this code:

public class AConverter implements CustomerConverter {
    protected final Logger LOG = LoggerFactory.getLogger(AConverter.class);

    private final static String SEPARATOR = ";";
    private final static String CR = "\n";

    public String create(Customer customer) {
        if (customer == null) throw new IllegalArgumentException("no cust");

        LOG.info("Exporting customer, uidpk: {}, userid: {}",
                customer.getUidPk(), customer.getUserId());

        StringBuilder buf = new StringBuilder();
        doHead(buf, customer);
        doAddress(buf, customer);
        doTail(buf, customer);
        return buf.toString();
    }

    private void doHead(StringBuilder buf, Customer customer) {
        append(buf, "<HEAD>");
        append(buf, String.valueOf(customer.getUidPk()));
        append(buf, limitTo(40, customer.getFullName()));
    }

    private void doAddress(StringBuilder buf, Customer customer) {
        append(buf, limitTo(40, street1of(customer)));
        append(buf, limitTo(40, addressOf(customer)));
        append(buf, limitTo(80, customer.getEmail()));
        append(buf, limitTo(40, street2of(customer)));
        append(buf, limitTo(25, customer.getPhoneNumber()));
        append(buf, countryOf(customer));
        append(buf, countryOf(customer));
    }

    private void doTail(StringBuilder buf, Customer customer) {
        buf.append(fodselsnummerOf(customer));
        buf.append(CR);
    }

    private void append(StringBuilder buf, String s) {
        buf.append(s).append(SEPARATOR);
    }

    private String street1of(Customer customer) {
        final CustomerAddress shipto = customer.getPreferredShippingAddress();
        if (shipto == null) return " ";
        if (shipto.getStreet1() != null) return shipto.getStreet1();
        return " ";
    }

    private String street2of(Customer customer) {
        final CustomerAddress shipto = customer.getPreferredShippingAddress();
        if (shipto == null) return " ";
        if (shipto.getStreet2() != null) return shipto.getStreet2();
        return " ";
    }

    private String addressOf(Customer customer) {
        final CustomerAddress shipto = customer.getPreferredShippingAddress();
        if (shipto == null) return " ";

        final String post = preferredShippingAddress.getZipOrPostalCode();
        final String city = preferredShippingAddress.getCity();

        if (post != null && city != null) return post + " " + city;
        if (post == null && city != null) return city;
        if (post != null && city == null) return post;
        return " ";
    }

    private String countryOf(Customer customer) {
        final CustomerAddress shipto = customer.getPreferredShippingAddress();
        if (shipto == null) return " ";
        if (shipto.getCountry() != null) return shipto.getCountry();
        return " ";
    }

    private String limitTo(int numOfChars, String value) {
        if (value != null && value.length() > numOfChars)
            return value.substring(0, numOfChars);
        return value;
    }

    private String fodelsnummerOf(Customer customer) {
        try {
            Map<String, AttributeValue> profileValueMap =
                customer.getProfileValueMap();
            AttributeValue attributeValue = profileValueMap.get("CODE");
            return attributeValue.getStringValue();
        } catch (Exception e) {
            return " ";
        }
    }
}

I also notice that there is a problem with your format for the custom-format text file if any of the fields of the customer data (email address, for example) happen to have a semicolon in them, because that is your separator character. I trust that is a known issue?

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top