On 29.02.2016 08:07, Ancor Gonzalez Sosa wrote:
[...]
Oh, my, where to begin, where to end...
Let me start by saying that there are different styles of working, different
approaches to do things, even considering that it all revolves around an
object oriented approach.
We don't need to discuss that we all want code that not only works (this is
the most basic requirement, of course), but that is also easy to use, easy to
maintain, adequately documented and testable. I think we all agree on those
points.
There seem to be different schools of thought, though, how to achieve that.
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.
RSpec is one approach to testing, and it makes a claim - a very bold claim -
that it also serves as a substitute for a formal specification.
While I agree that it is a test tool that does its job reasonably well, I
dispute the claim that it can serve as documentation. It just doesn't. Try
reading somebody else's RSpec code without having read documentation of the
class that it tests, without having read an example. You will not figure out
what the hell that class does. You may find out what some fringe cases are
all about, but there is just no way to get a reasonable overview what the
class is all about. You get swamped by a flood of details before you get to
see the overall picture.
So, IMHO RSpec fails miserably on its second claim - acting as specification.
The trouble is that it forces the developers to be a lot more verbose during
writing the tests than should be necessary; the rationale for that is the
"it's also a spec" part which IMHO just does not materialize.
But maybe that's just me; I'd be interested in other opinions about that.
Please notice that I am all in favour in writing and having tests. This is a
must-have to ensure covering all major use cases and a lot of fringe cases,
and it's a prerequisite for avoiding regressions in the future. It's costly,
but it will (hopefully) be a worthwhile investment.
But there is a caveat: However hard we try, there will never ever be 100%
test coverage. There will always be untested scenarios. This is not to argue
against unit testing - to the contrary; it's just to avoid believing that
this will solve or avoid all possible problems.
For example, take that DiskSize class from libstorage-ng. It's a pretty small
class that can handle disk sizes in MiB, GiB, TiB etc., and I added an RSpec
test suite prette early while writing it. I thought it was pretty complete
and had pretty good coverage.
"Pretty" is the commanding word here. "Pretty" means "not 100%", "not
perfect". And not 100% perfect it was: Only when using that class extensively
I found that there were still some aspects missing, despite all that testing.
For example, I got output formatted as "1024.0 GiB" when I expected "1.0 TiB"
- the classic off-by one error when going up to the next-higher unit -
comparing for < 1024 when it should have been <= 1024.
Now, that was easy to fix, but it demonstrates what I wrote above: However
complete you think your test suite is, it will always miss some important
test cases. You learn that over time while using a class. A test suite will
always be work in progress; it will never be complete as long as the code it
tests is in use.
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).
I agree (and as a matter of fact, I even wrote that in that new functions'
description) that it's just cosmetic to use a class method (or, in Ruby
lingo, "singleton" method) to avoid the one-trick-pony use.
I do not agree that this in any way violates the object-oriented approach -
to the contrary.
(more about this below)
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 }
Again, this is an approach I don't like at all because it focuses on the
wrong thing: All that stuff is not about creating device graphs. We are not
in the business of making or selling device graphs. Device graphs are just a
tool for what we really intend to do. Concentrating on an irrelevant (or, to
put it less harshly, behind-the-scenes) tool is most misleading for anybody
looking at that code.
Worse, putting the burden of file handling to the caller IMHO is not
desirable; why would I as a caller want to bother with that file? I want a
function that does that for me, including error handling and whatnot.
Worse still, you need to obey a call protocol when you handle it like that;
you introduce boilerplate code that everybody will have to copy verbatim from
some other place where it works.
Back in the early 1990s when I started programming X11 and OSF/Motif, we had
to write tons of boilerplate code. Change any little thing, and you are
doomed; it fails in some crazy way, and you'll never know why. And you
couldn't even comment out stuff to find out when it starts to fail; every
little bit of that boilerplate was needed to make it work.
Been there. Done that. Got the f*ing T-shirt. As a matter of fact, got a
whole dressing cabinet full of those T-shirts. Want any? ;-) ("I was sent to
software never-never land, and all I got is this lousy T-shirt").
Back to the point I promised earlyer: Object oriented programming.
What constitutes an object, a class?
- Some data (however minor) it keeps around to do its thing
- Some service it provides based on those data
- Encapsulating internal stuff to ensure outside independency of
implementation details
- Optionally, being able to inherit from that class and to reimplement
(overwrite) certain aspects of how it works.
And now we have those "one-trick-ponies", those classes that perform only one
job, the classes that can in the most common use case be hidden in a single
function. Is that a contradiction to the above?
Well, IMHO no. For one thing, it's not really only that one thing it does;
that one thing is just the most common use case. But if such a class is
written well, it also gives you access to other methods that perform just one
aspect of its main task, or that lets you perform that main task in different
ways, or that lets you specify additional parameters to that main task or to
any of the sub-tasks.
Encapsulation is the main thing. IMHO it's the fine art of OOP to do most
things with useful parameters without having to write tons of that
boilerplate code mentioned above. Only when you want to do the nonstandard
use case you should have to specify parameters, or at least most of them
(there is not always a good default; filenames are a prime example).
Take the YaST UI, for example. By default, you don't have to specify
anything; just get the widget factory from the UI, and then you can merrily
begin creating widgets. No boilerplate code needed at all. But if you want a
special setup, use different init calls and then get the widget factory and
create widgets. To the outside, this looks like procedural code, but it's
really not. It carries its state inside, and it uses reasonable defaults for
the most common use cases.
And this is similar to the classes in yast-storage-ng. It might not always
show, but this is the philosophy behind it. The defaults should always be
reasonable, but still it leaves you room to tweak a lot of things. And, more
importantly, it hides the gory details from the outside. In many cases,
however, those gory details are accessible enough for the caller or for
derived classes.
For example, we had this discussion on IRC last week that Josef wanted
minimalistic YAML files for those device trees and auto-generate partitions
and file systems because his use case just doesn't care about most of that.
While this would not be desirable for the normal case IMHO, I pointed out to
him that it's trivial to add that functionality by inheriting from the
standard FakeDeviceFactory (which can read YAML files) and reimplement one or
two methods to add that functionality. This is a lot harder with purely
procedural code.
A lot of things are happening behind the scenes in those classes, and you
shouldn't need to know about that when you just want to use them. But you can
if you want, and there is support for nontrivial use cases. But the main use
case is and should be simple. This is an important design goal IMHO.
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.
By all means, yes.
I consider tests in the "examples" directory to be dead code.
I pointed that out to you before: Those examples are not tests. They are
examples. They serve as documentation to show how a class is used, and they
are an important development tool to work on that stuff, to have easy access
to those classes without going through the hassle of starting an installation
(possibly with creating a package and a driver update disk with that package
and integrating that DUD into an installation ISO and starting an
installation in a virtual machine).
I wrote those examples mostly for myself as development tools. If you take
them away, all you will achieve is that I will simply no longer share them
and keep them to myself. But since we are a team, I figured that I'd like to
share all development tools to make life easier.
Life as a YaST developer is hard enough with all those fragile and
over-complex tools we have to use on an everyday basis. We can very well use
some simple tools to make life easier.
I know I repeat myself, but those examples are not intended as a substitute
for unit tests. We need unit tests on all levels we can get. But we also need
simple examples to have a realistic chance to work with our own 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.
Anything in src/clients that is not simply invokable by itself is dead code
indeed. If I need yet another external binary to invoke something written in
a scripting language, something is very wrong. Maybe we should reconsider
that approach and try to get rid of that additional binary.
A have a similar feeling about code used for debugging.
As long as a piece of code is not 100% perfect (which will never happen),
there will be the need for debugging code. And since debugging code tends to
solve a temporary problem that is only valid for a very limited time, this is
very much a moving target.
If...
> 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...
...this is the claim, I'd no longer call it debugging code. Then it becomes
part of the interface, and it needs the same amount of testing amd
maintenance, too.
But look at it from another side: If we take the "agile" approach seriously,
things like debugging code are not graven in stone. It will change as
development progresses. What is useful debugging code today, it might be
leftover cruft tomorrow because tomorrow we'll have a lot better tools to
replace it.
For example, that "dump_disks" function we talked about lately was just a
crude approach to get any useful output, and yes, it was pretty much
misplaced where it was at first, but it was there, it was available, and it
served its purpose - until the next development step. We not can use much
more useful YAML output instead because we have that nice new class that does
that for us.
This is what being agile is all about: Do things, improve them, iterate until
the stakeholders are satisfied (which always means "good enough", never
"perfect").
and use mechanisms that would be useful when that code is
executed as an RSpec test or yast client.
q.e.d.
CU
--
Stefan Hundhammer <[email protected]>
YaST Developer
SUSE Linux GmbH
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)
Maxfeldstr. 5, 90409 Nürnberg, Germany
--
To unsubscribe, e-mail: [email protected]
To contact the owner, e-mail: [email protected]
