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

A good management of Java code

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

Problem

I'm a beginner in JEE development. I have just finished a web application that I'm going to deploy in a server.

It's a new level for me; it's not just a simple application that I can run for 10 min, etc.

My question is for the quality of code. Of course we all know that a piece of code that works does not mean it's correct. In my managed beans, I use @postConstruct, for example:

@PostConstruct
public void post() {
    listChaletss = chaletService.getAllChalet();

    for (Chalet it : listChaletss) {

        if (it.getType().equals("permanent")) {

            listChaletp.add(it);
        }
    }
    allPeriodes = periodeService.getAllPeriodes();
    // System.out.println("size periode" + allPeriodes.size());
    for (Chalet order : listChaletss) {
        order.setEditable(false);
    }
}


I'm wondering if someone can determine if we can face any memory problems using this application. Some piece of code and any help from some Java experts will be welcomed.

Solution

Your variables listChaletss, listChaletp, and allPeriodes appear to be instance variables. We can't tell whether you're leaking memory without further context — what is the expected lifetime of this object? If this object only has a brief existence, then you probably don't have to worry about how it uses memory. On the other hand, if it's a long-lived object, you have to be careful.

There is cause for concern, though. You store the result of .getAllChalet() in instance variable listChaletss. Why an instance variable? Is listChaletss acting as some kind of cache? If so, then the code would likely be:

public void post() {
    if (null == listChaletss) {
        listChaletss = chaletService.getAllChalet();
    }
    ...
}


… or better yet:

private Chalet[] getChalets() {
    if (null == listChaletss) {
        listChaletss = chaletService.getAllChalet();
    }
    return listChaletss;
}

public void post() {
    for (Chalet it : getChalets()) {
        ...
}


… or better yet, use a WeakReference.

On the other hand, if listChaletss is not meant to be a cache, then you should just make it a local variable so that you don't hang on to the result. Unintentionally keeping a reference to the result leads to a "lingering" memory leak: you waste memory storing listChaletss, but you recover that memory the next time you call .post(), only to waste it again.

Similar considerations apply to your other two instance variables.

Code Snippets

public void post() {
    if (null == listChaletss) {
        listChaletss = chaletService.getAllChalet();
    }
    ...
}
private Chalet[] getChalets() {
    if (null == listChaletss) {
        listChaletss = chaletService.getAllChalet();
    }
    return listChaletss;
}

public void post() {
    for (Chalet it : getChalets()) {
        ...
}

Context

StackExchange Code Review Q#33265, answer score: 4

Revisions (0)

No revisions yet.