310 likes | 338 Views
Object and Class Creational Design Patterns. The practicality of modules. CS 236700: Software Design. Usage (1/2). Let’s consider the following directory…. …Then, we can use imgdir.exe as follows:. C:>imgdir.exe a.gif b.jpg c.bmp a.gif,GIF,273,310 b.jpg,JPG,128,82 c.bmp,BMP,128,82.
E N D
Object and ClassCreationalDesign Patterns The practicality of modules CS 236700: Software Design
Usage (1/2) • Let’s consider the following directory… • …Then, we can use imgdir.exe as follows: C:>imgdir.exe a.gif b.jpg c.bmp a.gif,GIF,273,310 b.jpg,JPG,128,82 c.bmp,BMP,128,82
Usage (2/2) • Command line options: C:\temp>imgdir.exe imgdir [-S <sep>] [-f <fields>] [-V] file1 file2 ... -s field separator enclosed in quotes -f fields to display: N-name T-type, W-width, H-height example "NTWH“ -v verbose mode: non image files are listed as well
The mission • Current state: • Imgdir is written in C. • Original source code - 465 lines. • Some bugs were fixed so we can have a clean start • Code quality is poor • Code style is poor • Our mission: Rewrite imgdir.c in a modular way • We CANNOT use classes • We CAN use other C++ features: namespaces, overloading,… • No textbook solution
Source code overview (1/3) /* Command line codes: */ #define FIELDOPTION 1 /* for –f <fields> */ #define SEPOPTION 2 /* for –s <sep> */ #define VERBOSEOPTION 3 /* for –v */ #define NOTOPTION 0 /* indicates a file name */ typedef struct { char *sep; /* seperator string for fields */ char *fields; /* fields to display: nthw */ int fieldlen; /* == strlen(fields) short verbose; /* 1 – Verbose mode is ON */ } DISPLAYOPTIONS;
Source code overview (2/3) #define NOTYPE 0 #define JPEGTYPE 1 #define GIFTYPE 2 #define BMPTYPE 3 typedef struct { int imgtype; /* NOTYPE, JPEGTYPE, etc..*/ char *imgtypename; /* “BMP”, “GIF”, or “JPG” */ char *filename; long width; /* width of image in pixels */ long height; /* height of image in pixels */ } IMAGEINFO;
Source code overview (3/3) void swapShort(short, short *); void swapLong(long, long *); int ffindMarker(FILE *, int, int,long); int setOption(int,char **, int, DISPLAYOPTIONS *); int getOptionCode(char *); void imageType(char *, IMAGEINFO *); void imageSize(IMAGEINFO *); int isjpg(char *); /* 1 – true, 0 - false */ int isgif(char *); int isbmp(char *); void jpegSize(char *, long *,long *); void gifSize(char *, long *,long *); void bmpSize(char *, long *,long *); void dspEntry(DISPLAYOPTIONS*, IMAGEINFO*); void dspUsage(); void main(int,char **);
The mission (continued) • Why do we want to rewrite the source code ? • Suppose, we want to change the value of NOTOPTION to -1: #define NOTOPTION -1 //previously 0 • The problem: void main(int argc, char *argv[]) { // .. for(i = 1; i < argc; ++i) { int t; if(t = getOptionCode(argv[i]) { // Do something } else break; } // .. Rest of main() NOTOPTION is now -1 => t is never 0
Analysis • In imgdir.c a small change can create program-wide bugs • Poor continuity • The program has a flat structure => we must check all 15 functions • No separation => every function can be effected • Relevant terms: • dependency, coupling, cohesion, continuity • So, let’s make this thing modular…
Module breakdown – stage 1 • We will use namespaces as modules • Each function will “move” into an appropriate module • The initial list of modules: • <main>: main() • <isimage>: imageType(), isjpg(), isbmp(), isgif() • <sizeofimage>: imageSize(), jpegSize(), bmpSize(), gifSize() • <utils>: ffindMarker(), swapShort(), swapLong() • <commandline>: getOptionCode(), setOption() • <display>: dspEntry(), dspUsage() Imgdir.c-stage1
<display> <isimage> <Main> <utils> <Commandline> <sizeofimage> Dependency graph • Shows the “usage” relation between modules • Built by checking function calls between modules Number of edges = 6 Imgdir.c-stage1 • Activities of main(): • scans the command line • builds display the option • For each image: • Check image type • Find size • Display the data A little problem: How does <display> know the details of the current image (width, height, type…) ??
Dependency graph • The image details are passed via an IMAGEINFO struct. • A function call is not the only source of dependency • Other C elements can also yield a dependency: typedef, struct, enum • => Let’s place more elements in our modules • IMAGEINFO is placed inside <isimage> • DISPLAYOPTIONS is placed inside <display> Number of edges = 9 <display> <isimage> <main> <utils> <Commandline> <sizeofimage> Imgdir.c-stage2
Module breakdown – stage 2 • After adding IMAGEINFO and DISPLAYOPTIONS • <main>: main() • <isimage>: imageType(), isjpg(), isbmp(), isgif(), IMAGEINFO • <sizeofimage>: imageSize(), jpegSize(), bmpSize(), gifSize() • <utils>: ffindMarker(), swapShort(), swapLong() • <commandline>: getOptionCode(), setOption() • <display>: dspEntry(), dspUsage(), DISPLAYOPTIONS Imgdir.c-stage2
<display> <display> <isimage> <image> (<isimage>,<sizeofimage>, <utils>) <main> <utils> <main> <commandline> <commandline> <sizeofimage> Coupling reduction (1/4) • Considering <isimage>, <sizeofimage>, <utils>: • <utils> is a shared module • <utils> is an implementation detail • There are only three elements used by <main>, <display>: • IMAGEINFO, imageType(), imageSize() • => Define a new module: <image> • A module which contains sub-modules: • <utils>, <sizeofimage>, <isimage> • We can simulate public/private access level Number of edges = 5
Coupling reduction (2/4) • A new module was created: <image> • <main>: main() • <image> imageType(), imageSize(), IMAGEINFO • <isimage>: isjpg(), isbmp(), isgif(), • <sizeofimage>: jpegSize(), bmpSize(), gifSize() • <utils>: ffindMarker(), swapShort(), swapLong() • <commandline>: getOptionCode(), setOption() • <display>: dspEntry(), dspUsage(), DISPLAYOPTIONS
<display> <image> (<isimage>,<sizeofimage>, <utils>) <main> <commandline> <display> (<commandline>) <image> (<isimage>,<sizeofimage>, <utils>) <main> Coupling reduction (3/4) • Considering <main>,<display>,<commandline>: • <main> is coupled with 3 other modules – Too much • The reason for <main>’s coupling with <commandline>: for(i = 1; i < argc; ++i) { if(t = getOption(..)) setOption(..) else break; } • Our next step: • Move the loop into <display> • A new function in <display>: buildOptions() • Place <commandline> inside <display> Number of edges = 3 Imgdir.c-stage3
Coupling reduction (4/4) • <commandline> moved into <display>: • <main>: main() • <image> imageType(), imageSize(), IMAGEINFO • <isimage>: isjpg(), isbmp(), isgif(), • <sizeofimage>: jpegSize(), bmpSize(), gifSize() • <utils>: ffindMarker(), swapShort(), swapLong() • <display>: dspEntry(), dspUsage(), DISPLAYOPTIONS, buildOptions() • <commandline>: getOptionCode(), setOption() Imgdir.c-stage3
Additional steps • What else? • use enum instead of #define • Obeys scope rules • => Can be hidden inside a module • Rewrite using classes ?!
Summary: Coupling reduction • Friendly suggestion: coupling reduction at the system’s upper-level is usually the best place to start. • We used number of edges as a “modularity meter” • There are other important factors, such as: • circles • number of modules • number of functions per module • The “algorithm” • Work on the “Most-coupled” modules • In our case: <main> • Shared modules => make them private sub-modules • In our case: <utils> was moved into the new module <image> • Creating new modules can decrease coupling • <image> • Don’t forget the non-function elements: typedefs, structs, enums,…
ffindMarker() int ffindMarker(FILE *infile, int c1, int c2,long mxbytes) { int c,done,rval,state; long bytecount; rval = 0; state = 0; done = 0; bytecount = 0; while (!done) { if ((bytecount == mxbytes) && (mxbytes != 0)) { done = 1; } else { c = fgetc(infile); if (mxbytes != 0) bytecount++; } switch (state) { case 0: if ( c == c1) state = 1; else if ( c == EOF) done = 1; break; case 1: if ( c == c2) { done = 1; rval = 1; } else if (c == EOF) done = 1; else { fseek(infile, -1, SEEK_CUR); state = 0; } break; } } return rval; }
setOption() int setOption(int opt, char **argv, int index, DISPLAYOPTIONS *dsp) { switch (opt) { case FIELDOPTION : dsp->fields = argv[++index]; dsp->fieldlen = dsp->fields; break; case SEPOPTION : dsp->sep = argv[++index]; break; case VERBOSEOPTION : dsp->verbose = 1; break; } return index; /* index of last argv entry that was read */ }
swapShort() void swapShort(short i, short *t) { char *pi; char *pt; pi = (char *)&i; pt = (char *)t; pt[0] = pi[1]; pt[1] = pi[0]; }
getOptionCode() int getOptionCode(char *opt) { int rval; if(strcmp(opt,"-f")== 0) rval = FIELDOPTION; else if(strcmp(opt,"-s")== 0) rval = SEPOPTION; else if(strcmp(opt,"-v") == 0) rval = VERBOSEOPTION; else rval = NOTOPTION; return rval; }
isJpg() int isjpg(char *fname) { int rval; FILE *infile; rval = 0; infile = fopen(fname,"rb"); /* look for jpeg markers in first two bytes */ if(ffindMarker(infile,0xff,0xd8,2L)) rval = 1; /* check last two bytes for end marker */ fseek(infile, -2, SEEK_END); rval = rval && ffindMarker(infile,0xff,0xd9,2L); fclose(infile); return rval; }
bmpSize() void bmpSize(char *fname, long *width, long *height) { FILE *infile; long w,h; short bigindian; bigindian = ((char *)(&INDIANFLAG))[0]; /* determine endianess */ infile = fopen(fname,"rb"); fseek(infile,18,SEEK_SET); /* skip header header string */ fread(&w,sizeof(long),1,infile); fread(&h,sizeof(long),1,infile); if (bigindian) /* bmp are little indian format */ { swapLong(w,width); swapLong(h,height); } else { *width = w; *height = h; } fclose(infile); }
dspEntry() void dspEntry(DISPLAYOPTIONS* dsp, IMAGEINFO img) { int i; if(dsp.fieldlen > 0) { for( i = 0; i < dsp.fieldlen-1; i++) { switch (dsp.fields[i]) { case 'N' : case 'n' : printf("%s%s",img.filename, dsp.sep); break; case 'T' : case 't': printf("%s%s",img.imgtypename, dsp.sep); break; case 'W' : case 'w' : printf("%ld%s",img.width, dsp.sep); break; case 'H' : case 'h' : printf("%ld%s", img.height, dsp.sep); } } switch(dsp.fields[dsp.fieldlen-1]) { case 'N' : case 'n': printf("%s\n",img.filename); break; case 'T' : case 't' : printf("%s\n",img.imgtypename); break; case 'W' : case 'w' : printf("%ld\n",img.width); break; case 'H' : case 'h' : printf("%ld\n", img.height); } } }
main() void main(int argc, char *argv[]) { int i; if(argc == 1) dspUsage(); for(i = 1; i < argc; ++i) { int t; if(t = getOptionCode(argv[i]) { i = setOption(t, argv, i, &displayOptions); } else break; } for( ; i < argc; ++i) { imageType(argv[i],¤tImage); if(displayOptions.verbose || currentImage.imgtype != NOTYPE) { imageSize(¤tImage); dspEntry(displayOptions, currentImage); } } // for } // main()
imageType() void imageType(char *img, IMAGEINFO *info) { int rval; info->filename = img; if(isjpg(img)) info->imgtype = JPEGTYPE; else if(isgif(img)) info->imgtype = GIFTYPE; else if(isbmp(img)) info->imgtype = BMPTYPE; else info->imgtype = NOTYPE; }
imageSize() void imageSize(IMAGEINFO *info) { switch (info->imgtype) { case JPEGTYPE : jpegSize(info->filename,&(info->width),&(info->height)); info->imgtypename = "JPG"; break; case BMPTYPE : bmpSize(info->filename,&(info->width),&(info->height)); info->imgtypename = "BMP"; break; case GIFTYPE : gifSize(info->filename,&(info->width),&(info->height)); info->imgtypename = "GIF"; break; case NOTYPE : info->width = info->height = 0; info->imgtypename = "XXX"; } }
buildOptions() int buildOptions(int argc, char** argv, DISPLAYOPTIONS* result) { /* this function replaces the first loop in main() */ int i; result->sep =","; result->fields = "ntwh"; result->fieldlen = 4; result->verbose = 0; for(i = 1; i < argc; ++i) { int t; if(t = getOptionCode(argv[i]); i = setOption(t, argv, i, &displayOptions); else break; } return i; }