6
\$\begingroup\$

After getting a CR from @pacmaninbw and @ALX23z here, I want to share my new code, and to ask for better ways (which always exist) to improve the code, even with new libraries. The only thing that important to me, is the way of receiving the parameters have to be the command line [I am using Linux OS, so it's highly common to use command line params].

So, to separate the main to smaller functions, alongside avoiding messy functions' parameters handling, I created a class to handle the whole initialize part of the cmd params:


Edit:

cmd_options.h

#ifndef COMPUTERMONITORINGSTATISTICSPARSER_CMD_OPTIONS_H
#define COMPUTERMONITORINGSTATISTICSPARSER_CMD_OPTIONS_H

#include <iostream>
#include <boost/program_options.hpp>

struct cmd_options_data {
    explicit cmd_options_data(const std::string &options_description) :
            visible_options(options_description) {}

    bool help = false;                      // Show help message
    bool verbose = false;                   // Display login/logout details
    bool anomaly_detection = false;         // Show anomalies details if found
    bool analyze_activity = true;           // Analyze login/logout total/summarize times
    std::string week_start_day;
    std::string log_file_path;
    std::string normal_login_word;
    boost::program_options::options_description visible_options;
    boost::program_options::variables_map variables_map;
};

class cmd_options {
public:
    explicit cmd_options(int ac, char* av[]);

    cmd_options_data get_data();

private:
    boost::program_options::options_description init_cmd_po_generic_options();

    boost::program_options::options_description init_cmd_po_calender_options();

    boost::program_options::options_description init_cmd_po_logger_options();

    boost::program_options::options_description init_cmd_po_hidden_options();

    boost::program_options::options_description init_cmd_po_mode_options();

    boost::program_options::positional_options_description init_cmd_positional_options();

    boost::program_options::options_description group_cmd_options() {
        return boost::program_options::options_description();
    }

    template<class... Args>
    boost::program_options::options_description group_cmd_options(const boost::program_options::options_description &option, Args&... options);

    void apply_program_options(int ac, char* av[]);

    void update_flags();

    cmd_options_data _options_data;
    boost::program_options::options_description full_options;
    boost::program_options::positional_options_description positional_options;
};

template<class... Args>
boost::program_options::options_description cmd_options::group_cmd_options(const boost::program_options::options_description &option, Args&... options) {
    boost::program_options::options_description group;
    group.add(option);
    group.add(group_cmd_options(options...));
    return group;
}

#endif //COMPUTERMONITORINGSTATISTICSPARSER_CMD_OPTIONS_H

cmd_options.cpp

#include "cmd_options.h"

namespace boost_cmd_po = boost::program_options;

cmd_options::cmd_options(int ac, char* av[]) : _options_data("Usage: program [options] [path/]logger_filename") {

    auto generic_options = init_cmd_po_generic_options();
    auto calender_options = init_cmd_po_calender_options();
    auto logger_options = init_cmd_po_logger_options();
    auto mode_options = init_cmd_po_mode_options();
    auto hidden_options = init_cmd_po_hidden_options();

    _options_data.visible_options.add(
            group_cmd_options(
                    generic_options,
                    calender_options,
                    logger_options,
                    mode_options
            )
    );

    full_options.add(
            group_cmd_options(
                    generic_options,
                    calender_options,
                    logger_options,
                    mode_options,
                    hidden_options
            )
    );
    positional_options = init_cmd_positional_options();

    apply_program_options(ac, av);

    update_flags();
}

boost_cmd_po::options_description cmd_options::init_cmd_po_generic_options() {
    auto group = boost_cmd_po::options_description("Generic options");
    group.add_options()
            ("help,h", "produce help message")
            //("verbose", boost_cmd_po::value<bool>(&_options_data.verbose)->default_value(false), "Show detailed times of login.");
            ("verbose", "Show detailed times of login.");
    return group;
}

boost_cmd_po::options_description cmd_options::init_cmd_po_calender_options() {
    auto group = boost_cmd_po::options_description("Calender options");
    group.add_options()
            ("week-start-day,d", boost_cmd_po::value<std::string>(&_options_data.week_start_day)->default_value("Monday"), "Week starting day ('--week-start-day help' for a list).");
    return group;
}

boost_cmd_po::options_description cmd_options::init_cmd_po_logger_options() {
    auto group = boost_cmd_po::options_description("Logger options");
    group.add_options();
    return group;
}

boost_cmd_po::options_description cmd_options::init_cmd_po_hidden_options() {
    auto group = boost_cmd_po::options_description("Logger options");
    group.add_options()
            ("log-path,l", boost_cmd_po::value<std::string>(&_options_data.log_file_path)->default_value( "/home/sherlock/message_from_computer"), "Path to login/logout logger.");
    return group;
}

boost_cmd_po::options_description cmd_options::init_cmd_po_mode_options() {
    auto group = boost_cmd_po::options_description("Mode options");
    group.add_options()
            //("analyze-log", boost_cmd_po::value<bool>(&_options_data.analyze_activity)->default_value(true), "Analyze activity - show activity times and summarise activity.")
            ("no-analyze", "Disable activity analyzing - don't show activity times/summarise.")
            //("anomaly-detection", boost_cmd_po::value<bool>(&_options_data.anomaly_detection)->default_value(false), "Check for anomalies in logger.")
            ("anomaly-detection", "Check for anomalies in logger.")
            ("normal-login-word", boost_cmd_po::value<std::string>(&_options_data.normal_login_word)->default_value("login"), "For anomaly detector- word that should symbol a login line in login/logout logger (after '+' sign).");
    return group;
}

boost_cmd_po::positional_options_description cmd_options::init_cmd_positional_options() {
    boost_cmd_po::positional_options_description pd;
    pd.add("log-path", -1);
    return pd;
}

void cmd_options::apply_program_options(int ac, char **av) {
    boost_cmd_po::store(
            boost_cmd_po::command_line_parser(ac, av)
                    .options(full_options)
                    .positional(positional_options)
                    .run(), _options_data.variables_map);
    boost_cmd_po::notify(_options_data.variables_map);
}

void cmd_options::update_flags() {
    _options_data.help              = (bool) _options_data.variables_map.count("help");
    _options_data.verbose           = (bool) _options_data.variables_map.count("verbose");
    _options_data.analyze_activity  = !(bool) _options_data.variables_map.count("no-analyze");
    _options_data.anomaly_detection = (bool) _options_data.variables_map.count("anomaly-detection");
}

cmd_options_data cmd_options::get_data() {
    return _options_data;
}

main.cpp

#include <iostream>
#include <boost/filesystem.hpp>
#include <boost/date_time.hpp>
#include "core/day.h"
#include "core/log_handler.h"
#include "utilities/design_text.h"
#include "cmd_options.h"

int main(int ac, char* av[]) {
    cmd_options command_line_options(ac, av);
    cmd_options_data cmd_data = command_line_options.get_data();

    /// --help / -h option handler
    if (cmd_data.help) {
        std::cout << cmd_data.visible_options << "\n";
        return EXIT_SUCCESS;
    }

    /// --log-path / -l option handler
    if (!boost::filesystem::exists(cmd_data.log_file_path))
        throw std::runtime_error("Log file path doesn't exist.");

    /// --week-start-day / -d option handler
    /// Initialize available days list
    auto available_days = std::vector<day>{{"sunday", boost::date_time::weekdays::Sunday},
                           {"monday", boost::date_time::weekdays::Monday},
                           {"tuesday", boost::date_time::weekdays::Tuesday},
                           {"wednesday", boost::date_time::weekdays::Wednesday},
                           {"thursday", boost::date_time::weekdays::Thursday},
                           {"friday", boost::date_time::weekdays::Friday},
                           {"saturday", boost::date_time::weekdays::Saturday}};
    if (auto selected_day = std::find(available_days.begin(), available_days.end(), boost::to_lower_copy(cmd_data.week_start_day)); selected_day != available_days.end()) { // Selected day exists
        log_handler::week_start_day = selected_day->day_symbol;
    } else { // Selected day doesn't exists
        if (cmd_data.week_start_day == "help") { // Produce help days message
            std::cout << "Available days:" << std::endl;
            std::cout << "\tSun [Sunday]" << std::endl;
            std::cout << "\tMon [Monday]" << std::endl;
            std::cout << "\tTue [Tuesday]" << std::endl;
            std::cout << "\tWed [Wednesday]" << std::endl;
            std::cout << "\tThu [Thursday]" << std::endl;
            std::cout << "\tFri [Friday]" << std::endl;
            std::cout << "\tSat [Saturday]" << std::endl;
            return EXIT_SUCCESS;
        }
        throw std::runtime_error("Unfamiliar day, for options list use '-d [ --week-start-day ] help'.");
    }

    // Anomalies detector
    auto anomaly_detected = log_handler::anomalies_detector(cmd_data.log_file_path, cmd_data.normal_login_word, cmd_data.anomaly_detection);
    if (cmd_data.analyze_activity) // Analyze logger times
        log_handler::analyze(cmd_data.log_file_path, cmd_data.verbose);
    if (anomaly_detected) // Produce anomalies warning if needed
        std::cout << "\n\n" << design_text::make_colored(std::stringstream() << "*** Anomaly detected! ***", design_text::Color::NONE, design_text::Color::RED, true) << std::endl;

    return EXIT_SUCCESS;
}

Update:

After @pacmaninbw review, new updated post: program options from command line initialize [v3 - after CR]

\$\endgroup\$
  • 2
    \$\begingroup\$ This is definitely an improvement, main()'s dependency on boost::program_options has been reduced, the implementation can be changed without altering the code in main(). I suggest that you add code to main that tests cmd_options and cmd_options_data. Show how they are used within the program, and use the 3 flags that have been created. Test with and without verbose, with and without anomaly_detection and with and without analyze_activity. After this test with 2 of each of the options and then finally test with all 3 options at once. \$\endgroup\$ – pacmaninbw Oct 15 '19 at 0:35
  • \$\begingroup\$ @pacmaninbw Thanks! I added the full main function with the params process. After a check, all the flags are working correctly. \$\endgroup\$ – KorelK Oct 15 '19 at 1:30
  • \$\begingroup\$ Wait tow or three days before going another round. Get more of your application ready. Post more of the program next time. \$\endgroup\$ – pacmaninbw Oct 16 '19 at 3:16
  • \$\begingroup\$ @pacmaninbw thanks! I would like to continue the CR of the updated version: codereview.stackexchange.com/questions/230930/… \$\endgroup\$ – KorelK Oct 18 '19 at 2:03
1
\$\begingroup\$

First, thank you for providing the link to your GitHub repository, it allowed a more complete review.

I've noticed a real tendency in the code to avoid creating classes and to use procedural programming rather than object oriented programming. Namespaces are used instead of creating classes. The use of classes and objects can be very powerful, for one thing it allows inheritance and polymorphism. The use of classes can also decouple modules and reduce dependencies, right now the modules are strongly coupled and this has a tendency to prevent necessary changes to the architecture as the program matures and grows.

I've also noticed a rather strong tendency to use auto rather than declaring the proper types. While the auto type is very useful in some cases such as ranged for loops maintaining this code can be more difficult. Personally types help me to understand the code better. I would almost say this code is abusing the use of auto.

Avoid Using Namespace std

One or more of the source files in the core directory and the utilities directory still contain the using namespace std; statement.

Complexity

Once again the function main() is too complex (does too much). As programs grow in size the use of main() should be limited to calling functions that parse the command line, calling functions that set up for processing, calling functions that execute the desired function of the program, and calling functions to clean up after the main portion of the program.

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

This code should probably a function be in day.cpp and the function prototype should be in day.h:

    auto available_days = std::vector<day>{{"sunday", boost::date_time::weekdays::Sunday},
                                           {"monday", boost::date_time::weekdays::Monday},
                                           {"tuesday", boost::date_time::weekdays::Tuesday},
                                           {"wednesday", boost::date_time::weekdays::Wednesday},
                                           {"thursday", boost::date_time::weekdays::Thursday},
                                           {"friday", boost::date_time::weekdays::Friday},
                                           {"saturday", boost::date_time::weekdays::Saturday}};

The function should return a type of std::vector<day>;

Or perhaps the function should perform the ensuing search for the day and return the day itself.

    auto selected_day = get_selected_day_of_the_week()

Try Catch Throw Blocks

The code in main() currently contains a throw exception but there is no try{} catch{} code to catch the exception, this will result in the program terminating without reporting the problem. At best in the debugger it will report unhandled exception. The main() code should contain a try block and a catch block to handle any exceptions, the throw statement should probably be called in one of the sub functions that main() calls. If this code stays in main() it might be better to change the throw to std::cerr << "MESSAGE" << std::endl.

Prefer \n Over std::endl;

For performance reasons \n is preferred over std::endl, especially in loops where more than one std::cout is expected. std::endl calls a system routine to flush the output buffer. Calling a system function means that the program will be swapped out while the system function is executing.

        if (cmd_data.week_start_day == "help") { // Produce help days message
            std::cout << "Available days:" << std::endl;
            std::cout << "\tSun [Sunday]" << std::endl;
            std::cout << "\tMon [Monday]" << std::endl;
            std::cout << "\tTue [Tuesday]" << std::endl;
            std::cout << "\tWed [Wednesday]" << std::endl;
            std::cout << "\tThu [Thursday]" << std::endl;
            std::cout << "\tFri [Friday]" << std::endl;
            std::cout << "\tSat [Saturday]" << std::endl;
            return EXIT_SUCCESS;
        }

was refactored to

        if (cmd_data.week_start_day == "help") { // Produce help days message
            std::cout << "Available days:\n";
            std::cout << "\tSun [Sunday]\n";
            std::cout << "\tMon [Monday]\n";
            std::cout << "\tTue [Tuesday]\n";
            std::cout << "\tWed [Wednesday]\n";
            std::cout << "\tThu [Thursday]\n";
            std::cout << "\tFri [Friday]\n";
            std::cout << "\tSat [Saturday]" << std::endl;
            return EXIT_SUCCESS;
        }

to flush all the output at the end.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot! Do you think it'll be fine if we go to another round, or just publish here my current state? \$\endgroup\$ – KorelK Oct 15 '19 at 23:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.