Removing magic numbers and other subtleties

[Darshit Shah]

Hi,

I was updating the libpsl API usage in GNU Wget earlier today and
realized something, libpsl returns integer codes for boolean
questions.

On further looking at the source, I saw that libpsl doesn't use any
macros / enums to handle return codes. Even the public functions
return magic numbers. Personally, I've always found using numbers in
the source code to be bad practice and maintenence hell in the future.

However, as I was updating the source to eliminate magic numbers, I
happened upon a question. Is it good practice to return boolean values
from a library? Currently libpsl returns 1 for success and 0 for
failure. I guess returning a true / false value is more verbose and
explicit. I am just not sure of returning such values is considered
good programming practice in a library. Since libpsl doesn't need to
set errno, simple bool values might be a nice thing to have?

--
Thanking You,
Darshit Shah

[Tim Ruehsen]

On Friday 04 July 2014 21:02:31 Darshit Shah wrote:
> Hi,
>
> I was updating the libpsl API usage in GNU Wget earlier today and
> realized something, libpsl returns integer codes for boolean
> questions.
>
> On further looking at the source, I saw that libpsl doesn't use any
> macros / enums to handle return codes. Even the public functions
> return magic numbers. Personally, I've always found using numbers in
> the source code to be bad practice and maintenence hell in the future.
>
> However, as I was updating the source to eliminate magic numbers, I
> happened upon a question. Is it good practice to return boolean values
> from a library? Currently libpsl returns 1 for success and 0 for
> failure. I guess returning a true / false value is more verbose and
> explicit.

The library is C89 - there is no 'bool' there are integer numbers 0 and 1.
What you want is a return type of 1 bit width - which is technically is an
integer type. Meanwhile there are bitfields available in C you can't return
one (AFAIK). And it would be very obscure - ireally never saw this.

> I am just not sure of returning such values is considered
> good programming practice in a library. Since libpsl doesn't need to
> set errno, simple bool values might be a nice thing to have?

Yes, you get "bool" values. 0 and !0 (which is 1 in C).

But if one of those functions return 'magic' numbers (neither 0 nor 1), it
should be fixed.

Tim

[Daniel Stenberg]

On Fri, 4 Jul 2014, Darshit Shah wrote:

> Currently libpsl returns 1 for success and 0 for failure.

That is somewhat unusual for libraries I think. It is more common to use 0 for
success and then non-zero for errors. You can then also reserve the freedom to
introduced more error codes in the future...

--

/ daniel.haxx.se

[Darshit Shah]

That was the first reason why I wanted to abstract this. Returning a 1
for success seemed very different from what I'm used to.

Okay, I get that because of the C89 compatibility issues we probably
cannot use bool values. Maybe we can fix the return codes then? I
would really like to see standard values there so that developers
don't make a mistake when using libpsl.

> --
>
> / daniel.haxx.se

[Rockdaboot]

Hmm, i thought Darshit talked about the psl_is_ functions. Other functions like the utf8lower one return if course 0 for ok else non-zero. These values are defined in the header file and documented. Ti

Am 04.07.2014 17:56 schrieb Daniel Stenberg <dan...@haxx.se>:
>
> On Fri, 4 Jul 2014, Darshit Shah wrote:
>
> > Currently libpsl returns 1 for success and 0 for failure.
>
> That is somewhat unusual for libraries I think. It is more common to use 0 for
> success and then non-zero for errors. You can then also reserve the freedom to
> introduced more error codes in the future...
>

> --
>
>   / daniel.haxx.se
>
> --
> You received this message because you are subscribed to the Google Groups "libpsl-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to libpsl-bugs...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Darshit Shah]

I meant both earlier. utf8lower uses an enum to declare error codes
which are pretty clear. And success is 0, so that is fine. However,
the psl_is_* family of functions returns 1 for true which is
inconsistent and causes problems. While implementing in Wget, I had to
constantly remember that the return codes here are the opposite of
what is expected.

[Daniel Kahn Gillmor]

I agree that zero-for-success, non-zero-for-failure is a better
convention, because it allows for alternate error codes.

I believe this is known as the "Anna Karenina principle" -- happy
functions are all alike, but unhappy functions are unhappy in their own
ways.

--dkg

[Tim Ruehsen]

Ok, i got it. I also do lot's of java. So i am used to is_ functions. These are very handy - no error possible... Ok java has exceptions. We have to rename these functions then. They ask a question and thus return yes or no, 1 or 0. Very intuitive to me. Why didn't you complain earlier... Please make suggestions for reasonable function naming. Tim

[Darshit Shah]

It's all fine. I think the function names are okay, they're a bit
long, but we can work with that.

Just change the return values. 0 for true and 1 for false. Since that
is what we're all used to in C.

[Tim Ruehsen]

On Saturday 05 July 2014 11:42:29 Darshit Shah wrote:
> It's all fine. I think the function names are okay, they're a bit
> long, but we can work with that.
>
> Just change the return values. 0 for true and 1 for false. Since that
> is what we're all used to in C.

Hi Darshit,

my last answer was a bit short. I was standing in a crowded, hot train being
shaken while typing on this mini touch keyboard of my mobile phone :-(

There is a 'is' class of functions that do not return errors but just 0 for
'No/False' and 1 for 'Yes/True'. Example: isatty(), isdigit(), ...

These functions simply ask a question that has to be answered with Yes or No.
You normally use it like
if (isupper(c)) ...
and NOT like
if (isupper(c)==1) ...

Your idea now is to change these functions and turn around the logic. Ok, BUT
one should also rename these function at the same time to e.g. isnotupper() to
use it like
if (isnotupper(c)==0)
which you ask for.


Right now you use the 'is' PSL functions like
if (psl_is_public_suffix(psl, domain)) ...
If you insist on checking for 0 (or a define) as success, than we should
rename these functions to e.g.
if ((rc=psl_test_public_suffix(psl, domain))==PSL_SUCCESS) print 'suffix'
else print error code 'rc'.

My design goal was to provide as-easy-as-possible functions.
But you might be right and we should return an error code... but that might
not be enough. A programmer might also be interested in why the param is a
public suffix (Unknown TLD / prvailing * match, Domain might be an exception,
domain has too many labels to be a PS, ...).

For this case we could define some codes and extend the function like
if (psl_is_public_suffix(psl, domain, &why)) {
print The <domain> is a public suffix because <why>
} else {
print The <domain> is *NOT* a public suffix because <why>
}

What do you think ?

And strictly spoken, library functions should also not return char *, like
psl_unregistrable_domain()... it should return an error code and take a third
argument char **ret to return the allocated string.

It is simply a design decision. And so far I decided for simple functions
which drop some details. If you really need more how would you like it ?
We could also have a set of 'simple' function (that we already have) plus
'detailed' functions (see above) ... everything is possible.

Tim