Thread: [GUI Plugin] Reading List
View Single Post
Old 01-03-2012, 11:19 PM   #330
davidfor
Grand Sorcerer
davidfor ought to be getting tired of karma fortunes by now.davidfor ought to be getting tired of karma fortunes by now.davidfor ought to be getting tired of karma fortunes by now.davidfor ought to be getting tired of karma fortunes by now.davidfor ought to be getting tired of karma fortunes by now.davidfor ought to be getting tired of karma fortunes by now.davidfor ought to be getting tired of karma fortunes by now.davidfor ought to be getting tired of karma fortunes by now.davidfor ought to be getting tired of karma fortunes by now.davidfor ought to be getting tired of karma fortunes by now.davidfor ought to be getting tired of karma fortunes by now.
 
Posts: 24,905
Karma: 47303824
Join Date: Jul 2011
Location: Sydney, Australia
Device: Kobo:Touch,Glo, AuraH2O, GloHD,AuraONE, ClaraHD, Libra H2O; tolinoepos
Quote:
Originally Posted by kiwidude View Post
@davidfor - see post #323/#324 above for the discussion on move to list and why I removed the view_list line. You obviously use the move to list functionality yourself (unlike me), and as I don't believe it is an API call that Jim needs to use I'm open to suggestions if you want the previous behavior restored.
I could have sworn I read through those posts last night but didn't see anything that I can see the problem, but I think the lists need to be refreshed when these actions are taken (add, remove and move) from the menu. One problem is that if the list isn't updated, the user can't be sure the action happened, and if the user tries again, they will get an error.


Quote:
I'll take a look at the refresh_screen stuff tonight. I tried to make the changes for Jim around midnight that night just to get something out for him, and clearly didn't give it my normal TLC.
OK, after reading all this code and discussing it a little here, I feel myself dropping into code review mode. Here are the suggestions I would be making if this was shown to me at work:

- Separate the database and GUI updates. This will help with exposing methods as part of the API as it means that no unexpected side-effects will happen. And, looking at it, the remove and add methods, probably shouldn't be interacting with the GUI in any way.

- For errors in the database/API methods, raise exceptions for the errors. These can be caught and handled by the calling code.

- Refactor the code that does the GUI updates. You can easily move this this to a separate method to be called from a lot of places. I would use the code from the bottom of "remove_books_from_list".

- Where do the GUI updates get done? In this, case, I would probably do them from the methods the menu call. These would check the input was OK, call the database updates, display any messages and update the GUI. But, I might also leave the input checking there and move the rest of it to another method. I would have to look at the code reuse scenarios properly to decide.

Those are my quick thoughts on this. On the other side of this, I do like your code. It is well laid out, easy to read, method and variable names are obvious and I can easily understand it.
davidfor is offline   Reply With Quote