current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 3 down vote favorite

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.

share|improve this question
3  
you are iterating listChaletss two times. This could be expensive. When you have this type situation, try to manage your structure in one iteration. –  Kinjal Oct 26 '13 at 5:05
2  
Why are you worried about memory problems in this particular code sample (when i overlook the loop redundancy)? Did you run it through some analysis or does it sometimes causes problems/exceptions/etc? –  DominikM Oct 29 '13 at 9:49

2 Answers 2

up vote 4 down vote

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.

share|improve this answer
up vote 1 down vote

A possible memory concern that I can think of is this line:

listChaletp.add(it);

If you're never removing elements from that list (which is hard for me to know since I only see a portion of your code), then sooner or later that list will get out of control (have too many useless elements) which will eat up your memory.

Besides this, the only possible concerns is your periodeService.getAllPeriodes() and chaletService.getAllChalet() but if those do what it sounds like they do (simply return a list without any computation or modifications of those lists) then you don't need to worry about those methods.

share|improve this answer

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.