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: [email protected]
For additional commands, e-mail: [email protected]
