Прошу рецензію на C++ код

Підписуйтеся на Telegram-канал «DOU #tech», щоб не пропустити нові технічні статті.

<p>Прошу переглянути нищенаведений код, який повинен працювати на<br/>
чистих Windows XP та Vista.</p>
<p>Ця програма (Launcher) призначена для запуску іншої програми (Target app).<br/>
Рішення про запуск приймається на основі наявності файлу autostart.<br/>
Якщо виникають проблеми при запуску — повідомити про це користувача та<br/>
записати в лог файл.</p>
<p>Launcher не повинен відкривати будь-яких власних вікон включаючи консольне, <br/>
UAC, compatibility або Windows Resource Protection, тощо.</p>
<p>Окреме питання про те, чи потрібна підтримка Unicode і як її реалізувати тоді? </p>


#define WIN32_LEAN_AND_MEAN // Exclude rarely-used stuff from Windows headers
#define STRICT

#include <windows.h>
#include <stdio.h>

#define LAUNCHER_NAME "GigatronLauncher"
#define AUTOSTART_FILEPATH "%APPDATA%\\Gigatron\\autostart"
#define TARGET_NAME	"Gigatron"
#define TARGET_PARAMETER "autostart"

inline bool AutoStartEnabled();
bool LaunchTargetApplication();
int HandleLaunchResult( const bool );

// Entry point of launcher program
int APIENTRY WinMain( HINSTANCE, HINSTANCE, LPSTR, int ){

bool launchSucceed = true;
	try{

const bool AUTOSTART_ENABLED = AutoStartEnabled();

if( AUTOSTART_ENABLED ){
			launchSucceed = LaunchTargetApplication();
		}
		else{
			// do nothing
		}

}
	catch(...){
		launchSucceed = false;
		// CloseHandle( pi.hProcess ); CloseHandle( pi.hThread );
	}

HandleLaunchResult( launchSucceed );

}

/* Answers whether autostart of target application is enabled. */
bool AutoStartEnabled(){

// Expand autostart file path
	LPSTR	expandedPath = new CHAR[ MAX_PATH ];
	// sprintf_s( autostartFilePath, MAX_PATH, "%APPDATA%\\%s\\%s", TARGET_NAME, AUTOSTART_FILENAME );
	ExpandEnvironmentStrings( ( LPSTR )AUTOSTART_FILEPATH, expandedPath, MAX_PATH );

// Check if autostart file exists
	OFSTRUCT openFileStructure = {0};
	const HFILE FILE_HANDLE = OpenFile( expandedPath, &openFileStructure, OF_EXIST );
	if( HFILE_ERROR != FILE_HANDLE ){
		return true;
	}
	else{
		return false;
	}

}

/* Launches target application and returns true if succeed, false if failed. */
bool LaunchTargetApplication(){

// Define command line string
	LPSTR commandLine = new CHAR[ MAX_PATH ];
	sprintf_s( commandLine, MAX_PATH, "%s.exe %s", TARGET_NAME, TARGET_PARAMETER );//_tcscat();

// Prepare process creation data
	PROCESS_INFORMATION processInfo = {0};
	ZeroMemory( &processInfo, sizeof( processInfo ) );
	STARTUPINFO startupInfo = {0};
	ZeroMemory( &startupInfo, sizeof( startupInfo ) );
	startupInfo.cb = sizeof( startupInfo );

// Start target application
	const BOOL PROCESS_RESULT = CreateProcess( NULL, commandLine, NULL, NULL, FALSE, 0, NULL,
		NULL, &startupInfo, &processInfo );

return ( 0 != PROCESS_RESULT );

}

/* Handles target application launch result. */
int HandleLaunchResult( const bool in_LaunchSucceed ){

const int LAST_ERROR_CODE = GetLastError();

if( in_LaunchSucceed ){
		return 0;
	}
	else{

// Log system error message to file
		HANDLE logFileHandle;
		try{

// Get last error message
			LPSTR systemErrorMessage;
			FormatMessage( FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
				NULL, LAST_ERROR_CODE, 0, ( LPTSTR )&systemErrorMessage, 0, NULL );

// Create log file and write mesage into it
			const LPSTR envPath = new CHAR[ MAX_PATH ];
			sprintf_s( envPath, MAX_PATH, "%APPDATA%\\%s\\%s.log", TARGET_NAME, LAUNCHER_NAME );
			const LPSTR logFilePath = new CHAR[ MAX_PATH ];
			ExpandEnvironmentStrings( envPath, logFilePath, MAX_PATH );

logFileHandle = CreateFile( logFilePath, GENERIC_WRITE, FILE_SHARE_READ, NULL, CREATE_ALWAYS,
				FILE_ATTRIBUTE_NORMAL, NULL );
			if( INVALID_HANDLE_VALUE != logFileHandle ){

DWORD numberOfBytesWritten;
				WriteFile( logFileHandle, systemErrorMessage, ( DWORD )strlen( systemErrorMessage ),
					&numberOfBytesWritten, NULL );

}
			else{
				// do nothing
			}

}
		catch(...){
			// do nothing
		}

if( INVALID_HANDLE_VALUE != logFileHandle ){
			CloseHandle( logFileHandle );
		}
		else{
			// do nothing
		}

// Display user message
		LPSTR userMessage = new CHAR[ MAX_PATH ];
		sprintf_s( userMessage, MAX_PATH, "Problem starting %s.", TARGET_NAME );
		MessageBox( NULL, userMessage, TARGET_NAME, MB_ICONERROR ); // TODO: add tech support button or adress.

return LAST_ERROR_CODE;

}

}
👍ПодобаєтьсяСподобалось0
До обраногоВ обраному0
LinkedIn
Дозволені теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter
Дозволені теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter
Це якийсь жах; не так код, як коменти.
Світ перевернувся!
Дотнети стл і проча муйня для такої гігантської системи.
Чи це народ так тепер стібається?!
Ось що сказав би Торвальдс про цю гілку
http://thread.gmane.org/gmane.comp.version-control.git/57643/focus=57918
Максим, викинь усе що стосується С++ (catch, new) і в жодному разі не читай про smart pointers, RAII і т.д.
Кодай на джаві і буде тобі щастя!
PS. Дивно що воно в тебе взагалі компілюється.
sprintf_s (envPath, MAX_PATH, "%APPDATA%\\%s\\%s.log», TARGET_NAME, LAUNCHER_NAME);
поміняй хоча б на
sprintf_s (envPath, MAX_PATH, "%%APPDATA%%\\%s\\%s.log», TARGET_NAME, LAUNCHER_NAME);

PPS. Насправді мені С++ дуже подобається, інша справа, що він тут ніяким боком.

Нормально. Це питання стилю. Ти б скоротив OpenFile до Open?

Так, інаписав би з маленької букви. приблизно як тут: linux.die.net/man/2/open
А взагалі якщо джавіст хоче з C++ познайомитись то я б радив перш за все звернути увагу на такі теми та поняття
1. Розміщення об’єктів в пам’яті (стек, купа, статичні об’єкти)
2. час життя об’єктів. деструктори
3. той самий RTTI, розкрутка стека

4. перегрузка ->, розумні вказівники


Если нет throw, то в catch нет никакого смысла.

Вообще то оператор new может генерировать ексепшин std: bad_alloc;

1) де саме пам’ять тече?
2) Smart Pointers, Smart Handles — це занадто складно, для мене:)
3) Може й краще, але не знаю чим і не знаю STL.
І не хочу тягнути ніяких DLL разом із ланчером.
4) GetLastError () по суті одразу й визивається.
1. Там где операторы new должен быть оператор delete когда выделенная память становится не нужной;
2. В принципе если прога маленькая то можна пережить;
3. STL не тянет дополнительных DLL — это шаблонные классы; (похоже на generics в.NET, про яву не знаю есть ли там такое);
4. В принципе да, но легко забыть и нечаянно перекрыть другой API функцией.

P.S. Ну и процедурный стиль программирования тоже напрягает.; -)


Дякую, я взагалі Java розробник.

ага... тогда всё понятно

зло? щось я не дуже розумію. Чому?

Матчасть: www.gotw.ca/...ations/xc .htm gotw.ca/...tions/mxc .htm

Не могли б обгрунтувати чи бодай дати посилання де це обгрунтовується?
по-перше цей код нічого не кидає там одні API функцї вони С++ ексепшенів не кидають взагалі,
по-друге погано лапати все підряд тобто проблема в трьохкрапках

по-третє як і сказав Terminator в С++ для управління ресурсами використовують RAII, а не try — catch.

Если нет throw, то в catch нет никакого смысла.


catch (...) — це зло, яке описане у всіх підручниках

зло? щось я не дуже розумію. Чому? Не могли б обгрунтувати чи бодай дати посилання де це обгрунтовується?

Стосовно стилю: це не С++, а enhanced C, на мій погляд, оскільки використовується угорська нотація та #define (це перше, що кидається у вічі)
catch (...) — це зло, яке описане у всіх підручниках. особливо підозрілим виглядає те, що catch (...) присутнє також у DEBUG build

якби це було написано на С++, використовувалось би RAII, а так — це грамотний С код з використанням С++ розширень

Дякую, я взагалі Java розробник.
C++ та Win32 API вже 4 роки не бачив.
До shadow
1) де саме пам’ять тече?
2) Smart Pointers, Smart Handles — це занадто складно, для мене:)
3) Може й краще, але не знаю чим і не знаю STL.
І не хочу тягнути ніяких DLL разом із ланчером.
4) GetLastError () по суті одразу й визивається.
До clewer_one
1) Нормально. Це питання стилю. Ти б скоротив OpenFile до Open?
2, 3) Строки це взагалі темна сторона сили, на разі:)
4) Це десктоп програма. Не потрібно сервісів і telnet’у.
Я протестував її трохи і при відсутності цільвої програми ""

вилятає C Run-Time Error R6002: floating-point support not loaded

1. занадто довгі імена функцій та змінних, задовбаєшся ти такими іменами оперувати
До прикладу LaunchTargetApplication я б скоротив до Launch

2. згоден про ліки

LPSTR commandLine = new CHAR[ MAX_PATH ];

Нащо ти взагалі то на купі виділяєш?
3. А от нарахунок строк з stl шановний shadow не до кінця правий, я б казав в купі з WinAPI CString юзати, а не std: string. Але певно не руками бавитись з буферами чарів
4. Захардкоджене ім’я программи.
Хочеш доречі розвинемо тему? Напиши сервіс до якого б ішов конфіг що складається з записів виду
< Ключ>,< повний шлях до бінарки>,< параметри>
сервіс має слухати певний TCP порт (його теж в конфіг винеси)
Клієнт встанолює TCP з’єднання з серсером і посилає йому ключ, якщо сервер знаходить такий ключ в своєму конфізі — запускає відповіну бінарку з відповідними параметрами, предає їй з’єднання разом з клієнтом на обслуговування і далі слухає собі свій порт.
В якості готового клієнта можеш використати telnet. В якості прикладу програми що запускаэться — ехо — програму що повторює те що отримує.

Цікаво?

1. Утечки памяти;
2. Работа напрямую с хендлами и указателями -, а нужно пользоватся обертками Smart Pointers, Smart Handles;
3. Строки лучше брать из STL, чем работать напрямую с CHAR/TCHAR;

4. GetLastError (); нужно делать сразу после вызовов API функций и писать в лог, а не в каких то левых местах;

Підписатись на коментарі