Which design pattern to use to improve this Java program

Question

I write a program to parse an XML file to get a specific tag value named SerialNum which is contained in a Header tag. The file is constructed as below:

it contains 1 Header and 1 Body
the Header may contains many SerialNum tags. We need to extract the value of the last tag.

I used the Stax parser to get the SerialNum value, and I wrote this code:

public String findIdValue(HttpServletRequest request) {

    String serialNumberValue = null;

    if(request != null){
        ServletInputStream servletInstream;
        try {
            servletInstream = request.getInputStream();
            XMLInputFactory factory = XMLInputFactory.newInstance();
            XMLStreamReader xmlStreamReader = factory.createXMLStreamReader(servletInstream);
            //begin parsing if we get <Header>
            //end parsing if we get <Header/> or </Header>

            int event = xmlStreamReader.getEventType();
            boolean enableToParse = false;
            boolean gotSerialNumber = false;
            boolean parseComplete = false;

            while( (xmlStreamReader.hasNext()) && (!parseComplete) ){
                switch(event) {
                case XMLStreamConstants.START_ELEMENT:
                    if("Header".equals(xmlStreamReader.getLocalName())){
                        //tag is header, so begin parse
                        enableToParse = true;
                    }else if(("SerialNum".equals(xmlStreamReader.getLocalName())) && (enableToParse) ){
                        //tag is serialNum so enable to save the value of serial number
                        gotSerialNumber = true;
                    }
                    break;
                case XMLStreamConstants.CHARACTERS:
                    //get serial number and end the parsing
                    if(gotSerialNumber){
                        //get wsa and end the parsing
                        serialNumberValue = xmlStreamReader.getText();
                        gotSerialNumber = false;                            
                    }

                    break;
                case XMLStreamConstants.END_ELEMENT:
                    //when we get </Header> end the parse
                    //when we get </SerialNum> reinit flags
                    //when we get </Header> end the parse even we don't get a serial number
                    if("Header".equals(xmlStreamReader.getLocalName())){
                        parseComplete= true;
                    }else if("SerialNum".equals(xmlStreamReader.getLocalName())){
                        //reinit flag when we get </SerialNum> tag
                        gotSerialNumber = false;
                    }
                    break;
                default:
                    break;
                }
                event = xmlStreamReader.next();
            }


        } catch (final XMLStreamException e) {
            //catch block
            LOG.info("Got an XMLStreamException exception. " + e.getMessage());
        }
        catch (final IOException e1) {
            //catch block
            LOG.info("Got an IOException exception. " + e1.getMessage());
        }

    }


    return serialNumberValue;
}

This code extract the needed value but the code quality is not very good: it is not easy to read and maintain. It contains a switch case and if else blocks nested in a while loop. Which design pattern to use to enhance the code quality?

Why don't you post your code to [Code Review](http://codereview.stackexchange.com/)? — Andrea Dusza, Dec 27 '15 at 14:44
The state pattern comes to mind. But I have the feeling your code isn't correct: it sets parseComplete to true as soon as it reads the first serial number. And you said you wanted the last one. — JB Nizet, Dec 27 '15 at 14:44
@Andrea Dusza: sorry, I did not understand your suggestion. Can you please clarify your idea? — amekki, Dec 27 '15 at 14:46
@RealSkeptic I read `parseComplete= true;` at the end of the `case XMLStreamConstants.CHARACTERS:` branch. — JB Nizet, Dec 27 '15 at 14:50
@JBNizet: I agee with your remark, I will correct the posted code. However, is not clear for me how to use the state pattern in this case. If you have a code proposal, it would be very helpful for me. — amekki, Dec 27 '15 at 14:52
@JBNizet Right you are. There is also a problem that it assumes all the characters of the tag will be available in a single event. — RealSkeptic, Dec 27 '15 at 14:52
@RealSkeptic: sorry, I did not understand "There is also a problem that it assumes all the characters of the tag will be available in a single event" — amekki, Dec 27 '15 at 14:55
@amekki with SAX and DOM at least, but I assume it's also true with Stax, nothing guarantees that the parser will return all the characters inside an element at once. It could break inside, and return two or more sequences of characters in a row when asking for the next element. So you need to concatenate all the characters sequences until you read the tag end. Read http://stackoverflow.com/questions/13786607/normalization-in-dom-parsing-with-java-how-does-it-work/13787629#13787629 for an example. — JB Nizet, Dec 27 '15 at 14:58
Event-based XML parsers are based on reading a buffer of data, parsing it and converting it into a chain of events. It could happen that a text node, e.g. "ABCDEF", will be split between two or more buffer reads. So it can give you "ABC" in one event and "DEF" in the next event. — RealSkeptic, Dec 27 '15 at 14:58
Your program has different states, modeled by sets of booleans. The State pattern consists in modelling them by classes. Every time you get a next element, you pass it to the current state, which should return the next state. At then end, you're supposed to be at the final state, and the context must contain the result. — JB Nizet, Dec 27 '15 at 15:02
@JBNizet and RealSkeptic: I would like to thank your for this important issue about event. I will correct the code for the real project. — amekki, Dec 27 '15 at 15:04
@JBNizet: I will try your proposal. But I would like to know if the state pattern will replace the while loop nested with switch case. — amekki, Dec 27 '15 at 15:08
As a first step, you could try to extract the code of your different switch cases in separate methods and give them meaningful names — Louis F., Jan 05 '16 at 09:18

fabriciorissetto · Answer 1 · 2016-01-05T23:08:03.883

I don't think your code needs a design pattern. But a cleaner code would be really nice.

I agree with the Louis F. suggested in the comments: "As a first step, you could try to extract the code of your different switch cases in separate methods and give them meaningful names".

I think your code has too much comments too. This is a code smell. Just a example:

if("Header".equals(xmlStreamReader.getLocalName())){
    //tag is header, so begin parse
    enableToParse = true;
}

What about remove that comment and explain it with code?

if(isTagHeader(xmlStreamReader)) {
    enableToParse = true;
}

Just some thoughts... your code doesn't look terrible. But I think the main point here is not about design patterns.

If you're interested in go deeper about it I highly recommend the book "Clean Code" from Rober C. Martin (Uncle Bob).

I would like to thank you for your post. I will add methods for the different cases. It would be very interesting for me, if I can also use a design pattern. — amekki, Jan 05 '16 at 20:34

Which design pattern to use to improve this Java program

1 Answers1