10
\$\begingroup\$

I have written an ASCII art generator library. I was practicing data abstraction and code abstraction and I wanted to know if there is something that can be improved.

File tree:

   |         |--Makefile
   |--fonts--|--StarStrips.c
   |         |--whimsy.c
   |
   |           |--core.h 
---|--include--|--StarStrips.h
   |           |--whimsy.h
   |
   |--Makefile

/fonts/Makefile

sharedlib: whimsy.o StarStrips.o
    @echo Building the Shared Library;  \
    gcc  -shared -fPIC -o ascii-arts.so whimsy.o StarStrips.o;  
StarStrips.o : StarStrips.c
    @echo building StarStrips;          \
    gcc -c -fPIC StarStrips.c -o StarStrips.o -std=c99 -I../include 
whimsy.o : whimsy.c
    @echo building whimsy;          \
    gcc -c -fPIC whimsy.c -o whimsy.o -std=c99 -I../include 

/font/whimsy.c

#include <whimsy.h>
#include <stdlib.h>
#include <stdio.h>
#include <ctype.h>
#include <string.h>
int whimsy_allowed(int c)
{
    return islower(c) || c == ' ';
}

int whimsy_index (int c)
{
    return  islower(c)? c -'a'+1 :0;
}

int whimsy_init(void){
    font_alloc(whimsy,10,27);
    d_alloc(whimsy,space,' ',5,ARRAY({
            "    ",
            "    ",
            "    ",
            "    ",
            "    ",
            "    ",
            "    ",
            "    ",
            "    ",
            "    "          }));
    d_alloc(whimsy,a,'a',11,ARRAY({
            "          ",
            "          ",
            "          ",
            " d888b8b  ",
            "d8P' ?88  ",
            "88b  ,88b ",
            "`?88P'`88b",
            "          ",
            "          ",
            "          "        }));
    d_alloc(whimsy,b,'b',11,ARRAY({
            " d8b      ",
            " ?88      ",
            "  88b     ",
            "  888888b ",
            "  88P `?8b",
            " d88,  d88",
            "d88'`?88P'",
            "          ",
            "          ",
            "          "        }));
    d_alloc(whimsy,c,'c',8,ARRAY({
            "       ",
            "       ",
            "       ",
            " d8888b",
            "d8P' `P",
            "88b    ",
            "`?888P'",
            "       ",
            "       ",
            "       "       }));
    d_alloc(whimsy,d,'d',11,ARRAY({
            "      d8b ",
            "      88P ",
            "     d88  ",
            " d888888  ",
            "d8P' ?88  ",
            "88b  ,88b ",
            "`?88P'`88b",
            "          ",
            "          ",
            "          "        }));
    d_alloc(whimsy,e,'e',8,ARRAY({
            "       ",
            "       ",
            "       ",
            " d8888b",
            "d8b_,dP",
            "88b    ",
            "`?888P'",
            "       ",
            "       ",
            "       "       }));
    d_alloc(whimsy,f,'f',11,ARRAY({
            "   ,d8888b",
            "   88P'   ",
            "d888888P  ",
            "  ?88'    ",
            "  88P     ",
            " d88      ",
            "d88'      ",
            "          ",
            "          ",
            "          "        }));
    d_alloc(whimsy,g,'g',11,ARRAY({
            "          ",
            "          ",
            "          ",
            " d888b8b  ",
            "d8P' ?88  ",
            "88b  ,88b ",
            "`?88P'`88b",
            "       )88",
            "      ,88P",
            "  `?8888P "        }));
    d_alloc(whimsy,h,'h',11,ARRAY({
            " d8b      ",
            " ?88      ",
            "  88b     ",
            "  888888b ",
            "  88P `?8b",
            " d88   88P",
            "d88'   88b",
            "          ",
            "          ",
            "          "        }));
    d_alloc(whimsy,i,'i',6,ARRAY({
            "  d8,",
            " `8P ",
            "     ",
            "  88b",
            "  88P",
            " d88 ",
            "d88' ",
            "     ",
            "     ",
            "     "         }));
    d_alloc(whimsy,j,'j',8,ARRAY({
            "   d8, ",
            "  `8P  ",
            "       ",
            "  d88  ",
            "  ?88  ",
            "   88b ",
            "   `88b",
            "    )88",
            "   ,88P",
            "`?888P "       }));
    d_alloc(whimsy,k,'k',12,ARRAY({
            " d8b       ",
            " ?88       ",
            "  88b      ",
            "  888  d88'",
            "  888bd8P' ",
            " d88888b   ",
            "d88' `?88b,",
            "           ",
            "           ",
            "           "       }));
    d_alloc(whimsy,l,'l',6,ARRAY({
            " d8b ",
            " 88P ",
            "d88  ",
            "888  ",
            "?88  ",
            " 88b ",
            "  88b",
            "     ",
            "     ",
            "     "         }));
    d_alloc(whimsy,m,'m',15,ARRAY({
            "              ",
            "              ",
            "              ",
            "  88bd8b,d88b ",
            "  88P'`?8P'?8b",
            " d88  d88  88P",
            "d88' d88'  88b",
            "              ",
            "              ",
            "              "    }));
    d_alloc(whimsy,n,'n',11,ARRAY({
            "          ",
            "          ",
            "          ",
            "  88bd88b ",
            "  88P' ?8b",
            " d88   88P",
            "d88'   88b",
            "          ",
            "          ",
            "          "        }));
    d_alloc(whimsy,o,'o',9,ARRAY({
            "        ",
            "        ",
            "        ",
            " d8888b ",
            "d8P' ?88",
            "88b  d88",
            "`?8888P'",
            "        ",
            "        ",
            "        "      }));
    d_alloc(whimsy,p,'p',11,ARRAY({
            "          ",
            "          ",
            "          ",
            "?88,.d88b,",
            "`?88'  ?88",
            "  88b  d8P",
            "  888888P'",
            "  88P'    ",
            " d88      ",
            " ?8P      "        }));
    d_alloc(whimsy,q,'q',11,ARRAY({
            "          ",
            "          ",
            "          ",
            ".d88b,.88P",
            "88P  `88P'",
            "?8b  d88  ",
            "`?888888  ",
            "    `?88  ",
            "      88b ",
            "      ?8P "        }));
    d_alloc(whimsy,r,'r',10,ARRAY({
            "         ",
            "         ",
            "         ",
            "  88bd88b",
            "  88P'  `",
            " d88     ",
            "d88'     ",
            "         ",
            "         ",
            "         "     }));
    d_alloc(whimsy,s,'s',9,ARRAY({
            "        ",
            "        ",
            "        ",
            " .d888b,",
            " ?8b,   ",
            "   `?8b ",
            "`?888P' ",
            "        ",
            "        ",
            "        "      }));
    d_alloc(whimsy,t,'t',9,ARRAY({
            "        ",
            "   d8P  ",
            "d888888P",
            "  ?88'  ",
            "  88P   ",
            "  88b   ",
            "  `?8b  ",
            "        ",
            "        ",
            "        "      }));
    d_alloc(whimsy,u,'u',10,ARRAY({
            "         ",
            "         ",
            "         ",
            "?88   d8P",
            "d88   88 ",
            "?8(  d88 ",
            "`?88P'?8b",
            "         ",
            "         ",
            "         "     }));
    d_alloc(whimsy,v,'v',10,ARRAY({
            "         ",
            "         ",
            "         ",
            "?88   d8P",
            "d88  d8P'",
            "?8b ,88' ",
            "`?888P'  ",
            "         ",
            "         ",
            "         "     }));
    d_alloc(whimsy,w,'w',16,ARRAY({
            "               ",
            "               ",
            "               ",
            " ?88   d8P  d8P",
            " d88  d8P' d8P'",
            " ?8b ,88b ,88' ",
            " `?888P'888P'  ",
            "               ",
            "               ",
            "               "   }));
    d_alloc(whimsy,x,'x',10,ARRAY({
            "         ",
            "         ",
            "         ",
            "?88,  88P",
            " `?8bd8P'",
            " d8P?8b, ",
            "d8P' `?8b",
            "         ",
            "         ",
            "         "
                        }));
    d_alloc(whimsy,y,'y',11,ARRAY({
            "          ",
            "          ",
            "          ",
            "?88   d8P ",
            "d88   88  ",
            "?8(  d88  ",
            "`?88P'?8b ",
            "       )88",
            "      ,d8P",
            "   `?888P'"        }));
    d_alloc(whimsy,z,'z',9,ARRAY({
            "        ",
            "        ",
            "        ",
            "d88888P ",
            "   d8P' ",
            " d8P'   ",
            "d88888P'",
            "        ",
            "        ",
            "        "      }));
    return 0;
}
int whimsy_exit(void){
    for(int i=0;i<whimsy.d_n;i++)
        for(int j=0;j<whimsy.c;j++)
            free(whimsy.d[i][j]);
    for(int i=0;i<whimsy.d_n;i++)
        free(whimsy.d[i]);
    free(whimsy.d);
    free(whimsy.r);
}
int whimsy_print(const char* s){
    int n = strlen(s);
    for(int i=0;i<n;i++)
                if(whimsy.allowed(s[i]) != 1)
                        return -1;
    for (int i=0;i<whimsy.c;i++)
        {
                for(int j=0;j<n;j++)
                        fputs(whimsy.d[whimsy.index(s[j])][i],stdout);
                putchar('\n');
        }

}

I won't show you /font/StarStrips.c because it's similar lengthy code and the goal of reviewing the code is not giving opinions about the fonts self.

/include/core.h

#ifndef _ascii_arts_core_
#define _ascii_arts_core_
/*
r rows
c columns
d design
char_n number of characters
*/
struct font{
unsigned int c;
unsigned int *r;
unsigned int d_n;
int (*allowed)(int);
int (*index)(int);
int (*init)(void);
int (*exit)(void);
int (*print)(const char*);
char ***d;
};
#define XX(a,b,c) a##b##c
#define font_alloc(font,column,nofdesigns)              \
    do{                             \
        font.d_n = nofdesigns;                  \
        font.c = column;                    \
        font.r = malloc(sizeof(int)*font.d_n);          \
            font.d = malloc(sizeof(char***)*font.d_n);      \
        for(int i=0;i<font.d_n;i++)             \
            font.d[i] =malloc(sizeof(char**)*font.c);   \
    }while(0)
#define ARRAY(...) __VA_ARGS__
#define d_alloc(font,__tok,_char,row,design)                \
    do{                             \
        font.r[font.index(_char)] = row;            \
        for(int j=0;j<font.c;j++)               \
            font.d[font.index(_char)][j] =          \
            malloc(sizeof(char*)*font.r[font.index(_char)]);\
        char * temp[]  = design;                \
        for(int i=0;i<font.c;i++)               \
            strcpy(font.d[font.index(_char)][i],temp[i]);   \
    }while(0)
#endif

/include/whimsy.h

#ifndef _whimsy_font_
#define _whimsy_font_
#include <core.h>
int whimsy_allowed(int c);
int whimsy_index(int c);
int whimsy_init(void);
int whimsy_exit(void);
int whimsy_print(const char* s);
struct font whimsy = {
    .allowed = whimsy_allowed,
    .index = whimsy_index,
    .init = whimsy_init,
    .exit = whimsy_exit,
    .print = whimsy_print
};
#endif

/Makefile

uninstall:
    @echo Uninstalling library;     \
    rm /usr/lib/libaarts.so -f;     \
    cd /usr/include         \
    rm whimsy.h core.h StarStrips.h -f
install: lib
    @echo Installing library;   \
    cp include/*.h /usr/include/;   \
    cp libaarts.so /usr/lib/
lib:
    @cd fonts;          \
    make sharedlib;         \
    mv ascii-arts.so ../libaarts.so

clean:
    @rm *.so -f;            \
    cd fonts;           \
    rm *.o -f;          \
    cd ../demo;         \
    rm *.d -f;
demos:
    @cd demo;           \
    make whimsy

Little notes

First of all, the main goal of this library was to provide a data structure to hold fonts for ASCII art characters and maybe ASCII art drawings (i.e. converting a PNG image to ASCII arts (not yet done)).

in the d_alloc macro the __tok parameter is useless and it is there just for backwards comptiability with previous versions of the same library.

\$\endgroup\$
5
\$\begingroup\$
  • fonts/Makefile

    Each object depends only on the corresponding source. It means that header modifications wouldn't trigger recompilation. You may fix it by explicitly spelling out dependencies:

    whimsy.o: whimsy.c whimsy.h core.h
    

    In general it is a good habit to have dependencies auto generated: even in your not very complicated case it is easy to miss the core.h dependency. Take a look at -M family of gcc options.

  • include/whimsy.h

    Do not define objects (like struct font whimsy) in the header file. You never know how many times the client would happen to #include "whimsy.h" in different places of their project. Better practice is to have the definition in the .c file, and declare it in the header as

    extern struct font whimsy;
    
  • DRY?

    Unfortunately, you didn't show your font file. Also, I'm 95% sure the init and print files are identical modulo font name. If I'm correct, you need to unify them, and have the unified version in core.c.

  • Allocation

    Allocating glyphs dynamically and copying them from a static area looks like a waste for me (it would make sense should you read glyphs from the text file instead). I would have a

    struct glyph {
        int width;
        char * appearance;
    };
    

    (strictly speaking, glyph.width is redundant: given a font height and appearance length you may calculate width at runtime); an array of glyphs as a part of struct font:

    struct font {
        ....
        struct glyph typeface[128];
        ....
    }
    

    and a static initialization of each font like

    static struct font whimsy = {
        ....
        .typeface = {
            ....
            ['a'] = (struct glyph) {
                .width = 10,
                .appearance =
                    "          "
                    "          "
                    "          "
                    " d888b8b  "
                    "d8P' ?88  "
                    "88b  ,88b "
                    "`?88P'`88b"
                    "          "
                    "          "
                    "          ";
            },
            ....    
        },
        ....
    };
    

    Beware that such partial array initialization is a gcc extension.

  • More abstraction

    Now you may take advantage of appearance being default initialized to an NULL, and conclude that such glyph is not implemented. An allowed method becomes a trivial test, and is easily abstracted out of the font specifics.

\$\endgroup\$
3
\$\begingroup\$
  1. Try to format you code properly. Things like these are hard to read:

    struct font{
    unsigned int c;
    unsigned int *r;
    unsigned int d_n;
    int (*allowed)(int);
    int (*index)(int);
    int (*init)(void);
    int (*exit)(void);
    int (*print)(const char*);
    char ***d;
    };
    

    Hard to read code is more difficult to maintain and you're more likely to miss a bug.

  2. Use braces consistently. Things like these just lead to trouble in the long run and don't make the code any easier to read.

    for(int i=0;i<whimsy.d_n;i++)
        for(int j=0;j<whimsy.c;j++)
            free(whimsy.d[i][j]);
    

    Adding additional braces and formatting makes it very explicit what the scope of each loop is and adding additional statements you're less likely to accidentally forget to add a brace.

    for (int i = 0; i < whimsy.d_n; i++) {
        for(int j = 0; j < whimsy.c; j++) {
            free(whimsy.d[i][j]);
        }
     }
    
  3. The use of defines for font_alloc and d_malloc are a pretty bad abuse of the pre-processor. You just use it as an automated copy-n-paste mechanism and copy-n-paste code is bad. #define has it's uses but in this case it's the wrong choice.

\$\endgroup\$
3
\$\begingroup\$

Portability: islower() is great, but c -'a'+1 assumes consecutive a-z. So if you want to work with those rare non-ASCII machines ...

int whimsy_index (int c) {
  if (islower(c)) {
    char s[2] = { c, '\0' };
    // Use base 36
    return strtol(s, 0, 36) - 10 + 1;
  }
  return 0;
}

Complex macros: Unclear as to the value of #define font_alloc(). Instead make a function.

Clear pointers. whimsy_exit() should not assume what the calling code does afterward with whimsy and its fields. Since this code frees the allocation, then NULL the pointers and zero the count.

int whimsy_exit(void){
  ....
  for(int i=0;i<whimsy.d_n;i++) {
     free(whimsy.d[i]);
  }

Use size_t for array index. int may not be wide enough.

int whimsy_print(const char* s){
  // int n = strlen(s);
  size_t n = strlen(s);
  // for(int i=0;i<n;i++)
  for(size_t i= 0; i < n; i++)

Avoid magic numbers. Why 10? Why 27? Even better, compute 10, 27 from the code.

int whimsy_init(void){
  // font_alloc(whimsy,10,27);
  #define WIDTH 10
  #define DESIGN_COUNT 27
  font_alloc(whimsy, WIDTH, DESIGN_COUNT);
\$\endgroup\$
  • 1
    \$\begingroup\$ +1 for the compatibility with rare non-Ascii machines I wish I could upvote you twice :) \$\endgroup\$ – oddcoder Sep 25 '15 at 12:29
  • \$\begingroup\$ @oddcoder My version of that is the size_t idea (though there are instances where it's caused problems having unsigned it can be done differently and it's safer in any case). Even though you couldn't up-vote the answer twice at least I can add to it - and have done. Portability tends to be a good thing too so together we have up-voted twice! :) \$\endgroup\$ – Pryftan Feb 21 at 18:43
1
\$\begingroup\$

Take a look at Figlet, which is very much the same thing you're doing.

As others have mentioned, I think it would be a good idea to separate font resources from code. Currently fonts are just C source files compiled into the application binary. A better way would be to define fonts in a plain text format making font creation easier and accessible to non-programmers as well.

Take a look at the FIGFont Standard, which specifies the font file format used by Figlet. If you were to implement this, you would have lots of fonts all ready to go.

I realize the Figlet fonts are a bit more complicated than what you currently do, and if you think it's too much you could settle for a simplified version without smushing and kerning.

Below is a sample from basic.flf by Craig O'Flaherty.

 .d8b. $@
d8' `8b$@
88ooo88$@
88~~~88$@
88   88$@
YP   YP$@
       $@
       $@@
d8888b.$@
88  `8D$@
88oooY'$@
88~~~b.$@
88   8D$@
Y8888P'$@
       $@
       $@@
 .o88b.$@
d8P  Y8$@
8P     $@
8b     $@
Y8b  d8$@
 `Y88P'$@
       $@
       $@@

Rendering "ABC" with Figlet using this font looks like this:

 .d8b.  d8888b.  .o88b. 
d8' `8b 88  `8D d8P  Y8 
88ooo88 88oooY' 8P      
88~~~88 88~~~b. 8b      
88   88 88   8D Y8b  d8 
YP   YP Y8888P'  `Y88P' 
\$\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.