[RFC] "magic" numbers (145,129)

[RFC] "magic" numbers (145,129)

[George Matveev]

[-- Attachment #1: Type: text/plain, Size: 2559 bytes --]

Hi,

this is proposal for a patch series which would replace
"magic" numbers 145 and 129 with well defined enumeration.

Currently we have the following (eliminating sms and CAIF related code):

geo(a)fermat:/home/work/ofono/ofono$ grep -nr 145 drivers src|grep -v
sms|grep -v CAIF
drivers/huaweimodem/voicecall.c:123:	if (ph->type == 145)
drivers/stemodem/voicecall.c:195:	if (ph->type == 145)
drivers/atmodem/voicecall.c:374:	if (ph->type == 145)
drivers/calypsomodem/voicecall.c:88:	if (ph->type == 145)
drivers/hfpmodem/voicecall.c:369:	if (ph->type == 145)
drivers/ifxmodem/voicecall.c:313:	if (ph->type == 145)
src/phonebook.c:41:#define TYPE_INTERNATIONAL 145
src/common.c:74:	{ 145,	"Message class not supported" },
src/common.c:408:	if (ph->type == 145 && (strlen(ph->number) > 0) &&
src/common.c:425:		ph->type = 145;	/* International */
geo(a)fermat:/home/work/ofono/ofono$

Even though TYPE_INTERNATIONAL is defined it is used only once:

geo(a)fermat:/home/work/ofono/ofono$ grep -nr _INTERNATIONAL .|grep -v SMS
./src/phonebook.c:41:#define TYPE_INTERNATIONAL 145
./src/phonebook.c:179:	if ((type == TYPE_INTERNATIONAL) && (number[0] !=
'+'))
geo(a)fermat:/home/work/ofono/ofono$

And for "magic" number 129 we have:

geo(a)fermat:/home/work/ofono/ofono$ grep -nr 129 drivers src|grep -v
sms|grep -v CAIF
drivers/atmodem/call-forwarding.c:92:		list[num].phone_number.type = 129;
drivers/atmodem/atutil.c:115:		int number_type = 129;
drivers/atmodem/ssn.c:72:	ph.type = 129;
drivers/calypsomodem/voicecall.c:298:		type = 129;
src/call-forwarding.c:794:		ph.type = 129;
src/common.c:70:	{ 129,	"Short message type 0 not supported" },
src/common.c:428:		ph->type = 129;	/* Local */
geo(a)fermat:/home/work/ofono/ofono$


So if we introduce enum phone_number_type in src/common.h
similar to sms_number_type defined in smsutils.h:

enum phone_number_type {
	PHONE_NUMBER_TYPE_LOCAL = 0,
	PHONE_NUMBER_TYPE_INTER = 1
};

and use it consistently with phone type then we may avoid magic numbers
and need to guess what they mean.

Also if we do so there will be no need to include any new headers in
voicecall.c files in case of
stemodem and hfpmodem drivers. Only voicecall.c for atmodem, calypsomodem,
and ifxmodem drivers will have to include common.h.

If project maintainers agree that such patch (series) might make oFono
code more structured
and easier to understand (no need to comment on each line) I'll submit a
patch series shortly.

Best regards,
George Matveev

RE: [RFC] "magic" numbers (145,129)

[Rajesh.Nagaiah]

[-- Attachment #1: Type: text/plain, Size: 2638 bytes --]

Hi George, 

> Hi,
> 
> this is proposal for a patch series which would replace 
> "magic" numbers 145 and 129 with well defined enumeration.
> 
> Currently we have the following (eliminating sms and CAIF 
> related code):
> 
> geo(a)fermat:/home/work/ofono/ofono$ grep -nr 145 drivers src|grep -v
> sms|grep -v CAIF
> drivers/huaweimodem/voicecall.c:123:	if (ph->type == 145)
> drivers/stemodem/voicecall.c:195:	if (ph->type == 145)
> drivers/atmodem/voicecall.c:374:	if (ph->type == 145)
> drivers/calypsomodem/voicecall.c:88:	if (ph->type == 145)
> drivers/hfpmodem/voicecall.c:369:	if (ph->type == 145)
> drivers/ifxmodem/voicecall.c:313:	if (ph->type == 145)
> src/phonebook.c:41:#define TYPE_INTERNATIONAL 145
> src/common.c:74:	{ 145,	"Message class not supported" },
> src/common.c:408:	if (ph->type == 145 && 
> (strlen(ph->number) > 0) &&
> src/common.c:425:		ph->type = 145;	/* International */
> geo(a)fermat:/home/work/ofono/ofono$
> 
> Even though TYPE_INTERNATIONAL is defined it is used only once:
> 
> geo(a)fermat:/home/work/ofono/ofono$ grep -nr _INTERNATIONAL 
> .|grep -v SMS ./src/phonebook.c:41:#define TYPE_INTERNATIONAL 145
> ./src/phonebook.c:179:	if ((type == 
> TYPE_INTERNATIONAL) && (number[0] !=
> '+'))
> geo(a)fermat:/home/work/ofono/ofono$
> 
> And for "magic" number 129 we have:
> 
> geo(a)fermat:/home/work/ofono/ofono$ grep -nr 129 drivers src|grep -v
> sms|grep -v CAIF
> drivers/atmodem/call-forwarding.c:92:		
> list[num].phone_number.type = 129;
> drivers/atmodem/atutil.c:115:		int number_type = 129;
> drivers/atmodem/ssn.c:72:	ph.type = 129;
> drivers/calypsomodem/voicecall.c:298:		type = 129;
> src/call-forwarding.c:794:		ph.type = 129;
> src/common.c:70:	{ 129,	"Short message type 0 not supported" },
> src/common.c:428:		ph->type = 129;	/* Local */
> geo(a)fermat:/home/work/ofono/ofono$
> 
> 
> So if we introduce enum phone_number_type in src/common.h 
> similar to sms_number_type defined in smsutils.h:
> 
> enum phone_number_type {
> 	PHONE_NUMBER_TYPE_LOCAL = 0,
> 	PHONE_NUMBER_TYPE_INTER = 1
> };
> 
> and use it consistently with phone type then we may avoid 
> magic numbers and need to guess what they mean.

145 and 129, doesnt only represent the type of number (TON), but also
includes the numbering plan indentification (NPI). 
145 - 1 001 0001 (1- EXT, 001 - international number, 0001 -
ISDN/telephony numbering plan )
129 - 1 000 0001 (1- EXT, 000 - unknown, 0001 - ISDN/telephony numbering
plan )

In 129 TON is unknown type, not Local or National, so it should be
_UNKNOWN not _LOCAL

BR,
Rajesh

Re: [RFC] "magic" numbers (145,129)

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3327 bytes --]

Hi George,

On 01/16/2011 12:39 PM, George Matveev wrote:
> Hi,
> 
> this is proposal for a patch series which would replace
> "magic" numbers 145 and 129 with well defined enumeration.
> 
> Currently we have the following (eliminating sms and CAIF related code):
> 
> geo(a)fermat:/home/work/ofono/ofono$ grep -nr 145 drivers src|grep -v
> sms|grep -v CAIF
> drivers/huaweimodem/voicecall.c:123:	if (ph->type == 145)
> drivers/stemodem/voicecall.c:195:	if (ph->type == 145)
> drivers/atmodem/voicecall.c:374:	if (ph->type == 145)
> drivers/calypsomodem/voicecall.c:88:	if (ph->type == 145)
> drivers/hfpmodem/voicecall.c:369:	if (ph->type == 145)
> drivers/ifxmodem/voicecall.c:313:	if (ph->type == 145)
> src/phonebook.c:41:#define TYPE_INTERNATIONAL 145
> src/common.c:74:	{ 145,	"Message class not supported" },
> src/common.c:408:	if (ph->type == 145 && (strlen(ph->number) > 0) &&
> src/common.c:425:		ph->type = 145;	/* International */
> geo(a)fermat:/home/work/ofono/ofono$
> 
> Even though TYPE_INTERNATIONAL is defined it is used only once:
> 
> geo(a)fermat:/home/work/ofono/ofono$ grep -nr _INTERNATIONAL .|grep -v SMS
> ./src/phonebook.c:41:#define TYPE_INTERNATIONAL 145
> ./src/phonebook.c:179:	if ((type == TYPE_INTERNATIONAL) && (number[0] !=
> '+'))
> geo(a)fermat:/home/work/ofono/ofono$
> 
> And for "magic" number 129 we have:
> 
> geo(a)fermat:/home/work/ofono/ofono$ grep -nr 129 drivers src|grep -v
> sms|grep -v CAIF
> drivers/atmodem/call-forwarding.c:92:		list[num].phone_number.type = 129;
> drivers/atmodem/atutil.c:115:		int number_type = 129;
> drivers/atmodem/ssn.c:72:	ph.type = 129;
> drivers/calypsomodem/voicecall.c:298:		type = 129;
> src/call-forwarding.c:794:		ph.type = 129;
> src/common.c:70:	{ 129,	"Short message type 0 not supported" },
> src/common.c:428:		ph->type = 129;	/* Local */
> geo(a)fermat:/home/work/ofono/ofono$
> 
> 
> So if we introduce enum phone_number_type in src/common.h
> similar to sms_number_type defined in smsutils.h:
> 
> enum phone_number_type {
> 	PHONE_NUMBER_TYPE_LOCAL = 0,
> 	PHONE_NUMBER_TYPE_INTER = 1
> };
> 
> and use it consistently with phone type then we may avoid magic numbers
> and need to guess what they mean.
> 

As Rajesh mentions in the other reply you at least have to assign proper
values to the enum.  Assigning 0 and 1 is not going to work.  Also,
please use proper naming.  PHONE_NUMBER_TYPE_INTER makes no sense to me.
 If you mean INTERNATIONAL then please spell that out.

Refer to 24.008 Section 10.5.4.7 for more details.

> Also if we do so there will be no need to include any new headers in
> voicecall.c files in case of
> stemodem and hfpmodem drivers. Only voicecall.c for atmodem, calypsomodem,
> and ifxmodem drivers will have to include common.h.
> 
> If project maintainers agree that such patch (series) might make oFono
> code more structured
> and easier to understand (no need to comment on each line) I'll submit a
> patch series shortly.

Personally I don't really consider these as 'magic numbers'.  They're so
widely used in 27.007 that if you don't know what type 145/128/129 is,
you have bigger problems ;)  However, if you really feel this makes
things better I'm okay with accepting such a patch series.

Regards,
-Denis

Re: [RFC] "magic" numbers (145,129)

[George Matveev]

[-- Attachment #1: Type: text/plain, Size: 4703 bytes --]

Rajesh and Denis, thank you for your comments.

It is really comforting to know that project maintainer knows about
specifications and could provide guidance to other developers.
This is what most contributors have come to expect from them.

What makes me wonder is why don't we use those specs consistently?

Instead of relying on sporadic attempts to define number types ad hock,
in different parts of project (see grep results) and using inconsistent
or simply wrong comments, wouldn't it be more logical to use spec?

Denis, you may be surprised to know that such an attempt had already
been made in the past (though possibly in a wrong place - src/driver.h)
and that patch was accepted by someone called Denis Kenzior :-)

Just have a look into oFono archive here:

http://lists.ofono.org/pipermail/ofono/2009-June/000130.html

Since other developers seem to agree that it'll make project code
more organized and readable I think I'll submit a patch using all
those values defined in 3GPP TS 24.008, section 10.5.4.7
"Called party BCD number", Table 10.5.118.

What I am not entirely sure though is a name of new enumeration,
possibly *called_number_type* would be more appropriate
and also more in line with the spec?

Best regards,
George

> Hi George,
>
> On 01/16/2011 12:39 PM, George Matveev wrote:
>> Hi,
>>
>> this is proposal for a patch series which would replace
>> "magic" numbers 145 and 129 with well defined enumeration.
>>
>> Currently we have the following (eliminating sms and CAIF related code):
>>
>> geo(a)fermat:/home/work/ofono/ofono$ grep -nr 145 drivers src|grep -v
>> sms|grep -v CAIF
>> drivers/huaweimodem/voicecall.c:123:	if (ph->type == 145)
>> drivers/stemodem/voicecall.c:195:	if (ph->type == 145)
>> drivers/atmodem/voicecall.c:374:	if (ph->type == 145)
>> drivers/calypsomodem/voicecall.c:88:	if (ph->type == 145)
>> drivers/hfpmodem/voicecall.c:369:	if (ph->type == 145)
>> drivers/ifxmodem/voicecall.c:313:	if (ph->type == 145)
>> src/phonebook.c:41:#define TYPE_INTERNATIONAL 145
>> src/common.c:74:	{ 145,	"Message class not supported" },
>> src/common.c:408:	if (ph->type == 145 && (strlen(ph->number) > 0) &&
>> src/common.c:425:		ph->type = 145;	/* International */
>> geo(a)fermat:/home/work/ofono/ofono$
>>
>> Even though TYPE_INTERNATIONAL is defined it is used only once:
>>
>> geo(a)fermat:/home/work/ofono/ofono$ grep -nr _INTERNATIONAL .|grep -v SMS
>> ./src/phonebook.c:41:#define TYPE_INTERNATIONAL 145
>> ./src/phonebook.c:179:	if ((type == TYPE_INTERNATIONAL) && (number[0] !=
>> '+'))
>> geo(a)fermat:/home/work/ofono/ofono$
>>
>> And for "magic" number 129 we have:
>>
>> geo(a)fermat:/home/work/ofono/ofono$ grep -nr 129 drivers src|grep -v
>> sms|grep -v CAIF
>> drivers/atmodem/call-forwarding.c:92:		list[num].phone_number.type =
>> 129;
>> drivers/atmodem/atutil.c:115:		int number_type = 129;
>> drivers/atmodem/ssn.c:72:	ph.type = 129;
>> drivers/calypsomodem/voicecall.c:298:		type = 129;
>> src/call-forwarding.c:794:		ph.type = 129;
>> src/common.c:70:	{ 129,	"Short message type 0 not supported" },
>> src/common.c:428:		ph->type = 129;	/* Local */
>> geo(a)fermat:/home/work/ofono/ofono$
>>
>>
>> So if we introduce enum phone_number_type in src/common.h
>> similar to sms_number_type defined in smsutils.h:
>>
>> enum phone_number_type {
>> 	PHONE_NUMBER_TYPE_LOCAL = 0,
>> 	PHONE_NUMBER_TYPE_INTER = 1
>> };
>>
>> and use it consistently with phone type then we may avoid magic numbers
>> and need to guess what they mean.
>>
>
> As Rajesh mentions in the other reply you at least have to assign proper
> values to the enum.  Assigning 0 and 1 is not going to work.  Also,
> please use proper naming.  PHONE_NUMBER_TYPE_INTER makes no sense to me.
>  If you mean INTERNATIONAL then please spell that out.
>
> Refer to 24.008 Section 10.5.4.7 for more details.
>
>> Also if we do so there will be no need to include any new headers in
>> voicecall.c files in case of
>> stemodem and hfpmodem drivers. Only voicecall.c for atmodem,
>> calypsomodem,
>> and ifxmodem drivers will have to include common.h.
>>
>> If project maintainers agree that such patch (series) might make oFono
>> code more structured
>> and easier to understand (no need to comment on each line) I'll submit a
>> patch series shortly.
>
> Personally I don't really consider these as 'magic numbers'.  They're so
> widely used in 27.007 that if you don't know what type 145/128/129 is,
> you have bigger problems ;)  However, if you really feel this makes
> things better I'm okay with accepting such a patch series.
>
> Regards,
> -Denis
>

Re: [RFC] "magic" numbers (145,129)

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2432 bytes --]

Hi George,

Kind reminder to please not top on this mailing list.

On 01/17/2011 03:10 PM, George Matveev wrote:
> Rajesh and Denis, thank you for your comments.
> 
> It is really comforting to know that project maintainer knows about
> specifications and could provide guidance to other developers.
> This is what most contributors have come to expect from them.
> 
> What makes me wonder is why don't we use those specs consistently?
> 
> Instead of relying on sporadic attempts to define number types ad hock,
> in different parts of project (see grep results) and using inconsistent
> or simply wrong comments, wouldn't it be more logical to use spec?
> 

Careful there, certainly we are extremely anal about using the specs and
the appropriate enums when it is... appropriate.  However, we break this
rule if it is prudent or makes the code look ugly.  If an enumeration
can be localized to a single function, then don't bother creating an
enum as this only makes the code harder to read.  It is fine to use
magic values if they are localized and you wouldn't understand the
function without the spec in hand.  Browse through stkutil.c, simutil.c
for plenty of examples of this.

There maybe parts that have suffered from bit-rot more than others, but
in general your criticism is a little misplaced.  I consider 145/129
thing to be border-line and can see it both ways.

> Denis, you may be surprised to know that such an attempt had already
> been made in the past (though possibly in a wrong place - src/driver.h)
> and that patch was accepted by someone called Denis Kenzior :-)
> 
> Just have a look into oFono archive here:
> 
> http://lists.ofono.org/pipermail/ofono/2009-June/000130.html

Yes, and I wonder where this code is now? Oh yes, it was refactored out
by the same guy because it was not actually used in the end ;)

> 
> Since other developers seem to agree that it'll make project code
> more organized and readable I think I'll submit a patch using all
> those values defined in 3GPP TS 24.008, section 10.5.4.7
> "Called party BCD number", Table 10.5.118.
> 
> What I am not entirely sure though is a name of new enumeration,
> possibly *called_number_type* would be more appropriate
> and also more in line with the spec?

Depends on whether you want to separate the number type from the
numbering plan or keep it as a single enum.

Regards,
-Denis