Mailinglist Archive: yast-devel (127 mails)

< Previous Next >
Re: [yast-devel] New libstorage: Returning NULL pointers vs. throwing exceptions
On Tue, 24 Nov 2015 15:47:24 +0100
Stefan Hundhammer <shundhammer@xxxxxxx> wrote:

On 24.11.2015 15:15, Josef Reidinger wrote:
Well, in OOP is also one more approach called NullObject pattern,
which is create kind if partition table, that in represent no
partition table.

This is wrong on so many levels that I don't even know where to
begin...

I know it is a bit controversy, similar like Singleton pattern. I agree
that over use it is bad design, but I also think it works nice in some
situations.


This proposed NullObject pretends to do a lot of things that it can't
deliver.

If something cannot be deliver then it is fair to raise exception in
this method. It is common used approach. Where it make sense provide
functionality and where it cannot deliver it, then raise exception.

It makes false promises.

promise depends on API contract.

And I can't even tell that it does
that - to the outside world, it looks just like the real thing; the
difference being that it does not do anything useful, so there is no
reasonable way to handle the other case.

Comparing to `if not nil` or `catch Exception` I think it can do
similar stuff like logging. Of course as I mention above that method
can also raise exception if something is required that cannot be
delivered.



so if NoPartitionTable is used and its partitions return single
partition, that is in fact filesystem on disk.

Only that I really need to know the difference in most cases because
I have to do different things. To stay with the current example: A
disk might have

- an old-style MS-DOS type partition table with 4 slots (where I need
to use an extended partition if I need any more partitions)

- a new style GPT partition table that can have any number of
parititions

- a filesystem directly on the disk device without any partition table

So for me I can see it as three types of partition table - msdos, gpt
and no. I do not see why is better to have

begin
if gpt?
elsif msdos?
end
rescue
# no partition table case
end

then simple

if gpt?
elsif msdos?
elsif no?
end


It sounds tempting to hide those gory details in the lib. But then,
we do need to know a lot about this because we might have to do
different things in the storage or bootloader proposal.

I do not see how how it hides details. It should be accessible for ones
who is interested in it, but hide to ones who is not interested. Like
in example above, if you need behave differently then detect it, but if
you want find boot disk, you are not interested what kind of partition
table is used.

It might just
change the requirements if or when we need any of the boot partitions
(/boot, EFI-boot, PReP) etc. etc.

So, since we are doing deeply technical stuff here, I fear we need to
know and to handle the gory details. Too much information hiding will
become counterproductive here pretty quickly.

For me it is not hiding, it just using specialized object or class as
nil. Object that provide basic functionality or behavior for some cases
and raise exception in others. Class that do not hide it is null one if
you need to know it.

so how can looks implementation for NoPartitionTable ( let me use ruby
pseudocode to make it shorter )

class NoPartitionTable

def create_partition(name, type)
raise UnsupportedOperation, "Cannot create partition for partitionless disk"
end

def delete_partition(name)
# act same as other classes when trying to delete non existing name
end

def get_partitions
# return list with single element containing volume as partition
end

def get_partition(name)
# act same as other classes when trying to get non existing name
end

def get_unused_partition_slot(all = true, logical = true)
return []
end
end


and thats it. So no fake promises, no hidding details and it can be used in
code interested in it.
And advantage is also that usage is same as for other partition tables.


There be dragons. ;-)



JFYI new ruby 2.3 will have &. operator for such cases, so you can
then write it like:

if disk&.partition_table&.gpt?

Yikes, that's really ugly.

I also more like try syntax from active support, but at least I see some logic
in that syntax.


When Moses came down mount Sinai, he must have dropped some stone
slabs
- that one, for example:

Thou shalt not use crazy interpunction in the middle of source
code.

;-)


which looks a bit better

I strongly disagree on that point.

probably matter of taste, I am too lazy to type too much characters :)



OTOH if there would be more checks that might fail (such as the
'if disk && disk.partition_table && disk.partition_table.gpt?'
check above), that one 'begin..rescue' block would handle them
all, and the control flow remains clear and straightforward.

And that's the beauty of it.

Well, I am not sure as you should use more specialized exception
catcher, otherwise you can catch also other problem, that should go
more top level, so in the end, you might want to rescue from three
kinds of exception like

rescue NoPartitionTable, NoDisk, NotGpt => e

Those things tend to accumulate - the exception types as well as the
ones caught in the rescue clauses.

It is questionable. If partitionless disks will be more common, then more code
must cope with it and it makes more methods that have to be aware of it. So in
the end it maybe not accumulate so much. That is reason why exception should be
really exceptional case, which other code is usually not interested in.



Using exceptions and not checks all over the place can help to
clarify the algorithm without clobbering the code with that
checking code - code that is very rarely executed and thus mostly
untested (try to create test cases for all those if/else
branches!).

on other hand testing all potential raiser of exception is also
quite tricky.

There are much less cases to test.

I worry that in more complex or top level code it can end that any line can
raise multiple exceptions, which can make tests hard, but I do not have
experience with using exceptions for this purpose.



Kind regards



Josef
--
To unsubscribe, e-mail: yast-devel+unsubscribe@xxxxxxxxxxxx
To contact the owner, e-mail: yast-devel+owner@xxxxxxxxxxxx

< Previous Next >