Is my program logic correct?

answered 2017-02-13 13:26:32 +0800

chillworld
5367 ● 4 ● 9
https://github.com/chillw...

updated 2017-02-14 09:18:44 +0800

About the logger, it's no problem to do that and if the name "Log" is good for every VM, yeah a static final would be the best approach.

Just take attention, a static final isn't for a Session but at application level.
So be carefull with naming the abstract VM.

About the list with entities....

This is for me not the right approach.
It looks like you want a kind of caching, so no extra queries are done to the DB.
caching happens at server side, not in your viewmodel, and there are some good strategies for it or you can make your own.
the only think you need to remember is that GUI is only graphical user interface and program logic isn't needed there except for visibility or disabling some buttons.
Caching stands under program logic (because you want a logic for speeding up the queries) and if you switch the GUI, you still want that, or not...?

hope this could help you.

Edit:

Of course, getting the value from cache will be faster then query 3 times to the DB.
But be aware, a combobox need's a ListModel, if you provide a normal collection to the model, there will be autocasting in the back.
But, if you use a ListModel, never but never make that static, as selectedItems is hold in the ListModel also and you will have strange issues when you do.
Now, why I say that caching is needed in the server, think whenever you create a new item, or edit an already existing object, you can update your cache there with no problems.

your code should like this :

VM :

 private ListModelList<Car> carsModel = new ListModelList<>(carService.findAll());

CarService :

public Collection<Car> findAll() {
    return Collections.unmodifiableList (cache);
}

So, you see that I return an unmodifiable collection (in this case List).
It's just for not giving the UI code the opportunity to alter your collection, as this would be just dangerous.

A simple cache service could be :

public Collection<Car> findAll() {
    if (cache == null) {
        cache = carRepo.findAll();
    }
    return Collections.unmodifiableList (cache);
}

public Car save (Car car) {
    if (findAll().contains(car)) { // to be sure that cache is filled we use the findAll()
        cache.remove(car);
    }
    Car saved = carRepo.save(car);
    cache.add(saved);
    return saved;
}

With this you keep your cache always correct, except if other applications query your DB or manual interventions on the DB. (that's why I call it simple cache)
That's also the reason why you should do it in the server, just to intercept your save or delete or anything what could alter your cache.
Maybe this is not yet the case, but always be prepared for when the time is there.
A GUI doesn't need to know if your collection is cache or it comes from a DB or even a file, it only need's the collection from your server.

Chill.