ASCII art generator in C

Question

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.

Answer 1

• 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.

• u185619: so where can I look for -M option I googled it but I couldn't find any clue
• vnp: Start with codereview.stackexchange.com/questions/104807/…. For full docs, see gcc.gnu.org/onlinedocs/gcc/…. Search keywords are "makefile automatic dependency generation.

Answer 2

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.

Answer 3

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);

• u185619: +1 for the compatibility with rare non-Ascii machines I wish I could upvote you twice :)
• Pryftan: @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! :)

Answer 4

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'