This is the (hopefully) last part of my list of thing I considered as
the obvious way to go while refactoring/reviewing code at some point but
turned out to be more controversial than I expected.
This week we'll probably agree in a more shared and less personal view
about these topics.
1) RSpec as a design validator
I like the RSpec approach to unit testing and I consider the RSpec tests
of a class like the detailed documentation about its behavior. I try to
write the RSpec tests relatively early in the process (without reaching
the extreme of TDD) because it allows me to validate my design
decisions. If I struggle to write meaningful readable RSpec test or I
need a lot of non-obvious mocking during the process, then I conclude
that my classes are not properly designed. In that case, I sometimes
switch to TDD mode, writing the desired RSpec and then fixing my class.
2) Another note about OOP design
In our new code there are some things that I consider kind of "design
smells". I can be, as always, wrong but I feel like some of our code is
misusing OOP as a mere way of encapsulate functionality. Going back to
concrete real examples, we had this:
fake_probing = Yast::Storage::FakeProbing.new
devicegraph = fake_probing.devicegraph
factory = Yast::Storage::FakeDeviceFactory.new(devicegraph)
factory.load_yaml_file(input_file)
yaml_writer = Yast::Storage::YamlWriter.new
yaml_writer.write(devicegraph, output_file)
The fact that we had a "new" every couple of lines is a design smell to
me. The adopted solution was creating class methods, so we can do things
like this.
fake_probing = Yast::Storage::FakeProbing.new
devicegraph = fake_probing.devicegraph
Yast::Storage::FakeDeviceFactory.load_yaml_file(devicegraph, input_file)
Yast::Storage::YamlWriter.write(devicegraph, output_file)
Just a cosmetic fix for an issue that I consider deeper: we are writing
good old procedural code dressed up as OO code. Every class has the only
responsibility of implementing one step in a process and only one public
method to execute that step (manipulating a set of I/O arguments).
Is just one example of solutions that are perfectly correct but look
quite unfamiliar to a Ruby developer's eyes and are quite hard to
express as a set of RSpec behavior-oriented specs. Not saying they are
wrong, just that they don't look like "the ruby way". Like this
alternative which (although not perfect) is a much more "rubyist" way of
writing the same:
factory = Yast::Storage::DevicegraphFactory.new
devicegraph = factory.create_from_file(input_file)
File.open(output_file, 'w') {|f| f.write devicegraph.to_yaml }
3) Write every piece of code as something to maintain
This is more related to Scrum than to Ruby or OOP. Since we are using
Scrum to create YaST, I expect most PBIs to result in a YaST client that
can be invoked using /sbin/yast2 or in a set of RSpecs that will be
executed in every "rake test:unit" and used as a reference documentation
in the future.
I consider tests in the "examples" directory to be dead code. I consider
that moving code from /test or /src/clients into /src/examples is the
first step to have it broken with nobody ever noticing.
A have a similar feeling about code used for debugging. It should be "in
the right place" design-wise. Don't think in the debugging code as
something temporary. If you are needing it today, you will probably need
it (or something very similar) also in the future. Make it part of your
design... and use mechanisms that would be useful when that code is
executed as an RSpec test or yast client.
That's all. Sorry again for the long text. I suck at summarizing.
Cheers.
--
Ancor González Sosa
YaST Team at SUSE Linux GmbH
--
To unsubscribe, e-mail: yast-devel+unsubscribe(a)opensuse.org
To contact the owner, e-mail: yast-devel+owner(a)opensuse.org