Re: [PATCH] Useless var in smsc_emi.c

agree'ing here... ++1

--
Telemaque - 06200 NICE - (FR)
Service Technique/Reseau - NOC 
Developpement SMS/MMS/Kiosques
http://www.telemaque.fr/
[email protected]
Tel : +33 4 93 97 71 64 (fax 68)

----- Original Message ----- 
From: "Colin Pitrat" <[email protected]>
To: "Kannel Devel" <[email protected]>
Sent: Friday, August 18, 2006 10:32 AM
Subject: [PATCH] Useless var in smsc_emi.c


Hello,
it looks like in the following piece of code (gw/smsc/smsc_emi.c:1080), 
the adc var is useless :

----------------------------------------------------------------

if (octstr_get_char(emimsg->fields[0], 0) == 'A') {
   /* we got an ack back. We might have to store the */
   /* timestamp for delivery notifications now */
   Octstr *ts, *adc;
   int     i;
   Msg *m;

   ts = octstr_duplicate(emimsg->fields[2]);
   if (octstr_len(ts)) {
     i = octstr_search_char(ts,':',0);
     if (i>0) {
       octstr_delete(ts,0,i+1);
       adc = octstr_duplicate(emimsg->fields[2]);
       octstr_truncate(adc,i);

       m = PRIVDATA(conn)->slots[emimsg->trn].sendmsg;
       if(m == NULL) {
         info(0,"EMI2[%s]: uhhh m is NULL, very bad",
                octstr_get_cstr(privdata->name));
     } else if (DLR_IS_ENABLED_DEVICE(m->sms.dlr_mask)) {
       dlr_add((conn->id ? conn->id : privdata->name), ts, m);
     }
     octstr_destroy(ts);
     octstr_destroy(adc);
   } else {
     octstr_destroy(ts);
   }
}

----------------------------------------------------------------

I think that the adc variable is useless, as it's only used to retrieve 
a part of emimsg->fields[2] and then it's destroyed without using it.

Another remark is that variables should be named with more explicit 
names. Short names doesn't make the code faster, but it makes it harder 
to read. In this example, maybe ts could be named timestamp, maybe m 
could be named message etc ... It does'nt even allow to code faster, as 
a comment has to explain that we store the timestamp.

-- 
Colin Pitrat (Bull Services Telco)
Bull,  Architect of an Open World (TM)
Tél : +33 (0)  1 30 80 72 93
www.bull.com



--------------------------------------------------------------------------------


> --- gateway/gw/smsc/smsc_emi.c 2006-08-18 10:21:59.000000000 +0200
> +++ gateway-new/gw/smsc/smsc_emi.c 2006-08-18 10:18:59.000000000 +0200
> @@ -1080,7 +1080,7 @@
>      if (octstr_get_char(emimsg->fields[0], 0) == 'A') {
>  /* we got an ack back. We might have to store the */
>  /* timestamp for delivery notifications now */
> - Octstr *ts, *adc;
> + Octstr *ts;
>  int i;
>  Msg *m;
> 
> @@ -1089,8 +1089,6 @@
>      i = octstr_search_char(ts,':',0);
>      if (i>0) {
>  octstr_delete(ts,0,i+1);
> - adc = octstr_duplicate(emimsg->fields[2]);
> - octstr_truncate(adc,i);
> 
>  m = PRIVDATA(conn)->slots[emimsg->trn].sendmsg;
>  if(m == NULL) {
> @@ -1100,7 +1098,6 @@
>      dlr_add((conn->id ? conn->id : privdata->name), ts, m);
>  }
>  octstr_destroy(ts);
> - octstr_destroy(adc);
>      } else {
>  octstr_destroy(ts);
>      }
>