Mailinglist Archive: yast-devel (79 mails)

< Previous Next >
Re: [yast-devel] Refactoring madness: chapter three
On 03/01/2016 10:01 AM, Josef Reidinger wrote:
On Mon, 29 Feb 2016 17:31:46 +0100
Stefan Hundhammer <shundhammer@xxxxxxx> wrote:

On 29.02.2016 08:07, Ancor Gonzalez Sosa wrote:
[...]

[...]

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.

We seem to disagree in the value of RSpec as examples substitute, but I
think there is no way somebody can disagree in your reasoning about test
coverage - you will always miss some test cases, even when the coverage
tool claims a 100% coverage. It's crucial that you update your testsuite
when you find a hole on it. So every bugfix should include the
corresponding fix/addition to the testsuite.

I once read that ideal test coverage is 90%...if you have 99% you are
too confident that it catch everything and if it is below, then it is
too weak. I think it is reasonable rule as from my experience there is
always some parts, where automatic tests are too complex and too
fragile, that it is easier to test manually then always fixing
automatic ones and such 90% advice is good start.


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)


[...]

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.

Ok. That's the goal, to discuss the approach. See below.

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.

I agree. I prefer otherway around. using IO streams which is more
flexible and allow e.g. also streming for example. So something like

device_graph = DeviceGraph.deserialize(input_file)
device_graph.serialize(output_file)

where you can have more flexible behavior like having input/output file
also streams beside strings.

I hate when we concentrate on a side track of the example instead of the
main point. ;-) The "not perfect" warning didn't help. I was not
advocating for extracting the File.open call out of the method. Or about
adding boilerplate code to handle files. My point was about the general
approach of this

# I find unnecessary and non obvious the need to fake hardware probing
# just to work with a devicegraph that is described in a YAML file
fake_probing = Yast::Storage::FakeProbing.new
# An additional call (with responsibility on fake_probing) just to
# create an empty devicegraph indexed in a global catalog
devicegraph = fake_probing.devicegraph
# devicegraph as an I/O argument
Yast::Storage::FakeDeviceFactry.load_yaml_file(devicegraph,input_file)
# I don't completely disagree, but I thing it's more natural in Ruby to
# do it like it's done below
Yast::Storage::YamlWriter.write(devicegraph, output_file)

vs this (modified version to focus on approach instead of file writing)

# I want to create a Devicegraph, so I instantiate a Devicegraph
# factory, not a FakeProbing + DeviceFactory (notice the difference
# between DevicegrahFactory and DeviceFactory)
factory = Yast::Storage::DevicegraphFactory.new
# It simply returns a meaningfull Devicegraph object.
# No I/O argument. No catalog. No hardware probing fake.
devicegraph = factory.create_from_file(input_file)
# More natural for Ruby developers. Very opinionated, I admit
devicegraph.serialize(output_file)

I don't see why the second version is more focused on devicegraphs as an
implementation detail than the first one. Can you elaborate on that?

As I see it, in the first one you create an empty devicegraph and then
populate it using a DeviceFactory. In the second one you create the
devicegraph directly using a DevicegraphFactory. I don't see that as a
problem or a more devicegraph-focused approach.

Actually, as a user of that API I would expect that "behind the scenes"
there will be also factories for each type of device, or at least a
generic DeviceFactory (like we indeed have). But I would expect that
DeviceFactory to return device objects, not to populate a given devicegraph.

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.

I agree that boilerplate should be reduced and ruby is quite good with
it, so there is only a bit of syntactic sugar needed.

Already said. My example was (not obviously enough) not about adding the
boilerplate File.open, but about the general design.

[...]

Cheers.
--
Ancor González Sosa
YaST Team at SUSE Linux GmbH
--
To unsubscribe, e-mail: yast-devel+unsubscribe@xxxxxxxxxxxx
To contact the owner, e-mail: yast-devel+owner@xxxxxxxxxxxx

< Previous Next >
References