Hi, I am interested how DRY is our code. So I use flay tool to analyze all our sources for webclient. You can find filtered (as some report is unrelevant or in generated part) report with my suggestion how to fix it at the end of email. My general impression is that our code doesn't contain much duplications which is good. Except some minor problems I see two major things which could be improved. The first one is sharing test_helper and add there some helpers which is quite often repeated in tests. Does anybody know how to share test_helper in easy way (so no hardcoded path from __FILE) ? The second one is that we have quite lot of data manipulation (like serializing to string, collecting things from arrays or filtering) in controllers. I think this manipulation should have descriptive methods in models and controller just call simple method. e.g. in status checking if something is changed and if so, then save it. I think it could be created method conditional_save, which inside check if something is changed and if so then call save, otherwise just return true. Another good example from status controller is method evaluate values, where many lines could be moved to model. Moving functionality to model has two main advantages. The first one is that if another developer want use some your functionality it use model, so everything which is not in model is just copied instead of sharing in model. The second one is that functionality in model is much easier to properly test then action in controller. some minor problems found in flay output: 4) Similar code found in :defn (mass = 306) ./plugins/mail/test/functional/mail_controller_test.rb:10 ./plugins/time/test/unit/systemtime_test.rb:7 ./plugins/time/test/unit/ntp_test.rb:7 ./plugins/software/test/unit/repositories_test.rb:8 ./plugins/software/test/functional/repositories_controller_test.rb:8 ./plugins/eulas/test/unit/eulas_test.rb:7 ./plugins/eulas/test/functional/eulas_controller_test.rb:9 ./plugins/roles/test/unit/role_test.rb:7 ./plugins/roles/test/functional/roles_controller_test.rb:8 ./plugins/users/test/unit/groups_test.rb:8 ./plugins/users/test/unit/users_test.rb:8 ./plugins/users/test/functional/groups_controller_test.rb:9 ./plugins/users/test/functional/users_controller_test.rb:9 ./plugins/firewall/test/unit/firewall_test.rb:7 ./plugins/network/test/unit/interface_test.rb:8 ./plugins/network/test/functional/network_controller_test.rb:8 ./plugins/status/test/functional/status_controller_test.rb:11 ./plugins/services/test/functional/services_controller_test.rb:10 same method to load xml fixture to mock rest-service. If we found solution for sharing test_helper then we should add this method to generic test_helper. 5) IDENTICAL code found in :resbody (mass*3 = 297) ./plugins/software/app/controllers/repositories_controller.rb:57 ./plugins/software/app/controllers/repositories_controller.rb:72 ./plugins/software/app/controllers/repositories_controller.rb:100 Handling ResourceNotFound exception and same output. Could be moved to one method which define action if this happen like rescue ActiveResource::NotFound report_not_found return end 7) IDENTICAL code found in :if (mass*3 = 216) ./plugins/software/app/controllers/repositories_controller.rb:42 ./plugins/software/app/controllers/repositories_controller.rb:61 ./plugins/software/app/controllers/repositories_controller.rb:88 same if before each action which expect id. I think it should be moved to method like check_params and then you can simple use check_params || return in these methods.. 8) IDENTICAL code found in :call (mass*3 = 216) ./plugins/time/test/functional/time_controller_test.rb:62 ./plugins/time/test/functional/time_controller_test.rb:78 ./plugins/time/test/functional/time_controller_test.rb:108 same checking of xml request in test. Should be refactored to method which found and return simple hash. 10) IDENTICAL code found in :iter (mass*3 = 198) ./plugins/users/app/controllers/users_controller.rb:59 ./plugins/users/app/controllers/users_controller.rb:104 ./plugins/users/app/controllers/users_controller.rb:146 same manipulation with groups string. Should be moved to separate method. 20) Similar code found in :defn (mass = 136) ./plugins/system/app/controllers/system_controller.rb:12 ./plugins/system/app/controllers/system_controller.rb:32 Reboot and restart actions looks very similar 28) Similar code found in :iter (mass = 114) ./webclient/test/dummy-host/host.rb:182 ./webclient/test/dummy-host/host.rb:224 ./webclient/test/dummy-host/host.rb:240 ./webclient/test/dummy-host/host.rb:249 ./webclient/test/dummy-host/host.rb:263 ./webclient/test/dummy-host/host.rb:389 mocking host in sinatra. Does we need this file. 30) IDENTICAL code found in :and (mass*2 = 104) ./plugins/status/app/controllers/status_controller.rb:139 ./plugins/status/app/controllers/status_controller.rb:224 same condition. I think that you can benefit from ClientException which has methods to detect type of error. 37) Similar code found in :iter (mass = 84) ./plugins/status/app/controllers/status_controller.rb:57 ./plugins/status/app/controllers/status_controller.rb:63 same block and almost same methods. I think it is possible to refactor it to more readable code. Especially move to separate method with proper name "magic" code which multiply and divide by numbers and then convert it) 50) Similar code found in :if (mass = 72) ./plugins/mail/app/controllers/mail_controller.rb:50 ./plugins/administrator/app/controllers/administrator_controller.rb:62 similar as above, I suggest to use ClientException which has method which helps to easier check type of backend exception. 51) IDENTICAL code found in :return (mass*2 = 72) ./plugins/registration/app/controllers/registration_controller.rb:91 ./plugins/registration/app/controllers/registration_controller.rb:99 Could be moved to some flash constructor for registration. It easier possible future improvements 53) IDENTICAL code found in :attrasgn (mass*2 = 68) ./plugins/users/app/controllers/groups_controller.rb:155 ./plugins/users/app/controllers/groups_controller.rb:198 same loop for set members of group. I think that you can add method to model, which takes string representation of groups and parse it inside. 54) Similar code found in :iter (mass = 68) ./plugins/users/app/controllers/users_controller.rb:32 ./plugins/users/app/controllers/users_controller.rb:67 ./plugins/samba_server/app/controllers/samba_server_controller.rb:33 ./plugins/samba_server/app/controllers/samba_server_controller.rb:74 These two plugins provide also xml output for webclient. I think it is confusing if only one of published module provide this behavior and I recommend remove it unless we decide that we need it for more plugins. 69) Similar code found in :block (mass = 46) ./plugins/users/app/controllers/users_controller.rb:167 ./plugins/users/app/controllers/users_controller.rb:210 same as above for generate xml. I think we should provide same behavior for all plugins. 73) Similar code found in :not (mass = 42) ./plugins/mail/app/controllers/mail_controller.rb:65 ./plugins/administrator/app/controllers/administrator_controller.rb:72 I think that test should be moved to administrator model to share functionality. 83) Similar code found in :if (mass = 34) ./webclient/lib/client_exception.rb:26 ./webclient/lib/client_exception.rb:27 same one line test. It could be rewrite to avoid unnecessary test. -- Josef Reidinger YaST team maintainer of perl-Bootloader, YaST2-Repair, parts of webyast -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org For additional commands, e-mail: yast-devel+help@opensuse.org