current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

The basic requirement of my code is to accept a file input, parse it, convert it to a java type (A DTO essentially) for use in another component of the system.

I should also note that I am not actually parsing a file from a filesystem, the FileParser will be used by a Message Driven Bean that has the contents of the file as a String already.

I am looking for input and general design/code review, thanks for your time.

I have 4 classes:

  1. FileParser
  2. Message
  3. MessageFactory
  4. MessageFieldTypes

The format of the file is as follows:

FIELDONETYPE/FIELDONEVALUE
FIELDTWOTYPE/FIELDTWOVALUE
FIELDTHREETYPE/FIELDTHREEVALUE
FIELDFOURTYPE/FIELDFOURVALUE
FIELDFIVETYPE/FIELDFIVEVALUE

etc.

There are roughly 20 fields.

My initial thought was to parse the file into a hashmap and then create the object. Here is the code listing for the parser:

final public class FileParser 
{
    public static final Message parse(String text) {    

    Map<MessageFieldTypes, String> map = new LinkedHashMap<MessageFieldTypes, String>();

    for(String line : text.split(" *\n *")) {
        String[] keyValuePair = line.split(" */ *");
        map.put(MessageFieldTypes.valueOf(keyValuePair[0]),
                keyValuePair.length == 1 ? "" : keyValuePair[1]);
    }

    return MessageFactory.createFromHashMap(map);
    }
}

Here is the code listing for the factory:

public class MessageFactory {

    private static final DateTimeFormatter DATE_TIME_FORMATTER = DateTimeFormat.forPattern("ddMMMyyyy");

    public static Message createFromHashMap(Map<MessageFieldTypes, String> map) {
        Message message = new Message();

        message.setFieldOne(new FieldOneType(map.get(MessageFieldTypes.FIELDONE),
                            map.get(MessageFieldTypes.FIELDTWO), 
                            DATE_TIME_FORMATTER.parseDateTime(map.get(MessageFieldTypes.FIELDTHREE)).toLocalDate()));       
        message.setFieldFour(new FieldFourType(map.get(MessageFieldTypes.FIELDFOUR)));
        message.setFieldFive(map.get(MessageFieldTypes.FIELDFIVE)));

        return message;
    }
}

And the Enum:

public enum MessageFieldTypes {
    FIELDONE,
    FIELDTWO,
    FIELDTHREE,
    FIELDFOUR,
    FIELDFIVE
}
share|improve this question
    
Not a criticism, just curious - any special reason for using a LinkedHashMap? –  Michael K Jul 25 '11 at 4:38
    
Yes, this I believe this is a mistake. I think initially I had thought about putting each value in an insertion ordered list (ArrayList or LinkedList) and just relying on the format of the message remaining constant to determine each field. The fact that this carried over to the map was probably just that the idea was in my mind. –  Abarax Jul 25 '11 at 4:55

1 Answer 1

up vote 2 down vote

If you can change your file format slightly (to type=value or type:value) I'd suggest using the Java Properties class. It can parse the file from a stream for you (Untested code):

final public class FileParser 
{
    public static final Message parse(String text) {
        text = text.replace("/", ":");   
        Properties typeValues = new Properties();
        typeValues.load(new StringReader(text));

        return MessageFactory.createFromHashTable(typeValues);
    }
}

Note that Properties extends HashTable, not HashMap. They chose HashTable because HashMap is not thread-safe. They work pretty much the same way, though.

On structure: I like the factory format, and it makes sense to use it here. You didn't mention whether the field types are known; if they are your factory code is fine. (It looks like they are, since you call several different constructors with fixed numbers of arguments.)

share|improve this answer
    
Thanks, that is very useful information. Unfortunately we do not have control over the format. We could pre-process it though and convert it to the properties format. Do you have any comments on the class structure? i.e. using a factory vs parsing inside the DTO itself etc. –  Abarax Jul 25 '11 at 4:00
    
I've updated the answer, also adding a simple "/" to ":" replacement should you choose the properties route. I think your code structure is fine. The factory could be a simple instantiation, but I think the code is clearer with it. –  Michael K Jul 25 '11 at 4:37

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.