From 3935ebcbb49304cd87568234be2ecf504806193b Mon Sep 17 00:00:00 2001 From: Vasily Date: Sat, 22 Dec 2018 12:20:28 +0100 Subject: [PATCH 1/2] Refactor command line argument parsing into function The command line argument parsing is done in a giant for loop in the main function. This presents readability and extension problems. Refactor the big for loop into smaller loops inside the function. In theory this will affect performance slightly at startup, but when considering the very low amount of possible command line arguments (5, currently) and the low amount of arguments being passed on a typical launch (0 or 1, mostly), the readability improvements are very much worth it. The string literals have also been moved to the "argument" namespace as constexpr, in order to avoid potential typos in string literals. Should the amount of command line arguments increase severely, it might be necessary to create a structure with only one pass through the argument loop, but such a situation is very unlikely. --- src/main.cpp | 125 +++++++++++++++++++++++++++++---------------------- 1 file changed, 71 insertions(+), 54 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 646d59f..1d47db1 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -110,59 +110,76 @@ public: }; -int main(int argc, char *argv[]) { - - bool desktopMode = false; - bool noSound = false; - bool noManifest = false; - // Parse command line arguments - for (int i = 1; i < argc; i++) { - if (std::string(argv[i]).compare("-desktop") == 0) { - desktopMode = true; - } else if (std::string(argv[i]).compare("-nosound") == 0) { - noSound = true; - } else if (std::string(argv[i]).compare("-nomanifest") == 0) { - noManifest = true; - } else if (std::string(argv[i]).compare("-installmanifest") == 0) { - int exitcode = 0; - QCoreApplication coreApp(argc, argv); - auto initError = vr::VRInitError_None; - vr::VR_Init(&initError, vr::VRApplication_Utility); - if (initError == vr::VRInitError_None) { - try { - installManifest(true); - } catch (std::exception& e) { - exitcode = -1; - std::cerr << e.what() << std::endl; - } - } else { - exitcode = -2; - std::cerr << std::string("Failed to initialize OpenVR: " + std::string(vr::VR_GetVRInitErrorAsEnglishDescription(initError))) << std::endl; +namespace argument { + // Checks whether a specific string was passed as a launch argument. + bool CheckCommandLineArgument(int argc, char *argv[], const std::string parameter) { + // The old way was one giant loop that tested every single parameter. + // Having an individual loop for all args most likely has a very small performance + // penalty compared to the original solution, + // but the gains in readability and ease of extension of not having a hundred line+ for loop are worth it. + for (int i = 0; i < argc; i++) { + if (std::string(argv[i]) == parameter) { + return true; + } + } + return false; + } + + constexpr auto DESKTOP_MODE = "-desktop"; + constexpr auto NO_SOUND = "-nosound"; + constexpr auto NO_MANIFEST = "-nomanifest"; + constexpr auto INSTALL_MANIFEST= "-installmanifest"; + constexpr auto REMOVE_MANIFEST= "-removemanifest"; +} + +int main(int argc, char *argv[]) { + + const bool desktop_mode = argument::CheckCommandLineArgument(argc, argv, argument::DESKTOP_MODE); + const bool no_sound = argument::CheckCommandLineArgument(argc, argv, argument::NO_SOUND); + const bool no_manifest = argument::CheckCommandLineArgument(argc, argv, argument::NO_MANIFEST); + const bool install_manifest = argument::CheckCommandLineArgument(argc, argv, argument::INSTALL_MANIFEST); + const bool remove_manifest = argument::CheckCommandLineArgument(argc, argv, argument::REMOVE_MANIFEST); + + if (install_manifest) { + int exitcode = 0; + QCoreApplication coreApp(argc, argv); + auto initError = vr::VRInitError_None; + vr::VR_Init(&initError, vr::VRApplication_Utility); + if (initError == vr::VRInitError_None) { + try { + installManifest(true); + } catch (std::exception& e) { + exitcode = -1; + std::cerr << e.what() << std::endl; } - vr::VR_Shutdown(); - exit(exitcode); - } else if (std::string(argv[i]).compare("-removemanifest") == 0) { - int exitcode = 0; - QCoreApplication coreApp(argc, argv); - auto initError = vr::VRInitError_None; - vr::VR_Init(&initError, vr::VRApplication_Utility); - if (initError == vr::VRInitError_None) { - try { - removeManifest(); - } catch (std::exception& e) { - exitcode = -1; - std::cerr << e.what() << std::endl; - } - } else { - exitcode = -2; - std::cerr << std::string("Failed to initialize OpenVR: " + std::string(vr::VR_GetVRInitErrorAsEnglishDescription(initError))) << std::endl; - } - vr::VR_Shutdown(); - exit(exitcode); + } else { + exitcode = -2; + std::cerr << std::string("Failed to initialize OpenVR: " + std::string(vr::VR_GetVRInitErrorAsEnglishDescription(initError))) << std::endl; } + vr::VR_Shutdown(); + exit(exitcode); + } else if (remove_manifest) { + int exitcode = 0; + QCoreApplication coreApp(argc, argv); + auto initError = vr::VRInitError_None; + vr::VR_Init(&initError, vr::VRApplication_Utility); + if (initError == vr::VRInitError_None) { + try { + removeManifest(); + } catch (std::exception& e) { + exitcode = -1; + std::cerr << e.what() << std::endl; + } + } else { + exitcode = -2; + std::cerr << std::string("Failed to initialize OpenVR: " + std::string(vr::VR_GetVRInitErrorAsEnglishDescription(initError))) << std::endl; + } + vr::VR_Shutdown(); + exit(exitcode); } + try { MyQApplication a(argc, argv); a.setOrganizationName("matzman666"); @@ -195,13 +212,13 @@ int main(int argc, char *argv[]) { LOG(INFO) << "Log File: " << logFilePath; } - if (desktopMode) { + if (desktop_mode) { LOG(INFO) << "Desktop mode enabled."; } - if (noSound) { + if (no_sound) { LOG(INFO) << "Sound effects disabled."; } - if (noManifest) { + if (no_manifest) { LOG(INFO) << "vrmanifest disabled."; } @@ -211,7 +228,7 @@ int main(int argc, char *argv[]) { QQmlEngine qmlEngine; - advsettings::OverlayController* controller = advsettings::OverlayController::createInstance(desktopMode, noSound); + advsettings::OverlayController* controller = advsettings::OverlayController::createInstance(desktop_mode, no_sound); controller->Init(&qmlEngine); QQmlComponent component(&qmlEngine, QUrl::fromLocalFile("res/qml/mainwidget.qml")); @@ -222,7 +239,7 @@ int main(int argc, char *argv[]) { auto quickObj = component.create(); controller->SetWidget(qobject_cast(quickObj), advsettings::OverlayController::applicationName, advsettings::OverlayController::applicationKey); - if (!desktopMode && !noManifest) { + if (!desktop_mode && !no_manifest) { try { installManifest(); } catch (std::exception& e) { @@ -230,7 +247,7 @@ int main(int argc, char *argv[]) { } } - if (desktopMode) { + if (desktop_mode) { auto m_pWindow = new QQuickWindow(); qobject_cast(quickObj)->setParentItem(m_pWindow->contentItem()); m_pWindow->setGeometry(0, 0, qobject_cast(quickObj)->width(), qobject_cast(quickObj)->height()); From 921d1ae828c38135f671426c75271ba84cf036f9 Mon Sep 17 00:00:00 2001 From: Vasily Date: Sat, 22 Dec 2018 14:45:49 +0100 Subject: [PATCH 2/2] Refactor manifest remove/install logic in main The install/remove manifest logic in main is currently duplicated. This bloats up main and creates unnecessarily large indented logic. The only thing that is different between the two large if blocks is a single function call. Refactoring to test for which function to call at the latest possible stage reduces code duplication. --- src/main.cpp | 74 +++++++++++++++++++++++++--------------------------- 1 file changed, 35 insertions(+), 39 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 1d47db1..922bcc6 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -133,6 +133,12 @@ namespace argument { constexpr auto REMOVE_MANIFEST= "-removemanifest"; } +enum ReturnErrorCode { + SUCCESS = 0, + GENERAL_FAILURE = -1, + OPENVR_INIT_ERROR = -2, +}; + int main(int argc, char *argv[]) { const bool desktop_mode = argument::CheckCommandLineArgument(argc, argv, argument::DESKTOP_MODE); @@ -141,44 +147,34 @@ int main(int argc, char *argv[]) { const bool install_manifest = argument::CheckCommandLineArgument(argc, argv, argument::INSTALL_MANIFEST); const bool remove_manifest = argument::CheckCommandLineArgument(argc, argv, argument::REMOVE_MANIFEST); - if (install_manifest) { - int exitcode = 0; - QCoreApplication coreApp(argc, argv); - auto initError = vr::VRInitError_None; - vr::VR_Init(&initError, vr::VRApplication_Utility); - if (initError == vr::VRInitError_None) { - try { - installManifest(true); - } catch (std::exception& e) { - exitcode = -1; - std::cerr << e.what() << std::endl; - } - } else { - exitcode = -2; - std::cerr << std::string("Failed to initialize OpenVR: " + std::string(vr::VR_GetVRInitErrorAsEnglishDescription(initError))) << std::endl; - } - vr::VR_Shutdown(); - exit(exitcode); - } else if (remove_manifest) { - int exitcode = 0; - QCoreApplication coreApp(argc, argv); - auto initError = vr::VRInitError_None; - vr::VR_Init(&initError, vr::VRApplication_Utility); - if (initError == vr::VRInitError_None) { - try { - removeManifest(); - } catch (std::exception& e) { - exitcode = -1; - std::cerr << e.what() << std::endl; - } - } else { - exitcode = -2; - std::cerr << std::string("Failed to initialize OpenVR: " + std::string(vr::VR_GetVRInitErrorAsEnglishDescription(initError))) << std::endl; - } - vr::VR_Shutdown(); - exit(exitcode); - } + if (install_manifest || remove_manifest) { + int exit_code = ReturnErrorCode::SUCCESS; + QCoreApplication coreApp(argc, argv); + auto openvr_init_status = vr::VRInitError_None; + vr::VR_Init(&openvr_init_status, vr::VRApplication_Utility); + if (openvr_init_status == vr::VRInitError_None) { + try { + if (install_manifest) { + installManifest(true); + } + else if (remove_manifest) { + removeManifest(); + } + } + catch (std::exception& e) { + exit_code = ReturnErrorCode::GENERAL_FAILURE; + std::cerr << e.what() << std::endl; + } + } + else { + exit_code = ReturnErrorCode::OPENVR_INIT_ERROR; + std::cerr << std::string("Failed to initialize OpenVR: " + std::string(vr::VR_GetVRInitErrorAsEnglishDescription(openvr_init_status))) << std::endl; + } + + vr::VR_Shutdown(); + exit(exit_code); + } try { MyQApplication a(argc, argv); @@ -258,7 +254,7 @@ int main(int argc, char *argv[]) { } catch (const std::exception& e) { LOG(FATAL) << e.what(); - return -1; + return ReturnErrorCode::GENERAL_FAILURE; } - return 0; + return ReturnErrorCode::SUCCESS; }