LINUX.ORG.RU

История изменений

Исправление firkax, (текущая версия) :

Как и ожидалось, общие места вполне есть. В начале уберём отдельные ветки по наличию/отсутствию name 

///   PREPARE AT CMD
	
	if( phonebookEntry->index < 0 && phonebookEntry->telNo){
		res = snprintf( gsmService->atCmdBuffer, AT_CMD_BUFFER_SIZE, "AT+CPBW=,\"%s\"", phonebookEntry->telNo );
		if( res < 0 ) return -1;
                if(phonebookEntry->name) {
			if( phonebookEntry->telNo[0] == '+' ) type = 145;
			else                                  type = 129;
			res = packUtf82Ucs2( phonebookEntry->name, gsmService->codecBuffer, CODEC_BUFFER_SIZE );
			if( res < 0 ) return -1;		
			res = snprintf(
				gsmService->atCmdBuffer+strlen(gsmService->atCmdBuffer),
				AT_CMD_BUFFER_SIZE-strlen(gsmService->atCmdBuffer),
				",%i,\"%s\"",
				type,
				gsmService->codecBuffer
			);
			if( res < 0 ) return -1;
                }
                res = snprintf(gsmService->atCmdBuffer+strlen(gsmService->atCmdBuffer), AT_CMD_BUFFER_SIZE-strlen(gsmService->atCmdBuffer), "\r");		
	}else if( phonebookEntry->index >= 0 && !phonebookEntry->telNo && !phonebookEntry->name ){
		
		//только индекс
		res = snprintf( gsmService->atCmdBuffer, AT_CMD_BUFFER_SIZE, "AT+CPBW=%i\r", phonebookEntry->index );
		
	}else if( phonebookEntry->index >= 0 && phonebookEntry->telNo){
		res = snprintf( gsmService->atCmdBuffer, AT_CMD_BUFFER_SIZE, "AT+CPBW=%i,\"%s\"", phonebookEntry->index, phonebookEntry->telNo );
		if( res < 0 ) return -1;
                if(phonebookEntry->name) {
			if( phonebookEntry->telNo[0] == '+' ) type = 145;
			else                                  type = 129;
			res = packUtf82Ucs2( phonebookEntry->name, gsmService->codecBuffer, CODEC_BUFFER_SIZE );
			if( res < 0 ) return -1;		
			res = snprintf(
				gsmService->atCmdBuffer+strlen(gsmService->atCmdBuffer),
				AT_CMD_BUFFER_SIZE-strlen(gsmService->atCmdBuffer),
				",%i,\"%s\"",
				type,
				gsmService->codecBuffer
			);
			if( res < 0 ) return -1;
                }
                res = snprintf(gsmService->atCmdBuffer+strlen(gsmService->atCmdBuffer), AT_CMD_BUFFER_SIZE-strlen(gsmService->atCmdBuffer), "\r");
	}else{
		//недопустимое сочетание
		return -1;
	}
	
	if( res < 0 ) return -1;
Видим что ветки с индексом и без индекса оказались почти одинаковые, объединяем их тоже 
///   PREPARE AT CMD
	if(phonebookEntry->telNo){
		res = snprintf( gsmService->atCmdBuffer, AT_CMD_BUFFER_SIZE, "AT+CPBW=");
		if( res < 0 ) return -1;
		if(phonebookEntry->index >= 0) {
			res = snprintf( gsmService->atCmdBuffer+strlen(gsmService->atCmdBuffer), AT_CMD_BUFFER_SIZE-strlen(gsmService->atCmdBuffer), "%i", phonebookEntry->index);
			if( res < 0 ) return -1;
		}
		res = snprintf( gsmService->atCmdBuffer+strlen(gsmService->atCmdBuffer), AT_CMD_BUFFER_SIZE-strlen(gsmService->atCmdBuffer), ",\"%s\"", phonebookEntry->telNo );
		if( res < 0 ) return -1;
                if(phonebookEntry->name) {
			if( phonebookEntry->telNo[0] == '+' ) type = 145;
			else                                  type = 129;
			res = packUtf82Ucs2( phonebookEntry->name, gsmService->codecBuffer, CODEC_BUFFER_SIZE );
			if( res < 0 ) return -1;		
			res = snprintf(
				gsmService->atCmdBuffer+strlen(gsmService->atCmdBuffer),
				AT_CMD_BUFFER_SIZE-strlen(gsmService->atCmdBuffer),
				",%i,\"%s\"",
				type,
				gsmService->codecBuffer
			);
			if( res < 0 ) return -1;
                }
                res = snprintf(gsmService->atCmdBuffer+strlen(gsmService->atCmdBuffer), AT_CMD_BUFFER_SIZE-strlen(gsmService->atCmdBuffer), "\r");		
	}else if( phonebookEntry->index >= 0 && !phonebookEntry->telNo && !phonebookEntry->name ){
		//только индекс
		res = snprintf( gsmService->atCmdBuffer, AT_CMD_BUFFER_SIZE, "AT+CPBW=%i\r", phonebookEntry->index );
	}else{
		//недопустимое сочетание
		return -1;
	}
	
	if( res < 0 ) return -1;
По-моему, получившийся код намного нагляднее (ещё бы имена переменных исправить...). Решены сразу несколько проблем:

1) осталось всего 3 ветки этого каскадного if-а вместо шести, да и строк стало поменьше

2) убран копипастенный код, и теперь, если захочешь внести изменения, их придётся вносить в одно место а не в несколько

3) код стал заметно нагляднее: видно, что на самом деле у нас есть два разрешённых варианта - «есть телефон» и «есть индекс без всего остального», при этом у варианта «есть телефон» есть два опциональных параметра - индекс и имя.

Ещё я бы рекомендовал заменить snprintf на что-то более нормальное, чтобы не писать каждый раз этот флуд со strlen-ами и не дублировать везде AT_CMD_BUFFER_SIZE. Чтобы выглядело примерно так:

buffer_struct = () {.size=AT_CMD_BUFFER_SIZE, .buf=gsmService->atCmdBuffer /* .len=0 можно не писать */ }; /* инициализация */
append_printf(&buffer_struct, "%i", index); /* добавление текста */

Можно наверно ещё и ветку «только индекс» немного объединить с остальным - начало AT-команды то везде одинаковое, можно его ещё до if-ов вывести, и индекс, если есть, тоже вывести там, а дальше только проверить корректность данных.

Исходная версия firkax, :

Как и ожидалось, общие места вполне есть. В начале уберём отдельные ветки по наличию/отсутствию name 

///   PREPARE AT CMD
	
	if( phonebookEntry->index < 0 && phonebookEntry->telNo){
		res = snprintf( gsmService->atCmdBuffer, AT_CMD_BUFFER_SIZE, "AT+CPBW=,\"%s\"", phonebookEntry->telNo );
		if( res < 0 ) return -1;
                if(phonebookEntry->name) {
			if( phonebookEntry->telNo[0] == '+' ) type = 145;
			else                                  type = 129;
			res = packUtf82Ucs2( phonebookEntry->name, gsmService->codecBuffer, CODEC_BUFFER_SIZE );
			if( res < 0 ) return -1;		
			res = snprintf(
				gsmService->atCmdBuffer+strlen(gsmService->atCmdBuffer),
				AT_CMD_BUFFER_SIZE-strlen(gsmService->atCmdBuffer),
				",%i,\"%s\"",
				type,
				gsmService->codecBuffer
			);
			if( res < 0 ) return -1;
                }
                res = snprintf(gsmService->atCmdBuffer+strlen(gsmService->atCmdBuffer), AT_CMD_BUFFER_SIZE-strlen(gsmService->atCmdBuffer), "\r");		
	}else if( phonebookEntry->index >= 0 && !phonebookEntry->telNo && !phonebookEntry->name ){
		
		//только индекс
		res = snprintf( gsmService->atCmdBuffer, AT_CMD_BUFFER_SIZE, "AT+CPBW=%i\r", phonebookEntry->index );
		
	}else if( phonebookEntry->index >= 0 && phonebookEntry->telNo){
		res = snprintf( gsmService->atCmdBuffer, AT_CMD_BUFFER_SIZE, "AT+CPBW=%i,\"%s\"", phonebookEntry->index, phonebookEntry->telNo );
		if( res < 0 ) return -1;
                if(phonebookEntry->name) {
			if( phonebookEntry->telNo[0] == '+' ) type = 145;
			else                                  type = 129;
			res = packUtf82Ucs2( phonebookEntry->name, gsmService->codecBuffer, CODEC_BUFFER_SIZE );
			if( res < 0 ) return -1;		
			res = snprintf(
				gsmService->atCmdBuffer+strlen(gsmService->atCmdBuffer),
				AT_CMD_BUFFER_SIZE-strlen(gsmService->atCmdBuffer),
				",%i,\"%s\"",
				type,
				gsmService->codecBuffer
			);
			if( res < 0 ) return -1;
                }
                res = snprintf(gsmService->atCmdBuffer+strlen(gsmService->atCmdBuffer), AT_CMD_BUFFER_SIZE-strlen(gsmService->atCmdBuffer), "\r");
	}else{
		//недопустимое сочетание
		return -1;
	}
	
	if( res < 0 ) return -1;
Видим что ветки с индексом и без индекса оказались почти одинаковые, объединяем их тоже 
///   PREPARE AT CMD
	if(phonebookEntry->telNo){
		res = snprintf( gsmService->atCmdBuffer, AT_CMD_BUFFER_SIZE, "AT+CPBW=");
		if( res < 0 ) return -1;
		if(phonebookEntry->index >= 0) {
			res = snprintf( gsmService->atCmdBuffer+strlen(gsmService->atCmdBuffer), AT_CMD_BUFFER_SIZE-strlen(gsmService->atCmdBuffer), "%i", phonebookEntry->index);
			if( res < 0 ) return -1;
		}
		res = snprintf( gsmService->atCmdBuffer+strlen(gsmService->atCmdBuffer), AT_CMD_BUFFER_SIZE-strlen(gsmService->atCmdBuffer), ",\"%s\"", phonebookEntry->telNo );
		if( res < 0 ) return -1;
                if(phonebookEntry->name) {
			if( phonebookEntry->telNo[0] == '+' ) type = 145;
			else                                  type = 129;
			res = packUtf82Ucs2( phonebookEntry->name, gsmService->codecBuffer, CODEC_BUFFER_SIZE );
			if( res < 0 ) return -1;		
			res = snprintf(
				gsmService->atCmdBuffer+strlen(gsmService->atCmdBuffer),
				AT_CMD_BUFFER_SIZE-strlen(gsmService->atCmdBuffer),
				",%i,\"%s\"",
				type,
				gsmService->codecBuffer
			);
			if( res < 0 ) return -1;
                }
                res = snprintf(gsmService->atCmdBuffer+strlen(gsmService->atCmdBuffer), AT_CMD_BUFFER_SIZE-strlen(gsmService->atCmdBuffer), "\r");		
	}else if( phonebookEntry->index >= 0 && !phonebookEntry->telNo && !phonebookEntry->name ){
		//только индекс
		res = snprintf( gsmService->atCmdBuffer, AT_CMD_BUFFER_SIZE, "AT+CPBW=%i\r", phonebookEntry->index );
	}else{
		//недопустимое сочетание
		return -1;
	}
	
	if( res < 0 ) return -1;
По-моему, получившийся код намного нагляднее (ещё бы имена переменных исправить...). Решены сразу несколько проблем:

1) осталось всего 3 ветки этого каскадного if-а вместо шести, да и строк стало поменьше

2) убран копипастенный код, и теперь, если захочешь внести изменения, их придётся вносить в одно место а не в несколько

3) код стал заметно нагляднее: видно, что на самом деле у нас есть два разрешённых варианта - «есть телефон» и «есть индекс без всего остального», при этом у варианта «есть телефон» есть два опциональных параметра - индекс и имя.

Ещё я бы рекомендовал заменить snprintf на что-то более нормальное, чтобы не писать каждый раз этот флуд со strlen-ами и не дублировать везде AT_CMD_BUFFER_SIZE. Чтобы выглядело примерно так:

buffer_struct = () {.size=AT_CMD_BUFFER_SIZE, .buf=gsmService->atCmdBuffer /* .len=0 можно не писать */ }; /* инициализация */
append_printf(&buffer_struct, "%i", index); /* добавление текста */