I am trying to refactor some legacy code. The task here is to construct lengthy messages/strings based on some pre-defined template that looks like this:
field1,8,String
filed2,5,Integer
field3,12,String
......
Then I am handed a java object that has all those fields. What needs to be done here is simply get data from the object fields and use them to construct a long message/string based on the template. Some of these fields also may be converted based on some simple rules. For example:
abc => a
def => d
ghi => g
As a result we need to check the values of these fields from time to time. Also there are rules about padding (mostly adding empty space to the right). So a conostructed message/string may look like this:
uater 4751 enterprise ......
Currently we are just using brutal force to do this job. First we feed the template into an ArrayList, each element is a line, eg, "field1,8,String". During the actual message construction, we loop through this ArrayList, and then fill the data into a StringBuffer. Here is some sample snippet
StringBuffer message = new StringBuffer(1000);
for (String field : templateFields) {
String[] fieldArray = field.split(Constants.SEPARATOR);
if (fieldArray[0].equalsIgnoreCase(Constants.WORKFLOW)) {
message.append(rightPad(object.getFieldOne(), Integer.parseInt(fieldArray[1])));
} else if (fieldArray[0].equalsIgnoreCase(Constants.WORKVOLUME)) {
message.append(rightPad(object.getFieldTwo(), Integer.parseInt(fieldArray[1]));
} else if (fieldArray[0].equalsIgnoreCase(Constants.WORKTYPE)) {
if (object.getFieldThree().equalsIgnoreCase("abc")) {
message.append(rightPad("a", Integer.parseInt(fieldArray[1]));
} else if (object.getFieldThree().equalsIgnoreCase("def")) {
message.append(rightPad("d", Integer.parseInt(fieldArray[1]));
} else {
message.append(rightPad("g", Integer.parseInt(fieldArray[1]));
}
} else if ......
}
As you can see, as hidious as it is, it gets the job done. But such code is error-prone, and is hard to maintain. I wonder if you guys have any tools or libraries or some elegant solutions to recommend.
Thanks so much! Hua
If I understand your question right, you have an approach where you are looping over possible templateFields
. That's not necessary.
Since every fieldArray[0]
is compared to some Constants
values and in case of a match is processed further, we can replace the for-loop by a Map
. Its keys are the possible Constants
values and its values are mappers. A mapper is a BiFunction
which takes the object
and the value of fieldArray[1]
and returns for these a message of type String
.
Let's start with the mappers:
public class FieldToMessageMapper {
private static final Map<String, Function<String, String>> WORKINGTYPE_MESSAGE_MAPPER = new HashMap<>();
static {
WORKINGTYPE_MESSAGE_MAPPER.put("abc", fieldArray1 -> rightPad("a", Integer.parseInt(fieldArray1)));
WORKINGTYPE_MESSAGE_MAPPER.put("def", fieldArray1 -> rightPad("d", Integer.parseInt(fieldArray1)));
WORKINGTYPE_MESSAGE_MAPPER.put("DEFAULT", fieldArray1 -> rightPad("g", Integer.parseInt(fieldArray1)));
}
private static Map<String, BiFunction<MyObject, String, String>> MESSAGE_MAPPER = new HashMap<>();
static {
MESSAGE_MAPPER.put(Constants.WORKFLOW, (o, fieldArray1) -> rightPad(o.getFieldOne(), Integer.parseInt(fieldArray1)));
MESSAGE_MAPPER.put(Constants.WORKVOLUME, (o, fieldArray1) -> rightPad(o.getFieldTwo(), Integer.parseInt(fieldArray1)));
MESSAGE_MAPPER.put(Constants.WORKTYPE,
(o, fieldArray1) -> WORKINGTYPE_MESSAGE_MAPPER.getOrDefault(o.getFieldThree().toLowerCase(), WORKINGTYPE_MESSAGE_MAPPER.get("DEFAULT")).apply(fieldArray1));
}
public static Optional<String> map(MyObject o, String fieldArray0, String fieldArray1) {
return Optional.ofNullable(MESSAGE_MAPPER.get(fieldArray0.toLowerCase()))
.map(mapper -> mapper.apply(o, fieldArray1));
}
private static String rightPad(String string, int pad) {
// TODO right pad
return string;
}
}
We do not return a mapper itself. FieldToMessageMapper
offers the method map
which does the mapping. It returns an Optional<String>
which shows the result might be empty if there is no mapping for the input.
To ensure to get a mapper independent of the characters case, all keys are String..toLowerCase()
.
Let's go on with the overall processing:
protected StringBuffer process(Collection<String> templateFields, MyObject object) {
StringBuffer message = new StringBuffer(1000);
for (String field : templateFields) {
String[] fieldArray = field.split(Constants.SEPARATOR);
String msg = FieldToMessageMapper.map(object, fieldArray[0], fieldArray[1])
.orElseThrow(() -> new IllegalArgumentException(String.format("Unsupported field %s", field)));
message.append(msg);
}
return message;
}
I don't know how you need to handle missing mappings. I choose fail fast by throwing an exception.
Please note: StringBuffer
is
A thread-safe, mutable sequence of characters. A string buffer is like a
String
, but can be modified.
If your processing isn't multithreaded you could use StringBuilder
. If the result isn't modified further, you could use String
.
Let me show a further alternative using Stream
which returns a String
:
protected String process(Collection<String> templateFields, MyObject object) {
return templateFields.stream()
.map(field -> field.split(Constants.SEPARATOR))
.map(fieldArray -> FieldToMessageMapper.map(object, fieldArray[0], fieldArray[1])
.orElseThrow(() -> new IllegalArgumentException(String.format("Unsupported field %s", Arrays.toString(fieldArray)))))
.collect(Collectors.joining());
}
If I got the code from the question right, there should be the following implementation of Constants
:
public class Constants {
public static final String SEPARATOR = ",";
public static final String WORKFLOW = "field1";
public static final String WORKVOLUME = "filed2";
public static final String WORKTYPE = "field3";
}
EDIT:
If you want to have a configuration approach you can elaborate this code further to use Spring configuration:
MessageMapper
which has two methods: String getKey()
and String map(MyObject o, String fieldArray1)
. getKey()
returns the Constants
value for which the mapper provides the mapping.MESSAGE_MAPPER
using this interface.CommonMessageMapper
which has a constructor CommonMessageMapper(MessageMapper... messageMappers)
. The messageMappers
has to be put in a Map<String, BiFunction<MyObject, String, String>> mappers
like: mappers.put(messageMapper.getKey(), messageMapper)
. Define a method String map(MyObject o, String fieldArray0, String fieldArray1)
which will lookup the appropriate MessageMapper mm
using fieldArray0
: MessageMapper mm = mappers.get(fieldArray0)
. Invoke then mm.map(o, feldArray1)
. (You may use here also an Optional
to handle the case when no appropriate mapper is present.)MessageMapper
and the CommonMessageMapper
have to be annotated as Bean
or Component
. The constructor of CommonMessageMapper
has to be annotated with @Autowired
.@Configuration
) which will inject the desired MessageMapper
into a CommonMessageMapper
and has a factory method for such a CommonMessageMapper
.CommonMessageMapper
instead of FieldToMessageMapper
above.