Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
robert-strandh opened this issue Sep 21, 2022 · 2 comments
Open

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

robert-strandh opened this issue Sep 21, 2022 · 2 comments

Comments

@robert-strandh
Copy link

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
Copy link
Owner

scymtym commented Sep 21, 2022

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants