On 03/01/2016 10:01 AM, Josef Reidinger wrote:
> On Mon, 29 Feb 2016 17:31:46 +0100
> Stefan Hundhammer <[email protected]> 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: [email protected]
To contact the owner, e-mail: [email protected]
