Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

i am trying to implement factory pattern for getting XML Document from server

(using javax.xml.parsers.DocumentBuilder)

I have the classes below for now, could you give your opinion ? Does the structure make sense for this pattern?

DocumentGeneratorFactory (abstract factory)

public interface DocumentGeneratorFactory {

    public Document createDocument(String scheme, String authority,
            String path, HashMap<String, String> parameters)
            throws ParserConfigurationException, SAXException, IOException;

    public Document createDocument(String scheme, String authority,String path)
            throws ParserConfigurationException, SAXException, IOException;
}

ProductDocumentGeneratorFactory (Concreate factory)

public class ProductDocumentGeneratorFactory implements
        DocumentGeneratorFactory {

    public Document createDocument(String scheme, String authority,
            String path, HashMap<String, String> parameters)
            throws ParserConfigurationException, SAXException, IOException {

        Uri.Builder uri = new Uri.Builder();
        uri.scheme(scheme);
        uri.authority(authority);
        uri.path(path);

        Set<Map.Entry<String, String>> set = parameters.entrySet();

        for (Map.Entry<String, String> params : set) {
            uri.appendQueryParameter(params.getKey(), params.getValue());
        }

        URL url = new URL(uri.toString());

        DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
        DocumentBuilder db = dbf.newDocumentBuilder();
        Document doc = db.parse(new InputSource(url.openStream()));
        doc.getDocumentElement().normalize();

        return doc;
    }

    public Document createDocument(String scheme, String authority, String path)
            throws ParserConfigurationException, SAXException, IOException {

        Uri.Builder uri = new Uri.Builder();
        uri.scheme(scheme);
        uri.authority(authority);
        uri.path(path);

        URL url = new URL(uri.toString());

        DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
        DocumentBuilder db = dbf.newDocumentBuilder();
        Document doc = db.parse(new InputSource(url.openStream()));
        doc.getDocumentElement().normalize();

        return doc;

    }

}

Request (Abstract Product)

public abstract class Request {
    Document doc;
    HashMap<String, String> queryStrings;

    abstract void prepareRequest() throws ParserConfigurationException, SAXException, IOException;  

}

ProductRequest (Product)

public class ProductRequest extends Request{
    ProductDocumentGeneratorFactory DocumentGeneratorFactory;   
    HashMap<String, String> queryStrings;

    public ProductRequest(ProductDocumentGeneratorFactory DocumentGeneratorFactory,HashMap<String, String> queryStrings){
        this.DocumentGeneratorFactory = DocumentGeneratorFactory;
        this.queryStrings = queryStrings;
    }

    @Override
    void prepareRequest() throws ParserConfigurationException, SAXException, IOException {
        doc = this.DocumentGeneratorFactory.createDocument("http", "ip-address", "default.aspx",this.queryStrings);     
    }

}
share|improve this question
add comment

1 Answer

up vote 1 down vote accepted

Makes sense, but you should DRY up your concrete factory at least. Forgive me my rusty Java, but something like this:

public class ProductDocumentGeneratorFactory implements
        DocumentGeneratorFactory {

    public Document createDocument(String scheme, String authority, String path)
            throws ParserConfigurationException, SAXException, IOException {

        Uri.Builder uri = createUri(scheme, authority, path);
        Document doc = createDocument(uri);
        return doc;
    }

    public Document createDocument(String scheme, String authority,
            String path, HashMap<String, String> parameters)
            throws ParserConfigurationException, SAXException, IOException {

        Uri.Builder uri = createUri(scheme, authority, path);
        appendParameters(uri, parameters);
        Document doc = createDocument(Uri.Builder uri);
        return doc;
    }

    private Uri createUri(string scheme, string authority, string path)
    {
        Uri.Builder uri = new Uri.Builder();
        uri.scheme(scheme);
        uri.authority(authority);
        uri.path(path);
        return uri;
    }

    private Document createDocument(Uri.Builder uri)
    {
        URL url = new URL(uri.toString());

        DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
        DocumentBuilder db = dbf.newDocumentBuilder();
        Document doc = db.parse(new InputSource(url.openStream()));
        doc.getDocumentElement().normalize();

        return doc
    }

    private void appendParameters(Uri.Builder uri, HashMap<string,string> parameters)
    {
        Set<Map.Entry<String, String>> set = parameters.entrySet();
        for (Map.Entry<String, String> params : set) {
            uri.appendQueryParameter(params.getKey(), params.getValue());
        }
    }
}

Of course you can expand on that by employing the template pattern for the extra parameter settings.

I would also look into extracting the parameters to a "parameter object" instead of several parameters.

share|improve this answer
add comment

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.