search

scymtym / architecture.builder-protocol

Protocol for flexible construction and traversal of results (e.g. ASTs in case of parsers)
GNU Lesser General Public License v3.0
8 stars 1 forks source link

Definition of relation-slot? looks very strange. #11

Open robert-strandh opened 1 year ago

robert-strandh commented 1 year ago

The definition of relation-slot? looks very strange. A minor issue is that the Scheme convention for predicates is used. But the major strangeness is that the function is presumably a predicate, but then it returns the value of a call to slot-type->cardinality, which presumably returns a cardinality which does not look like a Boolean value. So this definition is not understandable by itself and forces the reader to understand the details of a different function. A cardinality is usually a number, but this library also defines a relation-cardinality, which is also not a Boolean value.

scymtym commented 1 year ago

Would writing the body as

(and (not (null (c2mop:slot-definition-initargs slot)))
     (not (null (slot-type->cardinality (c2mop:slot-definition-type slot)))))

make the function easier to understand? The function is used as a predicate, is named like a predicate and would, when changed like that, also behave like a predicate.

robert-strandh commented 1 year ago

Not much.

It looks to me like there ought to be a name for the case where the the type of the slot is NIL, like SLOT-HAS-CARDINALITY-P (I am just guessing, I don't know the right name), so that slot-type->cardinality would signal an error if this new thing returns NIL.

In other words, I think it is confusing to overload a function with a name that suggests that it returns a cardinality so that it also returns NIL when there is no cardinality available. You could of course rename it to slot-type->cardinality-or-nil-if-there-is-no-cardinality, but when names must look like this to explain what they do, it indicates to me that there is something wrong with the abstraction.

I know that there are plenty of standard functions that do this, and this practice was no doubt kept for reasons of backwards compatibility. We all learn how these functions work after many years of practice, but I don't think it is a good idea to continue this practice in new code that new people must read and understand.

I also know that a library such a Clostrum contains many cases of NIL being returned as a default value, like for FDEFINITION. However, the Clostrum functions mimic standard functions which is part of the reason. And, I don't use a call to FDEFINITÎON in a context where a Boolean value is expected.

Then, if the universal builder is not a good idea, it doesn't matter much.