[P&AM Lab] генератор паролей

[Grigoriy A. Sitkarev]

Уже поздно. Завтра утром наверное прочтёте.

1. Давайте договоримся что комментарии мы будем стараться писать на 
английском языке, это полезно с точки зрения языковой практики, набора 
словарного запаса и пригодится в будущем -- не все программисты говорят 
на русском, но почти все понимают английский.

2. Хоть программа и не большая, даже здесь лучше бы было разбить её на 
ряд чётких, небольших функций. Каждая из которых выполняет одно действие 
и делает это хорошо. Хорошие программы состоят не из малого числа 
больших функций, а большого числа малых.

3. В K&R С внутри switch ключевые слова case не нужно отделять ещё раз 
табуляцией. Они относятся к тому же уровню вложенности.

switch (tag) {
case 0:
	...
	break;
case 1:
	...
	break;
	...
}

4. Если мы хотим вывести строку на терминал, не надо это делать посимвольно:

	for (i = 0; i < passlen; i++) {
		printf("%c", password[i]);
	}

Нужно было сначала терминировать нулём строку в password, и объявлять 
этот массив нужно было наверное так:

#define MAX_LENGTH	255

int main(int argc, char *argv[]) {
	...
	char password[MAX_LENGTH+1];
	...
	for (i = 0; i < passlen; i++) {
		tag = lrand48() % 3;
	...
	}

	//Выводим пароль на экран, чтобы было не так грустно//
	printf("Password: ");	

	password[passlen] = '\0';

	printf("%s", password);

Думаю что идея ясна.

5. Думаю что стоит читать полностью строку в буфер из stdin через 
fgets(3) а потом травить на неё strcasecmp(3) и проверять что там ввёл 
пользователь и устанавливать флаг. Другой вариант, переписать проще 
цикл, и не забыть про tolower(3).

6. Делать проверку повторно на confirm == 'Y' и confirm == 'y' не нужно. 
Для этого нужно было ранее сделать переменную-флаг и проверять её.

if (write_to_file) {
	...
}

7. Дальше, очень сложно сделано считывание путей. Наверно городить 
огород с динамическим выделением памяти для буферов не стоит, да и 
читать посимвольно и каждый раз увеличивать буфера на 1 байт тоже. Кроме 
того, очень сложные условия стоят в циклах, этого нужно стараться 
избегать. Они должны быть предельно простыми и идиоматичными. А уже 
внутри цикла проверяйте условия ещё раз и пользуйтесь оператором break.

for (pathlen = 0; (((c = getchar()) != EOF) &&
	(c != '\n') && (pathlen != PATH_MAX) ); pathlen++) {
	...
}

Проще бы было:

for (pathlen = 0; (c = getchar()) != EOF; pathlen++) {

	if (c == '\n')
		break;

	if (pathlen >= PATH_MAX-1)
		break;
	...
}

Обратите внимание что я проверяю PATH_MAX-1, потому что нужно ещё место 
для терминирующего нуля.

Про буфера я уже сказал, никаких malloc(3)/realloc(3) там всё же не 
нужно. Достаточно буферов статического размера. POSIX/SUSv2 гарантирует 
что имена файлов и путей вместятся в эти лимиты:

char filename[NAME_MAX+1];
char pathname[PATH];
char path[PATH];

У нас в рассылке раньше где-то было сообщение о том что это за лимиты и 
откуда они взялись.

И всё равно не понятно, почему отдельно просят файл а потом ещё и 
каталог? Если можно сразу попросить ввести путь. И в UNIX обычно так и 
делается. Если просят ввести путь к файлу, то туда можно вводить 
абсолютный и относительный пути (если не оговорено обратное).

Системный вызов open(2) вообще-то открывает путь как относительный так и 
абсолютный. Может быть в этом была загвоздка?

8. Читать, ещё раз повторюсь, посимвольно через getchar(3) наверное не 
стоит в тех случаях. Проще сделать это через fgets(3).

9. Принято в большинстве случаев сообщения на stderr и в лог-файлы 
отправлять без завершающей точки. Обратите внимание как пишет ядро на 
консоль и в логи, как ведут себя другие прикладные программы и утилиты. 
Отмечу что грамматически сообщения составлены корректно и ясно отражают 
смысл ошибок/событий.

Как итог, программу можно существенно упростить, она будет короче, 
понятнее и яснее. Есть над чем потрудиться. Когда я что-то ещё вспомню, 
напишу в рассылку. Очень приятно видеть что идёт процесс, приходит 
понимание и присутствует творчество. Нужно идти вперёд.

--
Г.А.

13.02.2011 22:32, Константин Никулов пишет:
> Прощу прощения, не тот файл сбросил. Вот в этом...