HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Creating PDF files from a ODT template

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
templatecreatingodtfilesfrompdf

Problem

The code is actually a TemplateBuilder.

We have 2 different templates, French and Dutch.

We need to change the template and then convert it to PDF so it can be stored in the DB later.

The main method is createBulletins().

What am I looking for?

  • I don't like the constantly reading the template from the byte[] for each bulletin, so if there is a possibility this could be better (with less memory consumption) I'm fully open.



  • Any other suggestions/improvements.



Framework used :


    fr.opensagres.xdocreport
    xdocreport
    1.0.5

    fr.opensagres.xdocreport
    fr.opensagres.xdocreport.converter.odt.odfdom
    1.0.5

    fr.opensagres.xdocreport
    org.odftoolkit.odfdom.converter.core
    1.0.5


Code (Java 1.6) :

```
public class TemplateBuilder {

private static final Logger LOGGER = LoggerFactory.getLogger(TemplateBuilder.class);
private static final int NATIONAL_NUMBER = 0;
private static final int LANGUAGE = 2;
private static final int NAME = 3;
private static final int FIRST_NAME = 4;
private static final int STREET = 5;
private static final int ZIP = 6;
private static final int CITY = 7;
private static final int COUNTRY = 8;
private static final int BIRTH_DATE = 9;
private static final int NUMBER = 10;
private static final int EMPLOYD_BY = 12;
private static final int TYPE = 13;

private byte[] templateFR;
private byte[] templateNL;

private RarExtractor extractor;
private Report report;

/**
* The bulletin service.
*/
@Inject
@Named("bulletinWebService")
private BulletinWebService bulletinService;

/**
* The country dao.
*/
@Autowired
private CountryDAO countryDAO;

/**
* The user helper.
*/
@Autowired
private BulletinUserHelper userHelper;

private final String today = new SimpleDateFormat("yyyyMMdd").format(new Date());

/**
* Creates all the bulletins from the given templates, with

Solution

Logging multiple languages

While you do make usage of logging, you write them in sometimes in Dutch and sometimes in English.

Please try to hold to 1 language, preferable English for the logging.

Static or not?

private final String today = new SimpleDateFormat("yyyyMMdd").format(new Date());


It's clear that this String need's to hold todays day formatted.

The thing what you could improve is making the SimpleDateFormat static.

Like this you don't have an instantiation each time you create an instantiation of the TemplateBuilder

private static final SimpleDateFormat DATE_FORMATTER = new SimpleDateFormat("yyyyMMdd");
private final String today = DATE_FORMATTER.format(new Date());


Known memory leak

Your code has a memory leak, not by you but by Apache.

There is a tracker for it but it isn't resolved because Apache would not see it as a leak.

I came upon this because I'm the OP and the server died after approximate 250 pdf's done.

I rewrote the replace function to a simpler search, this means no looking in header and footer, and just returns the first Node what match the text to search :

private void replace(String searchField, String data, OdfTextDocument document) throws Exception {
    Node node = findNode(searchField, document.getContentRoot().getChildNodes());
    if (node != null) {
        node.setTextContent(data);
        LOGGER.info("found node : " + searchField);
    } else {
        LOGGER.warn("didn't found node : " + searchField);
    }
}

private Node findNode(String searchField, NodeList nodeList) {
    int length = nodeList.getLength();
    for (int counter = 0; counter < length; counter++) {
        Node node = nodeList.item(counter);
        if (node.getNodeType() == Node.TEXT_NODE && searchField.equals(node.getTextContent())) {
            return node;
        } else {
            Node child = findNode(searchField, node.getChildNodes());
            if (child != null) {
                return child;
            }
        }
    }
    return null;
}


Memory building up

The method public Map createBulletins() design isn't good

Why do you need to hold all those and return it later for saving?

What if your getCsv().getEntrySet() holds more then 30k entries?

Note : This is real the real situation

It's stacking up memory waiting to go out of memory.

Change the method to void or int if you want to return the size for a report and save every x entries. (x could also be 1).
You have the BulletinWebService in the class who provides the save so use it.

Code Snippets

private final String today = new SimpleDateFormat("yyyyMMdd").format(new Date());
private static final SimpleDateFormat DATE_FORMATTER = new SimpleDateFormat("yyyyMMdd");
private final String today = DATE_FORMATTER.format(new Date());
private void replace(String searchField, String data, OdfTextDocument document) throws Exception {
    Node node = findNode(searchField, document.getContentRoot().getChildNodes());
    if (node != null) {
        node.setTextContent(data);
        LOGGER.info("found node : " + searchField);
    } else {
        LOGGER.warn("didn't found node : " + searchField);
    }
}

private Node findNode(String searchField, NodeList nodeList) {
    int length = nodeList.getLength();
    for (int counter = 0; counter < length; counter++) {
        Node node = nodeList.item(counter);
        if (node.getNodeType() == Node.TEXT_NODE && searchField.equals(node.getTextContent())) {
            return node;
        } else {
            Node child = findNode(searchField, node.getChildNodes());
            if (child != null) {
                return child;
            }
        }
    }
    return null;
}

Context

StackExchange Code Review Q#115878, answer score: 2

Revisions (0)

No revisions yet.