5
\$\begingroup\$

I am learning the xlib library and I made a xmenu utility in order to practice it.

It receives as input a menu specification where each line is a menu entry.
Each line can be indented with tabs to represent nested menus.
Each line is made of a label and a command separated by tab.
When you select an entry in the menu, the corresponding command is output to the stdout.
It becomes clearer by trying it.

Here is a sample input (the site may convert tabs to spaces, you have to know that lines are indented by tabs and labels and commands are separated by tabs):

Label A         Command 1
Label B         Command 2
Label C         Command 3
    sub A       Command 4
    sub B       Command 5
        subsubA Command 6
        subsubB Command 7
    sub C       Command 8
Label D         Command 9
    sub D       Command 10
        subsubC Command 11
Label E         Command 12
Label F         Command 13

By using this as stdin, the program will draw a menu with 6 entries (Label A to Label F), where Label C and Label D contains submenus.

The previous input will generate the following menu stack: menus

Here is the code:

#include <err.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <X11/Xlib.h>
#include <X11/Xutil.h>

/* macros */
#define LEN(x) (sizeof (x) / sizeof (x[0]))
#define MAX(x,y) ((x)>(y)?(x):(y))
#define MIN(x,y) ((x)<(y)?(x):(y))

#define FONT            "-*-*-medium-*-*-*-14-*-*-*-*-*-*-*"
#define UNPRESSEDBG     "#222222"
#define UNPRESSEDFG     "#cccccc"
#define PRESSEDBG       "#ffffff"
#define PRESSEDFG       "#000000"
#define DECORATIONBG    "#bbbbbb"
#define DECORATIONFG    "#000000"
#define ITEMW           130
#define ITEMB           5
#define BORDER          2

/* color enum */
enum {ColorFG, ColorBG, ColorLast};

/* draw context structure */
struct DC {
    unsigned long unpressed[ColorLast];
    unsigned long pressed[ColorLast];
    unsigned long decoration[ColorLast];

    Drawable d;
    GC gc;
    XFontStruct *font;
    int fonth;
};

/* menu geometry structure */
struct Geometry {
    int itemb;      /* item border */
    int itemw;      /* item width */
    int itemh;      /* item height */
    int border;     /* window border */
};

/* screen geometry structure */
struct ScreenGeom {
    int cursx, cursy;       /* cursor position */
    int screenw, screenh;   /* screen width and height */
};

/* menu item structure */
struct Item {
    char *label;
    char *output;
    int x, y;
    struct Item *next;
    struct Menu *submenu;
};

/* menu structure */
struct Menu {
    struct Menu *parent;
    struct Item *list;
    struct Item *selected;
    int x, y, w, h;
    unsigned level;
    unsigned nitems;
    Window win;
};

/* function declarations */
static unsigned long getcolor(const char *s);
static void setupdc(void);
static void setupgeom(void);
static void setupgrab(void);
static struct Item *allocitem(size_t count, const char *label, const char *output);
static struct Menu *allocmenu(struct Menu *parent, struct Item *list, unsigned level);
static void getmenuitem(Window win, int x, int y,
                        struct Menu **menu_ret, struct Item **item_ret);
static void printmenu(size_t level, struct Menu *menu);
static void drawmenu(void);
static void calcscreengeom(void);
static void calcmenu(struct Menu *menu);
static void setcurrmenu(struct Menu *menu);
static void parsestdin(void);
static void run(void);
static void cleanupexit(void);
static void usage(void);

/* X variables */
static Colormap colormap;
static Display *dpy;
static Window rootwin;
static int screen;
static struct DC dc;

/* menu variables */
static struct Menu *rootmenu = NULL;
static struct Menu *currmenu = NULL;

/* geometry variables */
static struct Geometry geom;
static struct ScreenGeom sgeom;

/* flag variables */
static Bool override_redirect = True;

int
main(int argc, char *argv[])
{
    int ch;

    while ((ch = getopt(argc, argv, "w")) != -1) {
        switch (ch) {
        case 'w':
            override_redirect = False;
            break;
        default:
            usage();
            break;
        }
    }
    argc -= optind;
    argv += optind;

    /* open connection to server and set X variables */
    if ((dpy = XOpenDisplay(NULL)) == NULL)
        errx(1, "cannot open display");
    screen = DefaultScreen(dpy);
    rootwin = RootWindow(dpy, screen);
    colormap = DefaultColormap(dpy, screen);

    /* setup */
    setupdc();
    setupgeom();
    setupgrab();

    /* generate menus and recalculate them */
    parsestdin();
    if (rootmenu == NULL)
        errx(1, "no menu generated");
    calcscreengeom();
    calcmenu(rootmenu);
    setcurrmenu(rootmenu);

    /* debug */
    //printmenu(0, rootmenu);

    /* run event loop */
    run();

    return 1;   /* UNREACHABLE */
}

/* get color from color string */
static unsigned long
getcolor(const char *s)
{
    XColor color;

    if(!XAllocNamedColor(dpy, colormap, s, &color, &color))
        errx(1, "cannot allocate color: %s", s);
    return color.pixel;
}

/* init draw context */
static void
setupdc(void)
{
    /* get color pixels */
    dc.unpressed[ColorBG] = getcolor(UNPRESSEDBG);
    dc.unpressed[ColorFG] = getcolor(UNPRESSEDFG);
    dc.pressed[ColorBG] = getcolor(PRESSEDBG);
    dc.pressed[ColorFG] = getcolor(PRESSEDFG);
    dc.decoration[ColorBG] = getcolor(DECORATIONBG);
    dc.decoration[ColorFG] = getcolor(DECORATIONFG);

    /* try to get font */
    if ((dc.font = XLoadQueryFont(dpy, FONT)) == NULL)
        errx(1, "cannot load font");
    dc.fonth = dc.font->ascent + dc.font->descent;

    /* create GC and set its font */
    dc.gc = XCreateGC(dpy, rootwin, 0, NULL);
    XSetFont(dpy, dc.gc, dc.font->fid);
}

/* init menu geometry values */
static void
setupgeom(void)
{
    geom.itemb = ITEMB;
    geom.itemh = dc.fonth + ITEMB * 2;
    geom.itemw = ITEMW;
    geom.border = BORDER;
}

/* grab pointer */
static void
setupgrab(void)
{
    XGrabPointer(dpy, rootwin, True, ButtonPressMask, GrabModeAsync,
                 GrabModeAsync, None, None, CurrentTime);
}

/* allocate an item */
static struct Item *
allocitem(size_t count, const char *label, const char *output)
{
    struct Item *p;

    if ((p = malloc(sizeof *p)) == NULL)
        err(1, "malloc");
    if ((p->label = strdup(label)) == NULL)
        err(1, "strdup");
    if ((p->output = strdup(output)) == NULL)
        err(1, "strdup");
    p->x = 0;
    p->y = count * geom.itemh;
    p->next = NULL;
    p->submenu = NULL;

    return p;
}

/* allocate a menu */
static struct Menu *
allocmenu(struct Menu *parent, struct Item *list, unsigned level)
{
    XSetWindowAttributes swa;
    struct Menu *p;

    if ((p = malloc(sizeof *p)) == NULL)
        err(1, "malloc");
    p->parent = parent;
    p->list = list;
    p->selected = NULL;
    p->x = 0;
    p->y = 0;
    p->w = geom.itemw;
    p->h = geom.itemh;
    p->level = level;
    p->nitems = 0;

    swa.override_redirect = override_redirect;
    swa.background_pixel = dc.decoration[ColorBG];
    swa.border_pixel = dc.decoration[ColorFG];
    swa.event_mask = ExposureMask | KeyPressMask | ButtonPressMask | ButtonReleaseMask
                   | PointerMotionMask;
    p->win = XCreateWindow(dpy, rootwin, 0, 0, geom.itemw, geom.itemh, geom.border,
                           CopyFromParent, CopyFromParent, CopyFromParent,
                           CWOverrideRedirect | CWBackPixel | CWBorderPixel | CWEventMask,
                           &swa);

    return p;
}

/* create menus and items from the stdin */
static void
parsestdin(void)
{
    char *s, buf[BUFSIZ];
    char *label, *output;
    unsigned level = 0;
    unsigned i;
    struct Item *item, *p;
    struct Menu *menu;
    struct Menu *prevmenu = NULL;
    size_t count = 0;   /* number of items in the current menu */

    while (fgets(buf, BUFSIZ, stdin) != NULL) {
        level = 0;
        s = buf;

        while (*s == '\t') {
            level++;
            s++;
        }

        label = output = s;

        while (*s != '\0' && *s != '\t' && *s != '\n')
            s++;

        while (*s == '\t')
            *s++ = '\0';

        if (*s != '\0' && *s != '\n')
            output = s;

        while (*s != '\0' && *s != '\n')
            s++;

        if (*s == '\n')
            *s = '\0';

        item = allocitem(count, label, output);

        if (prevmenu == NULL) { /* there is no menu yet */
             menu = allocmenu(NULL, item, level);
             rootmenu = menu;
             prevmenu = menu;
             count = 1;
        } else if (level < prevmenu->level) { /* item is continuation of previous menu*/
            for (menu = prevmenu, i = level;
                  menu != NULL && i < prevmenu->level;
                  menu = menu->parent, i++)
                ;

            if (menu == NULL)
                errx(1, "reached NULL menu");

            for (p = menu->list; p->next != NULL; p = p->next)
                ;

            p->next = item;
            prevmenu = menu;
        } else if (level == prevmenu->level) {
            for (p = prevmenu->list; p->next != NULL; p = p->next)
                ;
            p->next = item;
        } else if (level > prevmenu->level) {
            menu = allocmenu(prevmenu, item, level);

            for (p = prevmenu->list; p->next != NULL; p = p->next)
                ;

            p->submenu = menu;

            prevmenu = menu;
        }
    }
}

/* calculate screen geometry */
static void
calcscreengeom(void)
{
    Window w1, w2;  /* unused variables */
    int a, b;       /* unused variables */
    unsigned mask;  /* unused variable */

    XQueryPointer(dpy, rootwin, &w1, &w2, &sgeom.cursx, &sgeom.cursy, &a, &b, &mask);
    sgeom.screenw = DisplayWidth(dpy, screen);
    sgeom.screenh = DisplayHeight(dpy, screen);
}

/* recursivelly calculate height and position of the menus */
static void
calcmenu(struct Menu *menu)
{
    XWindowChanges changes;
    struct Item *item, *p;
    size_t i;

    /* calculate number of items */
    i = 0;
    for (item = menu->list; item != NULL; item = item->next)
        i++;
    menu->nitems = i;
    menu->h = geom.itemh * i;

    /* calculate menu's x and y positions */
    if (menu->parent == NULL) { /* if root menu, calculate in respect to cursor */
        if (sgeom.screenw - sgeom.cursx >= menu->w)
            menu->x = sgeom.cursx;
        else if (sgeom.cursx > menu->w)
            menu->x = sgeom.cursx - menu->w;

        if (sgeom.screenh - sgeom.cursy >= menu->h)
            menu->y = sgeom.cursy;
        else if (sgeom.screenh > menu->h)
            menu->y = sgeom.screenh - menu->h;
    } else {                    /* else, calculate in respect to parent menu */

        /* search for the item in parent menu that generates this menu */
        for (p = menu->parent->list; p->submenu != menu; p = p->next)
            ;

        if (sgeom.screenw - (menu->parent->x + menu->parent->w) >= menu->w)
            menu->x = menu->parent->x + menu->parent->w;
        else if (menu->parent->x > menu->w)
            menu->x = menu->parent->x - menu->w;

        if (sgeom.screenh - p->y > menu->h)
            menu->y = p->y;
        else if (sgeom.screenh - menu->parent->y > menu->h)
            menu->y = menu->parent->y;
        else if (sgeom.screenh > menu->h)
            menu->y = sgeom.screenh - menu->h;
    }

    /* calculate position of each item in the menu */
    for (i = 0, item = menu->list; item != NULL; item = item->next, i++) {
        item->x = menu->x;
        item->y = menu->y + i * geom.itemh;
    }

    /* update menu geometry */
    changes.height = menu->h;
    changes.x = menu->x;
    changes.y = menu->y;
    XConfigureWindow(dpy, menu->win, CWHeight | CWX | CWY, &changes);

    for (item = menu->list; item != NULL; item = item->next) {
        if (item->submenu != NULL)
            calcmenu(item->submenu);
    }
}

/* print menus */
static void
printmenu(size_t level, struct Menu *menu)
{
    struct Item *item;
    size_t i;

    for (item = menu->list; item != NULL; item = item->next) {
        for (i = 0; i < level; i++)
            putchar('\t');
        printf("%u:%s: %s\n", menu->nitems, item->label, item->output);
        if (item->submenu != NULL)
            printmenu(level+1, item->submenu);
    }
}

/* get menu and item of given window and position */
static void
getmenuitem(Window win, int x, int y,
            struct Menu **menu_ret, struct Item **item_ret)
{
    struct Menu *menu = NULL;
    struct Item *item = NULL;

    for (menu = currmenu; menu != NULL; menu = menu->parent) {
        if (menu->win == win) {
            for (item = menu->list; item != NULL; item = item->next) {
                if (x >= item->x && x <= item->x + geom.itemw &&
                    y >= item->y && y <= item->y + geom.itemh) {
                    *menu_ret = menu;
                    *item_ret = item;
                    return;
                }
            }
        }
    }
}

/* set currentmenu to menu, umap previous menus and map current menu and its parents */
static void
setcurrmenu(struct Menu *menu)
{
    struct Menu *p;

    for (p = currmenu; p != NULL; p = p->parent)
        XUnmapWindow(dpy, p->win);

    currmenu = menu;

    for (p = currmenu; p != NULL; p = p->parent)
        XMapWindow(dpy, p->win);
}

/* draw items of the current menu and of its ancestors */
static void
drawmenu(void)
{
    struct Menu *menu;
    struct Item *item;
    size_t nitems;      /* number of items before current item */
    unsigned long *color;
    size_t labellen;
    int labelx, labely;
    int y;

    for (menu = currmenu; menu != NULL; menu = menu->parent) {
        nitems = 0;
        for (item = menu->list; item != NULL; item = item->next) {

            /* determine item color */
            if (item == menu->selected)
                color = dc.pressed;
            else
                color = dc.unpressed;

            /* calculate item's y position */
            y = nitems * geom.itemh;

            /* draw item box */
            XSetForeground(dpy, dc.gc, color[ColorBG]);
            XFillRectangle(dpy, menu->win, dc.gc, 0, y,
                           geom.itemw, geom.itemh);

            /* draw item label */
            labellen = strlen(item->label);
            labelx = 0 + dc.fonth;
            labely = y + dc.fonth + geom.itemb;
            XSetForeground(dpy, dc.gc, color[ColorFG]);
            XDrawString(dpy, menu->win, dc.gc, labelx, labely, item->label, labellen);

            /* draw triangle, if item contains a submenu */
            if (item->submenu != NULL) {
                int trianglex = geom.itemw - (geom.itemb + dc.fonth);
                int triangley = y + geom.itemb;

                XPoint triangle[] = {
                    {trianglex, triangley},
                    {trianglex + dc.fonth, triangley + dc.fonth/2},
                    {trianglex, triangley + dc.fonth},
                    {trianglex, triangley}
                };

                XFillPolygon(dpy, menu->win, dc.gc, triangle, LEN(triangle),
                             Convex, CoordModeOrigin);
            }

            nitems++;
        }
    }
}

/* run event loop */
static void
run(void)
{
    struct Menu *menu;
    struct Item *item;
    XEvent ev;

    while (!XNextEvent(dpy, &ev)) {
        switch(ev.type) {
        case Expose:
            drawmenu();
            break;
        case MotionNotify:
            getmenuitem(ev.xbutton.window, ev.xbutton.x_root, ev.xbutton.y_root,
                        &menu, &item);
            if (menu != NULL && item != NULL) {
                menu->selected = item;
                drawmenu();
            }
            break;
        case ButtonPress:
            getmenuitem(ev.xbutton.window, ev.xbutton.x_root, ev.xbutton.y_root,
                        &menu, &item);
            if (menu != NULL && item != NULL) {
                if (item->submenu != NULL) {
                    setcurrmenu(item->submenu);
                } else {
                    printf("%s\n", item->output);
                    cleanupexit();
                }
                drawmenu();
            } else {
                cleanupexit();
            }
            break;
        }
    }
}

/* cleanup and exit */
static void
cleanupexit(void)
{
    XCloseDisplay(dpy);
    exit(0);
}

/* show usage */
static void
usage(void)
{
    (void)fprintf(stderr, "usage: xmenu [-w] menuname\n");
    exit(1);
}

You compile it with the following commands (You may have to change /usr/X11R6 to /usr/ on Linux):

cc -Wall -Wextra -I/usr/X11R6/include -c xmenu.c
cc -o xmenu xmenu.o -L/usr/X11R6/lib -lX11
\$\endgroup\$

2 Answers 2

6
\$\begingroup\$

Prefer functions and const variables over macros

While the LEN macro might be unavoidable, MIN and MAX can typically be implemented as functions. This avoids problems with side-effects, because in your macros one of the arguments is always evaluated twice.

For the constants, declare static const variables, like:

static const char *FONT = "-*-*-medium-*-*-*-14-*-*-*-*-*-*-*";
static int ITEMW = 130;
...

Don't write what you don't use

MIN and MAX are not used in the code you posted, so it's better to remove them completely.

Use consistent naming

In your code, you sometimes write things out completely, in other cases you are abbreviating things. While overly long names might be detrimental, with autocompletion nowadays it should not be a problem to write somewhat long names. Things I would change:

  • DC -> DrawContext
  • ScreenGeom -> ScreenGeometry
  • win -> window
  • cursx -> cursorx

If you use all lower-case names, you might consider using underscores to separate words for extra clarity, such as cursor_x.

Also, when you have a pointer to something, don't use p for the name of the pointer, but rather choose a name that describes what is being pointed to. So instead of:

struct Menu *p;

Write:

struct Menu *menu;

In places where this results in conflicts, like in setcurrmenu(), consider renaming one variable to make the distinction clear, like:

static void
setcurrmenu(struct Menu *new_currmenu)
{
    ...
    currmenu = new_currmenu;
    ...
}

Some abbreviations are commonplace, and it's fine to keep those, like x, y, w and h for coordinates and dimensions, and i and j for loop counters, n and len for counts.

Declare variables as close as possible to the place of use

A long time ago compilers needed all local variables to be declared at the top of a function, however nowadays that is not necessary. Consider declaring them at the place where they are first used. Also, you can declare variables inside for and while-statements. For example, you can write:

for (struct Menu *menu = currmenu, menu != NULL; menu = menu->parent) {
    size_t nitems = 0;
    for (struct Item *item = menu->list; item != NULL; item = item->next) {
         ...
         int y = nitems * geom.itemh;
         ...

Note that you are already doing that in some places.

Avoid forward declarations

Your code starts with a list of forward function declarations, followed by the actual function definitions. However, this means you are repeating yourself unnecessarily. You can avoid forward declarations by reordering your functions. For example, main() calls parsestdin(), so by writing the definition of parsestdin() before that of main() you don't need the forward declarations.

Only in rare cases, like two functions both calling each other, should you need forward declarations.

Have the parsing function take a FILE * pointer

Instead of always reading from stdin, you can make the parser take a FILE * argument to allow it to read from other sources. This is very easy to do and makes the function much more generic.

Simplify parsing by using convenient standard library functions

You are parsing the input lines byte by byte. You can simplify it by using standard library functions like strspn() and strtok_r(), or alternatively scanf(). Here are two alternatives, the first using str*() functions:

while (fgets(buf, BUFSIZ, stdin) != NULL) {
    /* Get the indentation level */
    size_t level = strspn(buf, "\t");

    /* Get the label */
    char *s = buf + level;
    char *saveptr = NULL;
    char *label = strtok_r(s, " \t\n", &saveptr);

    /* Get the output */
    char *output = strtok_r(NULL, "\n", &saveptr);
    if (output == NULL)
        output = label;

The second alternative uses scanf(), using the %n conversion to get the size of each element of the line, instead of copying the elements to separate buffers:

while (fgets(buf, BUFSIZ, stdin) != NULL) {
    int level;
    int labelend;
    int outputstart;
    int outputend = 0;

    if (sscanf(buf, "%*[\t]%n%*s%n %n%*[^\n]%n", &level, &labelend, &outputstart, &outputend) < 2)
        err(1, "error parsing input");

    char *label = buf + level;
    buf[labelend] = '\0';

    char *output = label;
    if (outputend > 0) {
        output = buf + outputstart;
        buf[outputend] = '\0';
    }

Split parsing the textual input and building internal data structures

You are doing both in parsestdin() at the moment. But consider that in the future, you might want to programmatically build menus. In that case it makes more sense to have a function like addmenuitem() to add an item to an existing struct Menu.

Avoid global variables

Global variables are convenient at first, but as your projects grow, they become a burden. For example, what if you want to have two menus visible at the same time? Start by moving the global variables into main(), and if functions called by main(), either directly or indirectly, access the previously global variables, ensure you pass pointers to the local variables to these functions as arguments and return variables.

For example, parsestdin() should not use the global variable rootmenu, but rather declare a local one and return it at the end:

static struct Menu *
parsestdin(void)
{
    struct Menu *rootmenu;
    ...
    return rootmenu;
}

Some functions can return by value, for example calcscreengeom(). Other functions should get passed information as parameters; for example drawmenu() should get currmenu, dpy and dc as parameters.

\$\endgroup\$
5
  • \$\begingroup\$ I forgot to remove the unused macros and the debug function before sending the code to review, sorry about that. The unused macros are there because I needed them in a previous version of the code. \$\endgroup\$
    – phillbush
    May 14, 2020 at 19:27
  • \$\begingroup\$ I thought use #define to create constants was still acceptable in C, it used to be the only way. \$\endgroup\$
    – pacmaninbw
    May 14, 2020 at 22:07
  • \$\begingroup\$ It must have been a long time ago that it was the only way. static const variables have been a thing since at least C89. There are a few issues with them though; for example you can't use them for array sizes or for case statements. See also: stackoverflow.com/questions/1674032/… \$\endgroup\$
    – G. Sliepen
    May 14, 2020 at 22:31
  • \$\begingroup\$ The program is now very mature (thanks for your review), and is now a project in github. I would appreciate if you checked it out. \$\endgroup\$
    – phillbush
    May 30, 2020 at 2:53
  • \$\begingroup\$ PS: I haven't removed the forward declarations because it would generate an enormous git diff, and the only global variables are the variables used by the xlib functions (which should be global, anyway). \$\endgroup\$
    – phillbush
    May 30, 2020 at 7:12
1
\$\begingroup\$

Comments and Observations

On code review we review working code, it is best to remove all debug code prior to posting the question on code review, so that we don't suspect the code isn't working.

Don't include code that isn't used, this is actually a bad programming practice. Of the 3 macros in the code, LEN(x), MAX(x,y) and MIN(x,y) only LEN(x) is being used. Including unused code can confuse maintainers of the code. It increases the amount of code they have to go through when fixing bugs or adding new features. Never expect to be the only one that needs to reads or modifies the code, write for the audience.

Note, the first answer for this stackoverflow.com question provides better definitions for MAX(x,y) and MIN(x,y).

If you are using stdin for input, there is no reason to use X11, the whole point of X11 is to provide graphic user interfaces. Don't mix text input with graphic input.

If you're not going to use the advice in the reviews, why bother to post to code review? Going back through your questions, I'm not the first person to remark on this.

Choice of Language for Graphic Programming

Prefer C++ over C for graphic programs. Graphic programming is generally Object Oriented and C++ is an object oriented language. Some of the benefits are that you can create a basic class/type for a window, menu or label and then add to that class through inheritance, which removes a lot of programming.

Another benefit of C++ over C is the improved ability of error handling through exceptions which also allows recovery. An exception is an event which occurs during the execution of a program. Exceptions are available in most modern programming languages. Recovery means that the program can return to a known state after an error, which is much better than exiting the program.

Avoid calling exit() From Lower Level Functions

The only way to exit the program is a lower level function where exit(EXIT_FAILURE) is called, this is not a good programming practice in C. It prevents the program from recovering or cleaning up after itself and can have side effects. If this was an operating system instead of a compiler it would bring the system down. A better way would be to call setjmp() in main.c and longjmp() where the error occurs. To use setjmp() and longjmp() include setjmp.h. The call to exit() should really only occur if there is no recovery path, it is always a good practice to exit from main.

Prefer System Define Macros Over Hard Coded Numbers

The standard header file stdlib.h includes system macro definitions for EXIT_FAILURE and EXIT_SUCCESS these make the code much more readable than return 0; or return 1; in main() or exit(1); in a sub function. Using these macros are a best practice.

Global Variables

As previously mentioned in your last question global variables generally are considered a bad practice because they make the code harder to write, read, debug and maintain because without searching the whole program it's not clear where the variables are modified. Use local variables whenever possible, and pass necessary information into functions as needed.

Complexity

The code in the functions main(), static void drawmenu(void) and static void calcmenu(struct Menu *menu) 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.

Break the code into smaller functions, with an eye for reuse.

Struct and Variable Names

The good here is that you are generally using variable names longer than one character (exceptions such as p in calcmenu should be noted), the bad is that the variables names are still to short or use abbreviations. This program doesn't need commenting as much as it needs self documenting code. A variable called geom might be something other than geometry, what geometry is it referring to. What is menu->w? when I'm fixing code I don't have time to go searching for comments.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ the w's, h's, x's and y's are widiths, heights, x positions and y positions, respectivelly. I based my program in thingmenu, which also uses a lot of global variables. It is hard to use an event-based code like the ones using xlib without using global variables a lot. I do not know C++. C is the only language I know. \$\endgroup\$
    – phillbush
    May 14, 2020 at 18:48
  • \$\begingroup\$ "If you are using stdin for input, there is no reason to use X11, the whole point of X11 is to provide graphic user interfaces. Don't mix text input with graphic input.". I write this program to work as a filter like dmenu (I also based my code on dmenu, which also uses xlib and also contains a lot of global variables). \$\endgroup\$
    – phillbush
    May 14, 2020 at 18:53
  • 1
    \$\begingroup\$ Oh, you beat me with an answer, and I missed it! \$\endgroup\$
    – G. Sliepen
    May 14, 2020 at 19:16

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.