5
\$\begingroup\$

This is my first program ever made in C. Before that I programmed just a little bit in C++ (didn't touch OOP so I did only structural programming). This is my second "project" ever made, the first "project" was even bigger joke. I am new to programming.

The project is nowhere near to be finished. However, it somehow works, and I don't want to add new features until this mess is cleaned up. I feel like for me the bigger the mess is, the harder it is to find small mistakes.

That's why I come here: would you please be able to take a look at my code and tell me what's wrong with it? Especially program structure-wise.

Program features:

  • after execution immedietely starts recording cursor's movements and cursor/keyboard keystrokes
  • if 'W' is pressed the recording stops
  • and 5 seconds later the recording is played-back, simulating what has been recorded

Basically during the recording phase it snapshots the keyboard and mouse state every ~10 miliseconds.

Few things to mention:

  • doubly linked list is not freed yet. I will handle the memory leaks.
  • the program does not and probably will not support simulating multiple keypresses at the same time
  • I will make it more user-friendly and such after I fix this mess

Here is link to the github containing all the code: https://github.com/Wenox/WinAuto

For example, I wonder if such function:

POINT get_cursor(void)
{
    POINT P = {};
    GetCursorPos(&P);
    return P;
}

deserves its own input_cursor.c file?

The main thing I am curious about is the recording.c file containing:

#include ..bunch of files..

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

/** adds cursor's position to the functions queue */
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];
    }
}

/** adds latest keystroke's description to the functions queue */
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); // then add it to the queue
    key_buff[0] = key_buff[1];
}

/** returns true if newly added node is function of sleep type, false otherwise */
bool is_prev_sleep_func(struct f_queue **head)
{
    return (*head)->f_type == _SLEEP;
}

/** adds sleep to functions queue */
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 node rather than add new one
}

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

    while(key_buff[1] != KEY_W) {             // stop recording when '3' is pressed
        add_cursor(head, tail, cursor_buff);
        add_keystroke(head, tail, key_buff);
        add_sleep(head, tail, sleep_dur);
    }
}

and the play_recording() function from replay.c file:

/** function replays the recording */
void play_recording(struct f_queue *tail)
{
    while (tail) {
        if (tail->f_type == _GETCURSOR)
            SetCursorPos(tail->f_args[0], tail->f_args[1]);
        else if (tail->f_type == _GETKEY)
            send_input(tail->f_args[0]);
        else if (tail->f_type == _SLEEP)
            Sleep(tail->f_args[0]);

        tail = tail->prev;
    }
}

if it looks neat/readable/understandable to a person who reads it for the first time, and if something is clearly wrong/could be improved/is a bad practice.

For all suggestions/advices on any kind of improvements I am gladly thankful. Many thanks.

\$\endgroup\$
  • 3
    \$\begingroup\$ Welcome to Code Review. Your question looks already good. It will be perfect if you add the code of the project in the question itself, instead of only linking to GitHub. Since your program consists of multiple files, it's ok if you include an "interesting subset" of the code. For those who want to checkout the code the GitHub link provides access to the rest. \$\endgroup\$ – Roland Illig May 3 at 19:48
  • \$\begingroup\$ @RolandIllig: Thank you, I have added more code. \$\endgroup\$ – weno May 3 at 20:00
  • \$\begingroup\$ I feel like you could avoided a lot of pain by just using C++? Why needlessly use C when it has no language support for linked-lists and other QOL? \$\endgroup\$ – Rishav May 3 at 22:52
  • \$\begingroup\$ @Rishav: It's a project for my 1st year university programming course. We've been told it has to be done in C. (although all kind of C libraries, portability or lack of portability, etc. allowed) \$\endgroup\$ – weno May 3 at 23:00
3
\$\begingroup\$

I took a look at your github and I think your code is broken up a bit too much. I think a few files can be consolidated. For instance, consider consolidating the files for; input_cursor.c, pressed_key.c into one file called "user_input.c" and do the same for the .h files.

I would also avoid making files named after reserved words in C. like "structures.h". Putting all of your structures in one place is fine if they all serve the same purpose, but name the file after the thing's purpose, not the thing.

I see you're not including std.io until the testing functions are declared in the file. This won't affect performance or compile time. It only distracts the programmer. so regardless of the purpose of the include or which functions will use it, put them all at the top. Now, if you're planning on deleting your test code after release then you really should put all your test functions in their own files in a different directory labeled "test" and define a makefile that can take in a test flag so that it knows to compile the test objects together with the rest of your code.

I also saw a variable declared in a c-code file called keys_pqueue[25] = {}, this is a configuration, and should go in a header file. leave the c-code files for functional code and the headers for all your different kinds of configurations and namespace management.

That's the only feedback I have for now. Your code looks very well thought-out. And C/C++ are hard languages to begin coding with. So it's all very well done!

\$\endgroup\$
2
\$\begingroup\$

Initial position

add_cursor() only adds the cursor's position to the function's queue if there is a change.

Depending on needs, I'd expect the initial mouse position to be added too and not assume it is {0,0}. Presently, the true initial mouse position is never saved.

Suggest:

POINT cursor_buff[2] = { get_cursor(), { 0,0}};
add_function(head, tail, _GETCURSOR, cursor_buff[0].x, cursor_buff[0].y);

while(key_buff[1] != KEY_W) {
  ....

Backwards?

play_recording() looks like tail = tail->prev; is playing back in reverse order. I'd expect re-play to start at the beginning and play use ->next.

Need to see add_function().

\$\endgroup\$
  • \$\begingroup\$ Thank you for feedback. add_cursor() does indeed add cursor if and only if cursor has moved. With an exception for initial cursor position: it is added. I have recorded a short video to demonstrate that: streamable.com/1roos Problem is there was a small bug you helped me discover: if cursor initial possition is (0, 0), then the program would crash. I have fixed it by initializing POINT array with -1 instead of 0. \$\endgroup\$ – weno May 8 at 12:03
  • \$\begingroup\$ Not sure about play_recording() part. I am intentionally adding at the start of the queue and then playing from the end-to-start. I guess because it is a doubly linked list I can also quickly keep adding to the tail instead of start, and then play from the start. Yeah makes it more clear I guess. \$\endgroup\$ – weno May 8 at 12:06
  • \$\begingroup\$ Real question is what should I choose: doubly linked list (more memory because two pointers for each node) or singly linked list (less memory, but requires calling some reverse_list() function). With singly linked list it could work like that: keep adding to the start, then reverse_list(), then play from the start. \$\endgroup\$ – weno May 8 at 12:06
  • \$\begingroup\$ @weno Use a single linked list. Carefully detail why you think you need a double linked list. Chances are, in the end a single linked one will do. A single linked list can perform all these in O(1) time: Add_to_front(), Add_to_end(), Take_from_front(), report_front(), report_end(), Walk_list__forward_step(). \$\endgroup\$ – chux - Reinstate Monica May 8 at 23:34

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.