[asterisk-dev] [Code Review]: app_alarmreceiver cleanup and fixes

[Pedro Kiefer]

Rmudgett,

When I started this clean up my main goal was to improve the overall readability of the code, especially the main loop of the app. I think this patch achieves this, but there is lots of room for improvements. KNK has a few patches with new features and cleanups for this app too, and I might make some others. Your review is very much appreciated, I'll cook a patch to clean up this function, and also the NULL string ending issue that Mark commented.

Sorry for not commenting on the reviewboard, as I don't have access to it dev-list is the only option. I'll post the new patch on one of the related JIRA ticket.

Cheers,
Pedro Kiefer

On Aug 20, 2012, at 4:18 PM, rmudgett <reviewboard at asterisk.org> wrote:

> 
> This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2075/
> 
> On August 20th, 2012, 1:44 p.m., Mark Michelson wrote:
> 
> /trunk/apps/app_alarmreceiver.c (Diff revision 3)
> static int ademco_verify_checksum(char *event)
> 362	
> static int ademco_verify_checksum(char *event)
> 363	
> {
> 364	
> 	static char digit_map[15] = "0123456789*#ABC";
> 365	
> 	static unsigned char digit_weights[15] = { 10, 1, 2, 3, 4, 5, 6, 7, 8, 9, 11, 12, 13, 14, 15 };
> 366	
> 367	
> 	int checksum = 0;
> 368	
> 	int i, j;
> 369	
> 370	
> 	for (j = 0, checksum = 0; j < 16; j++) {
> 371	
> 		for (i = 0; i < sizeof(digit_map); i++) {
> 372	
> 			if (digit_map[i] == event[j]) {
> 373	
> 				break;
> 416	
> 		}
> 374	
> 			}
> 417	
> 	}
> 375	
> 		}
> 418	
> 376	
> 419	
> 	return res;
> 377	
> 		if (i == 16) {
> 378	
> 			break;
> 379	
> 		}
> 380	
> 		checksum += digit_weights[i];
> 420	
> }
> 381	
> 	}
> There are a few things I don't like here.
> 
> First, I think you should pass the size of the event as a parameter to this function. This way there are fewer magic 16s being used in this function. I had to look a few times to make sure you could not possibly access data you should not be accessing.
> 
> Second, you can declare the arrays without the size being specified (e.g. static char digit_map[] = "0123456789*#ABC";)
> 
> Third, instead of using sizeof(digit_map) use ARRAY_LEN(digit_map). In this case, they evaluate to the same thing, but using ARRAY_LEN makes it much clearer that you mean the length of the array.
> You have to declare digit_map[15] = "0123456789*#ABC" because initializing it with a string would also includes a nul terminator in the array size.  Of course you could initialize it character by character.  Then you would not need the explicit array dimension.
> 
> Another way to declare this so you know what maps with what is:
> struct {
>   char digit;
>   char weight;
> } mapping[] = { {'0', 10 },... };
> 
> - rmudgett
> 
> 
> On August 17th, 2012, 1:17 p.m., KNK wrote:
> 
> Review request for Asterisk Developers.
> By KNK.
> Updated Aug. 17, 2012, 1:17 p.m.
> 
> Description
> 
> Review on behalf of Pedro Kiefer: cleanups from ASTERISK-20224, ASTERISK-20157 and the updated ASTERISK-16694 (author Fred van Lieshout) + improvement from ASTERISK-20158
> There are several issues opened for bugs in app_alarmreceiver (ASTERISK-19435, ASTERISK-18417, ASTERISK-16694 and ASTERISK-16668), which go away after the cleanup (ASTERISK-16694) replacing the proprietary toneburst code with ast_playtones_ functions, so the fix from ASTERISK-19435 is no longer necessary.
> The new functionality from ASTERISK-20158 is a bugfix on my opinion, because 'we get stuck on the alarm receiver app until it times out'
> Testing
> 
> Compile test only
> Bugs: ASTERISK-16668, ASTERISK-16694, ASTERISK-18417, ASTERISK-19435, ASTERISK-20157, ASTERISK-20158, ASTERISK-20224
> Diffs
> 
> /trunk/apps/app_alarmreceiver.c (370760)
> View Diff
> 
> --
> _____________________________________________________________________
> -- Bandwidth and Colocation Provided by http://www.api-digital.com --
> 
> asterisk-dev mailing list
> To UNSUBSCRIBE or update options visit:
>   http://lists.digium.com/mailman/listinfo/asterisk-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120820/ef8c7616/attachment.htm>