11
\$\begingroup\$

This is, kind of, my first programming project. It's a small project to complete first year's university programming course.

The program allows the user to record his mouse/keyboard/cursor activities and then replay it. It's a mini-bot.

I am unsure whether I will continue developing this project, or abandon C and go learn some OOP or web. Either way, I would really love to know what kind of mistakes I made.

Particularly: do you see something that hurts your eyes, some bad practices, terrible naming, some unreadable code?


Short video demonstration: https://streamable.com/7qcb3

Project's code: https://github.com/Wenox/WinAuto

The menu.c file was written in a rush, so you're likely to find the ugliest code in there. I am mostly interested about menu.c, smooth_cursor.c, replay.c, recording.c files.


I've got a small review and this code is vulnerable:

printf("Save recording as (i.e: myrecording.txt):\n");
char file_name[64];
scanf("%s", file_name);

(I will probably replace scanf with fgets combined with sscanf).

Other than that, now that I am looking at my code I probably could have used typedef on the struct. Heard that it's a bad practice, though.

I am not sure if I should remove the large, ugly comments from the .h files or not.


The program is launched from main like this:

int main(int argc, char **argv)
{
    struct f_queue *headptr = NULL;
    struct f_queue *tailptr = NULL;

    if (!h_switch_invoked(argc, argv))
        init_menu(headptr, tailptr, 0, 0);
    else
        init_menu(headptr, tailptr, 7, 0);

    return 0;
}

Here is menu.c file that I am particularly interested in. I've written it in a rush and never wrote "menu" before. So I came up with an idea to make it recursive, with helping enum, and not sure how good or bad idea that was:

bool h_switch_invoked(int argc, char **argv)
{
    if (argc > 1)
        if (0 == strcmp(argv[1], "-h"))
            return true;

    return false;
}

/** Enum containing various menu flags used to determine which <b>printf</b> should be displayed to the user, based on earlier program behaviour. */
enum menu_flags {               ///< start of definition
    NO_ERRORS,                  ///< default
    ERROR_NO_TXT_SUFFIX,        ///< when user forgot to input the .txt postfix
    ERROR_READING_FILE,         ///< when file was corrupted, does not exist or cannot be opened
    SAVED_HOTKEY,               ///< when the hotkey has been successfully saved
    SAVED_FILE,                 ///< when the file saved successfully
    STOPPED_PLAYBACK,           ///< when the recording playback successfully ended
    STOPPED_SCREENSAVER,        ///< when the screensaver has been successfully stopped
    HELP_SWITCH                 ///< when program was ran with '-h' switch
};

void draw_menu(const int flag_id)
{
    system("cls");

    switch (flag_id) {
        case 0:
            printf("WinAuto\n");
            break;
        case 1:
            printf("ERROR: File name must end with .txt suffix\n\n");
            break;
        case 2:
            printf("ERROR: No such file or file is corrupted\n\n");
            break;
        case 3:
            printf("Hotkey set successfully\n\n");
            break;
        case 4:
            printf("Recording saved successfully\n\n");
            break;
        case 5:
            printf("Playback finished or interrupted\n\n");
            break;
        case 6:
            printf("Welcome back\n\n");
            break;
        case 7:
            print_help();
            break;
        default: // do nothing
            break;
    }

    printf("Press 1 to set global hotkey (DEFAULT HOTKEY: F5)\n");
    printf("Press 2 to create new recording\n");
    printf("Press 3 to play recording\n");
    printf("Press 4 to start screensaver\n");
    printf("Press 5 to exit\n");
}

int get_menu_choice(void)
{
    int choice = 0;

    while (choice < 1 || choice > 5)
        if (1 != scanf("%d", &choice))
            fseek(stdin, 0, SEEK_END);

    return choice;
}

int get_hotkey(void)
{
    printf("Set hotkey: \n");
    int hotkey = 0;

    while (hotkey == 0 ||
           hotkey == KEY_RETURN ||
           hotkey == KEY_LMB ||
           hotkey == KEY_RMB ||
           hotkey == KEY_F5) {
                hotkey = get_keystroke();
           }

    FlushConsoleInputBuffer(GetStdHandle(STD_INPUT_HANDLE));
    return hotkey;
}

bool str_ends_with(const char *source, const char *suffix)
{
    int source_len = strlen(source);
    int suffix_len = strlen(suffix);

    return (source_len >= suffix_len) && (0 == strcmp(source + (source_len - suffix_len), suffix));
}

int get_cycles_num(void)
{
    printf("How many playing cycles? (>5 to play infinitely, default 1):\n");
    int cycles_num = 1;

    if (1 != scanf("%d", &cycles_num) || cycles_num <= 0) {
        fseek(stdin, 0, SEEK_END);
        get_cycles_num();
    }

    return cycles_num;
}

void exec_play_recording(struct f_queue *head, struct f_queue *tail, const int cycles_num, const int hotkey_id)
{
    printf("Playing recording...\n");
    printf("Press your hotkey to stop\n");

    if (cycles_num > 5) {
        make_queue_cyclic(head, tail);
        play_recording(tail, hotkey_id);
        unmake_queue_cyclic(head, tail);
    }
    else {
        for (int i = 0; i < cycles_num; i++)
            play_recording(tail, hotkey_id);
    }
}

void init_menu(struct f_queue *head, struct f_queue *tail, const int flag_id, const int hotkey_id);

void chosen_recording(struct f_queue *head, struct f_queue *tail, const int hotkey_id)
{
    printf("Save recording as (i.e: myrecording.txt):\n");
    char file_name[64];
    scanf("%s", file_name);

    if (str_ends_with(file_name, ".txt")) {
        record(&head, &tail, 10, hotkey_id);
        FlushConsoleInputBuffer(GetStdHandle(STD_INPUT_HANDLE));
        trim_list(&head);
        save_recording(tail, file_name);
        free_recording(&head, &tail);
        init_menu(head, tail, SAVED_FILE, hotkey_id);
    }
    else {
        init_menu(head, tail, ERROR_NO_TXT_SUFFIX, hotkey_id);
    }
}

void chosen_playback(struct f_queue *head, struct f_queue *tail, const int hotkey_id)
{
    printf("Type in file name of your recording (i.e: myfile.txt):\n");
    char file_name[64];
    scanf("%s", file_name);

    if (load_recording(&head, &tail, file_name)) {
        int cycles_num = get_cycles_num();
        exec_play_recording(head, tail, cycles_num, hotkey_id);
        FlushConsoleInputBuffer(GetStdHandle(STD_INPUT_HANDLE));
        free_recording(&head, &tail);
        init_menu(head, tail, STOPPED_PLAYBACK, hotkey_id);
    }
    else { // error when reading file
        if (tail)
            free_recording(&head, &tail);
        init_menu(head, tail, ERROR_READING_FILE, hotkey_id);
    }
}

void init_menu(struct f_queue *head, struct f_queue *tail, const int flag_id, const int hotkey_id)
{
    draw_menu(flag_id);

    int choice = get_menu_choice();
    static int hotkey = KEY_F5; /// default hotkey

    switch(choice) {
        case 1:
            hotkey = get_hotkey();
            init_menu(head, tail, SAVED_HOTKEY, hotkey);
            break;
        case 2:
            chosen_recording(head, tail, hotkey);
            break;
        case 3:
            chosen_playback(head, tail, hotkey);
            break;
        case 4:
            exec_screen_saver(hotkey);
            init_menu(head, tail, STOPPED_SCREENSAVER, hotkey);
            break;
        case 5:
            return;
        default: // do nothing
            break;
    }
}

Also, here's how an exemplary .h header file looks like. menu.h (note the large doxy comments that I am unsure whether should be kept or removed):

/** @file */

#ifndef MENU_H_INCLUDED
#define MENU_H_INCLUDED

/** The function outputs relevant text data to the user. The function helps the user navigate around the program.
@param flag_id menu flag to determine expected printf result based on earlier behaviour */
void draw_menu(const int flag_id);

/** The function prompts user to select menu choice to futher navigate around the program. Basic input validation is performed. */
int get_menu_choice(void);

/** The function saves user-inputted keystroke as a hotkey used in <b>2nd, 3rd and 4th</b> menu functions.
@warning User needs to remember his hotkey.
@warning For user's convenience, several hotkeys that would propably not me sense were blacklisted, including the default hotkey. */
int get_hotkey(void);

/** The function verifies if string (array of chars) ends with given suffix (other array of chars).
Used to validate if the file inputted by the user surely ends with .txt postfix.
@param source pointer to source array
@param suffix pointer to desired ending suffix of soruce array
@return <b>true</b> if source ends with suffix
@return <b>false</b> otherwise
@warning The function comes from stackoverflow.com */
bool str_ends_with(const char *source, const char *suffix);

/** The function prompts user to input how many cycles of recording he wishes to playback.
The input number has to be an integer greater or equal than 1, and if the input is greater than 5, then it is assumed the playback is infinitely loop.
<b>In such case the f_queue doubly linked list-queue attains cyclic properties.</b>
@return cycles_num the desired number of cycles */
int get_cycles_num(void);

/** The function executes the process of simulation of playing the recording.
In case if cycles number is greater than 5, the playback loop is infinite.
The playback loop ends at the end of all cycles, or <b>can be broken by pressing the set (or default if not set) hotkey</b>.
@param head pointer to the front of the <b>f_queue</b> list-queue
@param tail pointer to the last node of the <b>f_queue</b> list-queue
@param cycles_num the number of playback cycles
@param hotkey_id the turn-off playback key switch */
void exec_play_recording(struct f_queue *head, struct f_queue *tail, const int cycles_num, const int hotkey_id);

/** The function executes entire recording process when user chose <b>2</b>.
Recording is stopped when <b>hotkey</b> is pressed and saved into the inputted .txt file.
Hence it can be re-used afterwards for playback purposes.
The function <b>recurseively</b> goes back to the menu with appropriate <b>menu_flags</b>: SAVED_FILE or ERROR_NO_TXT_SUFFIX,
depending on the earlier behaviour.
@param head pointer to the front node of the <b>f_queue</b> linked list
@param tail pointer to the last node of the <b>f_queue</b< linked list
@param hotkey_id */
void chosen_recording(struct f_queue *head, struct f_queue *tail, const int hotkey_id);

/** Recursive function that loops the menu and loops the execution of the program.
The user chooses if he wants to set new hotkey, create new recording, playback old recording, start screensaver or end the program.
@param head pointer to the front node of <b>f_queue</b> doubly-linked list
@param tail pointer to the last node of <b>f_queue</b> doubly-linked list
@param flag_id the menu flag, depending on the value different output is displayed to the user
@param hotkey_id the turn-off switch for the program (default <b>F5</b>) */
void init_menu(struct f_queue *head, struct f_queue *tail, const int flag_id, const int hotkey_id);

/** Function prints detailed manual to the user if -h flag was invoked. */
void print_help();

/** Function checks the command line input switches. If -h switch is found, detailed manual is printed out to the user.*/
bool h_switch_invoked(int argc, char **argv);
#endif // MENU_H_INCLUDED

Here's recording "engine" from recording.c:

#define _GETCURSOR 1
#define _GETKEY 2
#define _SLEEP 3

void add_cursor(struct f_queue **head, struct f_queue **tail, POINT P[2])
{
    P[1] = get_cursor();

    if (P[0].x != P[1].x || P[0].y != P[1].y) {               ///< if current cursor pos != previous
        add_function(head, tail, _GETCURSOR, P[1].x, P[1].y); ///< add it to the queue
        P[0] = P[1];
    }
}

void add_keystroke(struct f_queue **head, struct f_queue **tail, int key_buff[2])
{
    key_buff[1] = get_keystroke();

    if (key_buff[1] != key_buff[0] && key_buff[1] != 0)     ///< if there was keystroke
        add_function(head, tail, _GETKEY, key_buff[1], -1); ///< add it to the queue

    key_buff[0] = key_buff[1];
}

bool is_prev_sleep_func(struct f_queue **head)
{
    return (*head)->f_type == _SLEEP;
}

void add_sleep(struct f_queue **head, struct f_queue **tail, const int sleep_dur)
{
    Sleep(sleep_dur);

    if (!is_prev_sleep_func(head))
        add_function(head, tail, _SLEEP, sleep_dur, -1);
    else
        (*head)->f_args[0] += sleep_dur; ///< increment the previous node, rather than add new one
}

void record(struct f_queue **head, struct f_queue **tail, const int sleep_dur, const int hotkey_id)
{
    int key_buff[2] = {-1, -1};                     ///< buffer for curr and prev pressed key
    POINT cursor_buff[2] = {{-1, -1}, {-1, -1}};    ///< buffer for curr and prev cursor position

    printf("RECORDING...\n[press your hotkey to stop]\n");
    while(key_buff[1] != hotkey_id) {             ///< stop recording when 'hotkey' is pressed
        add_cursor(head, tail, cursor_buff);
        add_keystroke(head, tail, key_buff);
        add_sleep(head, tail, sleep_dur);
    }
}

and replay "engine" from replay.c:

bool is_mouse_event(const int KEY_CODE)
{
    return KEY_CODE <= 2;
}

void send_mouse_input(const int KEY_CODE)
{
    INPUT ip = {0};
    ip.type = INPUT_MOUSE;

    switch(KEY_CODE) {
        case 1:
                ip.mi.dwFlags = MOUSEEVENTF_LEFTDOWN | MOUSEEVENTF_LEFTUP;
                break;
        case 2:
                ip.mi.dwFlags = MOUSEEVENTF_RIGHTDOWN | MOUSEEVENTF_RIGHTUP;
                break;
        default:
                return;
    }

    SendInput(1, &ip, sizeof(INPUT));
}

void send_keyboard_input(const int KEY_CODE)
{
    INPUT ip = {0};
    ip.type = INPUT_KEYBOARD;

    ip.ki.wVk = KEY_CODE;
    SendInput(1, &ip, sizeof(INPUT));   // press

    ip.ki.dwFlags = KEYEVENTF_KEYUP;
    SendInput(1, &ip, sizeof(INPUT));   // release
}

void send_input(const int KEY_CODE)
{
    if (is_mouse_event(KEY_CODE))
        send_mouse_input(KEY_CODE);
    else
        send_keyboard_input(KEY_CODE);
}

void play_recording(struct f_queue *tail, const int hotkey_id)
{
    while (tail) {
        if (check_key(hotkey_id))
            return;

        if (tail->f_type == _GETCURSOR)
            SetCursorPos(tail->f_args[0], tail->f_args[1]);     ///< Simulates cursor's position
        else if (tail->f_type == _GETKEY)
            send_input(tail->f_args[0]);                        ///< Simulates keystroke
        else if (tail->f_type == _SLEEP)
            Sleep(tail->f_args[0]);                             ///< Simulates waiting interval in between keystrokes and/or cursor's movements

        tail = tail->prev;
    }
}

Also, `functio

I am in need of all kind of criticism. Thanks.

\$\endgroup\$
  • 3
    \$\begingroup\$ Unfortunately your question is off-topic as of now, as the code to be reviewed must be present in the question. Code behind links is considered non-reviewable. Please add the code you want reviewed in your question. Thanks! \$\endgroup\$ – Edward Aug 13 at 18:25
  • 1
    \$\begingroup\$ You should post also the headers, which contain very important information. \$\endgroup\$ – Cacahuete Frito Aug 16 at 17:13
  • 1
    \$\begingroup\$ The lack of attention is probably due to the lack of code. You only posted some fragments of the code, which aren't enough for a good review. \$\endgroup\$ – Cacahuete Frito Aug 16 at 19:39
  • 1
    \$\begingroup\$ How about using puts instead of printf if you're not doing formatted output? \$\endgroup\$ – JL2210 Aug 16 at 22:21
  • 2
    \$\begingroup\$ I noticed that Cacahuete asked for the headers but as JL2210 commented, please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Aug 16 at 22:47
4
+100
\$\begingroup\$

Reserved identifiers

Identifiers starting with a single underscore followed by a capital letter are reserved by the Standard. You can't create any new name of that form at all in your code. (As you didn't post any headers I can't know, but I guess things like _GETCURSOR are yours, and not from some library).

C17::7.1.3:

7.1.3 Reserved identifiers

1 Each header declares or defines all identifiers listed in its associated subclause, and optionally declares or defines identifiers listed in its associated future library directions subclause and identifiers which are always reserved either for any use or for use as file scope identifiers.

— All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use, except those identifiers which are lexically identical to keywords.187)

— All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces.

— Each macro name in any of the following subclauses (including the future library directions) is reserved for use as specified if any of its associated headers is included; unless explicitly stated otherwise (see 7.1.4).

— All identifiers with external linkage in any of the following subclauses (including the future library directions) and errno are always reserved for use as identifiers with external linkage.188)

— Each identifier with file scope listed in any of the following subclauses (including the future library directions) is reserved for use as a macro name and as an identifier with file scope in the same name space if any of its associated headers is included.

So you should maybe name it GETCURSOR or GET_CURSOR or GETCURSOR_ or GETCURSOR__.


stderr

Error messages should be printed to stderr instead of stdout (which is where printf() prints). To do that, one uses fprintf(stderr, "...", ...);.


curses

Maybe you would like some very nice menus instead of just printing lines on the screen like messages. The curses libraries do that. There are various options you can use (all are more or less compatible, at least on the basics): pdcurses and ncurses are the two I've used, and they are relatively easy to learn (the basics at least).

As a bonus, curses is compatible with POSIX, so your program will not only run on Windows.

\$\endgroup\$

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.