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?

Answer 1

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).