[yast-devel] users controller refactoring
Trying to get the coverage running for all directories, I got stuck at the user's one was failing because the testcase is still Scr based, when the module is actually using the YaPI. Once I tried to make the test pass I realized the code was not structured in a way that makes it easy to test. Mainly, I found the following flaws: - The controller did direct calls to YaPI I pointed at this pattern in a previous email. If you want to test this, you will need to stub the YaPI, which is a lot of noise for a controller - The controller called instance modifier methods This is a very confusing way of writing code. Basically, you are in a method, lets say index. And from it you call another method, lets say fill_users, which modifies an instance variable, which is later used in the code. def index # this method modifies/creates @var fill_vars # and from now, the code relies on what fill_vars did @var.do_something It is very hard to figure out that fill_users does something with @users if you don't look at the implementation. As fill_users does not require anything from the controller, why does it need instance variables? either move the code to a module, or where it belongs, In this case, it makes sense to actually put it into the class where it belongs: User The only reason why in rails you have @variables is because the view accesses them. But 1) if it is not passed to the view, there is no reason for member variables 2) member variables is not a way to pass data across functions, it is the state of the object! Refactorings done: - all the instance modifier methods where removed. Similar methods are now in the User class: users = User.find_all user = User.find(id) user = User.create(id) user.save user.destroy The API imitates the ActiveRecord/ActiveResource one. Why not, that is what people is used to see in controllers. This also allows to separate testing in 2 parts: - the User class test, for which mocha stubs YaPI calls - the controller class, for which mocha stubs User class class This makes the controller test much simpler, you need to stub User class to return arrays of User, and not deal with stubing the complicated YaPI calls, why should the controller know about those anyway. It is the User class the class that is hiding that complexity. I haven't enabled the User class tests because I get this when calling YaPI calls in my machine: TypeError: can't convert Array into String /usr/lib64/ruby/vendor_ruby/1.8/dbus/type.rb:108:in `+' /usr/lib64/ruby/vendor_ruby/1.8/dbus/type.rb:108:in `to_s' /usr/lib64/ruby/vendor_ruby/1.8/dbus/marshall.rb:352:in `append' However I wanted to post the patch for review (and I guess I still need to see if it works, once the tests are done) Another pattern is where to use static methods or not. Consider the destroy method. If you implement it in the class User def destroy # do something with yapi and @id end You can define it as a class method: def self.destroy(id) # do something with yapi and id end And then define the previous one just as: def destroy self.class.destroy(@id) end Then you can use either as User.destroy("schubi") or as user.destroy (assuming user is an User object) -- Duncan Mac-Vicar P. - Engineering Manager, YaST SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nuernberg)
Another refactoring done has to do with 2 things: 1) use the stdlib or look around (active_support has quite a lot of stuff already) 2) Don't repeat yourself # FIXME convert ruby's under_scored keys to YaST's camel-cased ones data["cn"] = [ "s", self.cn ] unless self.cn.blank? data["userPassword"] = [ "s", self.user_password ] unless self.user_password.blank? data["homeDirectory"] = [ "s", self.home_directory ] unless self.home_directory.blank? data["loginShell"] = [ "s", self.login_shell ] unless self.login_shell.blank? data["uidNumber"] = [ "s", self.uid_number ] unless self.uid_number.blank? data["groupname"] = [ "s", self.groupname ] unless self.groupname.blank? Googling, you can find that activesupport already includes this functionality: http://refactormycode.com/codes/502-camelize "hello_world".camelize(:lower) => helloWorld Also consider using ActiveSupport attr_accessor_with_default ;-) Then, if you see code like the assignment above, you can save lot of typing by extracting the code in methods. For example you can create an array with cn, userPassword, homeDirectory, etc and iterate it, and then assign to data on each iteration. Using camelize to adapt the name on the way. Then even better, you can extract that to a method that taking an object (self in this case) creates the data structure, (in the refactoring, this was retrieve_data ) Duncan -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org For additional commands, e-mail: yast-devel+help@opensuse.org
participants (1)
-
Duncan Mac-Vicar Prett