[yast-devel] Summary of code review
Hi, during today meeting with Karel (external rails consultant) we do some public code review of my code and we also find some general problems. I try summarize it in this mail. Please ask if you don't understand something. I start with rest-service: We should have some general method in ancestor (e.g. YastApplicationController) of all webyast plugins which do something (so no permissions or resources) some general method permission checker with same behavior (like response permission denied). We in general should eliminate things which is repeating in each plugin. Have one line view is useless and better is use respond_to. webclient: My class LogException is useles if we disable backtrace silencing so logger.info e is enought (btw it looks like our webclient is not generated by rails 2.3, as few files missing, it looks like we generated it by 2.2) http://afreshcup.com/2008/11/29/rails-23-backtrace-silencing/ load_proxy method is good idea how to reduce needed lines of code but should not redirect or set any infos. Instead use throwing exceptions and use rescue_from to display correct page. This also discover why sometime our universal exception handler doesn't work, because some exception is not child of Exception but Error. This also discover how looks our Resource service and it is really unreadable and unmaintainable code, as it is really big, should use e.g. mixins or something else to reduce its size and complexity. Test for this class looks good, so we should try refactor it to better result. We should better structure whole architecture like some part of controller do things in one area which is not close to controller contract, so we should move it to mixin (especially use it in application controller). Karel doesn't like checking for gettext in application controller. some positives from code review - time module is good documented and testing looks good even if designer could recognize it is written after code. Ideas to reduce code size and duplicated code is good, only implementation is not always good. some general knowledge: - testing model is the easiest so move the most of functionality there for easier testing - over-mocking is not good, as it abstract from reality (it is useful when you don't have implemented some functionality) - .railsrc and .irbrc for some general routines available during console session I hope I don't forget much thinks, maybe someone add another knowledge. -- Josef Reidinger YaST team maintainer of perl-Bootloader, YaST2-Repair, webyast modules language and time -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org For additional commands, e-mail: yast-devel+help@opensuse.org
* Josef Reidinger <jreidinger@suse.cz> [Aug 18. 2009 15:52]:
Hi, during today meeting with Karel (external rails consultant) we do some public code review of my code and we also find some general problems. I try summarize it in this mail. Please ask if you don't understand something.
Thanks for the review and your exhaustive mail !
I start with rest-service: We should have some general method in ancestor (e.g. YastApplicationController) of all webyast plugins which do something (so no permissions or resources) some general method permission checker with same behavior (like response permission denied).
The "More details to permission problems (high)" tasks targets in this direction. I guess this needs someone taking care for 'infrastructure' in general. Any volunteers ?
We in general should eliminate things which is repeating in each plugin.
:-) That's DRY (don't repeat yourself) in 'Ruby speak'.
Have one line view is useless and better is use respond_to.
Hmm, I moved a couple of respond_to's to views in the past to have controller logic and resource representation more separated. Any reasons why this is considered 'bad' ?
webclient: My class LogException is useles if we disable backtrace silencing so logger.info e is enought (btw it looks like our webclient is not generated by rails 2.3, as few files missing, it looks like we generated it by 2.2) http://afreshcup.com/2008/11/29/rails-23-backtrace-silencing/
Ah, interesting. How can we 'refresh' existing code to 2.3 ?
load_proxy method is good idea how to reduce needed lines of code but should not redirect or set any infos. Instead use throwing exceptions and use rescue_from to display correct page. This also discover why sometime our universal exception handler doesn't work, because some exception is not child of Exception but Error.
This sounds as we need more code checking tests. Any recommendations ?
This also discover how looks our Resource service and it is really unreadable and unmaintainable code, as it is really big, should use e.g. mixins or something else to reduce its size and complexity. Test for this class looks good, so we should try refactor it to better result.
What is meant by 'Resource service' ?
We should better structure whole architecture like some part of controller do things in one area which is not close to controller contract, so we should move it to mixin (especially use it in application controller).
I guess this is an ongoing refactoring effort we all have to spend time on. Any ideas how to mark such code ?
Karel doesn't like checking for gettext in application controller.
Why ? What does he suggest as an alternative ?
some positives from code review - time module is good documented and testing looks good even if designer could recognize it is written after code. Ideas to reduce code size and duplicated code is good, only implementation is not always good.
some general knowledge: - testing model is the easiest so move the most of functionality there for easier testing - over-mocking is not good, as it abstract from reality (it is useful when you don't have implemented some functionality) - .railsrc and .irbrc for some general routines available during console session
I hope I don't forget much thinks, maybe someone add another knowledge.
It would be very helpful to move the general recommendations to a Wiki page and/or add stuff to the ToDo list. Klaus --- SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nürnberg) -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org For additional commands, e-mail: yast-devel+help@opensuse.org
On 08/24/2009 12:09 PM, Klaus Kaempf wrote:
* Josef Reidinger <jreidinger@suse.cz> [Aug 18. 2009 15:52]:
Hi, during today meeting with Karel (external rails consultant) we do some public code review of my code and we also find some general problems. I try summarize it in this mail. Please ask if you don't understand something.
Thanks for the review and your exhaustive mail !
I start with rest-service: We should have some general method in ancestor (e.g. YastApplicationController) of all webyast plugins which do something (so no permissions or resources) some general method permission checker with same behavior (like response permission denied).
The "More details to permission problems (high)" tasks targets in this direction.
I guess this needs someone taking care for 'infrastructure' in general. Any volunteers ?
I should take it. Maybe it should be enough to have permission check that throws exception instead of returning boolean. So then we should have same exception handler for this specific exception which show same result for all controllers. So it should be possible to write def show check_perm org.opensuse.yast.modules.yapi.time.read ... end or to be DRY def show check_yapi_perm time.read ... end
We in general should eliminate things which is repeating in each plugin.
:-) That's DRY (don't repeat yourself) in 'Ruby speak'.
Have one line view is useless and better is use respond_to.
Hmm, I moved a couple of respond_to's to views in the past to have controller logic and resource representation more separated.
Any reasons why this is considered 'bad' ? Because it decrease readability if we have one liner files, also if we use respond_to then we correct response to AJAX request (404).
webclient: My class LogException is useles if we disable backtrace silencing so logger.info e is enought (btw it looks like our webclient is not generated by rails 2.3, as few files missing, it looks like we generated it by 2.2) http://afreshcup.com/2008/11/29/rails-23-backtrace-silencing/
Ah, interesting. How can we 'refresh' existing code to 2.3 ?
I only find this http://cakebaker.42dh.com/2009/05/07/migration-from-rails-22-to-23/ I ask tomorrow if there is some automatic way.
load_proxy method is good idea how to reduce needed lines of code but should not redirect or set any infos. Instead use throwing exceptions and use rescue_from to display correct page. This also discover why sometime our universal exception handler doesn't work, because some exception is not child of Exception but Error.
This sounds as we need more code checking tests. Any recommendations ?
What I recommend is use some set of standard exceptions which we then unify handle and use some general tests that checks corner case if controller act as it should. (or at least document what should controller do in this situation like if user doesn't have permission to module then throw NoException etc)
This also discover how looks our Resource service and it is really unreadable and unmaintainable code, as it is really big, should use e.g. mixins or something else to reduce its size and complexity. Test for this class looks good, so we should try refactor it to better result.
What is meant by 'Resource service' ?
webclient/libs/yast/service_resource
We should better structure whole architecture like some part of controller do things in one area which is not close to controller contract, so we should move it to mixin (especially use it in application controller).
I guess this is an ongoing refactoring effort we all have to spend time on.
Any ideas how to mark such code ?
I think that common code ugly mark #XXX is enought as many editors highlight it and it should be easily greped.
Karel doesn't like checking for gettext in application controller.
Why ? What does he suggest as an alternative ?
We should just depends on it on package way same as another require which we need, not? (so add gettext to spec file) I cannot remember what he suggest maybe something like plugin that provides gettext functionality and checks it in init.rb? Doesn't anyone remember it?
some positives from code review - time module is good documented and testing looks good even if designer could recognize it is written after code. Ideas to reduce code size and duplicated code is good, only implementation is not always good.
some general knowledge: - testing model is the easiest so move the most of functionality there for easier testing - over-mocking is not good, as it abstract from reality (it is useful when you don't have implemented some functionality) - .railsrc and .irbrc for some general routines available during console session
I hope I don't forget much thinks, maybe someone add another knowledge.
It would be very helpful to move the general recommendations to a Wiki page and/or add stuff to the ToDo list.
OK, I try write notes from sessions and after discussion write it to wiki. -- Josef Reidinger YaST team maintainer of perl-Bootloader, YaST2-Repair, webyast modules language and time -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org For additional commands, e-mail: yast-devel+help@opensuse.org
participants (2)
-
Josef Reidinger -
Klaus Kaempf