Advanced Programming II Class Notes

Assignment 1 Code Review

Advanced Programming II - 6 February 2001

Code Size

newlines semicolons

16 120 7 42

54 136 16 44

55 155 16 57

67 175 17 60

76 179 20 64

78 187 23 67

87 188 31 77

88 269 32 89

93 272 33 94

99 294 34 132

105 321 37 134

110 41

newlines	semicolons
16	120	7	42
54	136	16	44
55	155	16	57
67	175	17	60
76	179	20	64
78	187	23	67
87	188	31	77
88	269	32	89
93	272	33	94
99	294	34	132
105	321	37	134
110		41

What is good?
Variability

The Basics - Magic Numbers

Numbers are meaningless; don't use them.

Which has more meaning?

`case 0:`	`case '<':`	`case OUT:`
`case 1:`	`case 'F':`	`case GOT_LBR:`
`case 2:`	`case 'O':`	`case GOT_F:`
`case 3:`	`case 'N':`	`case GOT_O:`
`case 4:`	`case 'T':`	`case GOT_N:`
`case 5:`	`case '>':`	`case PRE_ATT:`
`case 6:`		`case ATT_TXT:`

The Basics - Use Meaningful Names

static int flag = 1;
static int flag2 = 0;
if (*text == '<' || flag2 == 1)
flag2 = 1;
flag2 = 0;
flag = 0;
if (*text == ' ' || flag == 0)
flag = 0;
flag2 = 0;
if (flag == 0)
flag = 1;

What does flag and flag2 mean?
Use meaningful types: flag and flag2 are booleans.
No magic numbers - 0 and 1 are not true and false.

The Algorithm

This is correct by inspection.
It's easy to change.
Estimate code properties.
- How fast is the code?
- How big is the code?

Storage Management

Don't use new and delete if you don't have to.
- It's expensive in time and space.
- It's dangerous: dangling pointers and lost storage.
- It can cause run-time errors.

Dynamic Storage Management

Make sure you really need it.

Why do

char *temp = new char[5];
strcpy(temp,"<FONT");
char *temp2 = new char[5];

when

const char *temp = "<FONT";
char temp2[5];

works just as well.

Don't forget the overhead.
- Minimum Solaris allocation size is 16 bytes.
- Heap allocation slower than stack allocation.

A Storage Tragedy

static char tag[10];
static int tagindex=0;

// and so on

if (check_tag == 0 && tag_found == 0) {
  tagindex=0;
  tag[tagindex]=toupper(*text);
  tagindex++;
  }
else if (check_tag == 1 && tag_found == 0) {
  if (!isspace(*text) && *text != '>') {
    tag[tagindex]=toupper(*text);
    tagindex++;
    }
  else {
    tag_found=1;
    tag[tagindex]='\0';
    if (!strcmp(tag, "FONT"))
      font_flag=1;
    else
      font_flag=0;
    }

This is a buffer overflow.
- Hackers feed on this error.

A Storage Disaster

void disable_font(char *text, unsigned char_cnt) {

  char tmp_char, * tmp;
  static int flag = 0;

  *(text + char_cnt) = NULL;
  while (*text != NULL) {
    // and so on

This is an off-by-one error.
- It drops bombs in other people's code.
- It's difficult to find.
- It may be impossible to fix without source.

A Storage Problem

What's wrong with this code?

int inx = 0;
while (inx < char_cnt)
  switch(text[inx]) {
    if (tcount != 0 && 
        ((text[inx + 3] == 'T') || (text[inx + 3] == 't')))
    for (inx = inx + 4; inx != '>'; inx++)
    text[inx] = 32;
  inx++;

Always know what you're accessing and whether it's valid.
Note also the magic numbers.

Avoid Complexity

Constantly review your code for simplifications.
What is complex?
- More than 2 or 3 is complex.
- Deep or far or long is complex.
- Nesting or jumping or falling is complex.
- Big or small or obscure is complex.
- Duplication is complex.

Duplication Complexity

Repeatedly duplicating code is a sign of complexity.

if (text[i+1]=='\t'||text[i+1]=='\n'||text[i+1]==' '||text[i+1]=='F'||text[i+1]=='f')
if (text[i+2]=='F'||text[i+2]=='f'||text[i+2]=='O'||text[i+2]=='o')
if (text[i+3]=='N'||text[i+3]=='n'||text[i+3]=='O' ||text[i+3]=='o')
if (text[i+4]=='N'||text[i+4]=='n'||text[i+4]=='T'||text[i+4]=='t')
if (text[i+5]=='\t'||text[i+5] =='\n'||text[i+5]==' '||text[i+5]=='T'||text[i+5]=='t')
if (text[i+1]=='\t'||text[i+1]=='\n'||text[i+1]==' '||text[i+1]=='F'||text[i+1]=='f')
if (text[i+2]=='F'||text[i+2]=='f'||text[i+2]=='O'||text[i+2]=='o')
if (text[i+3]=='N'||text[i+3]=='n'||text[i+3]=='O' ||text[i+3]=='o')
if (text[i+4]=='N'||text[i+4]=='n'||text[i+4]=='T'||text[i+4]=='t')
if (text[i+5]=='\t'||text[i+5] =='\n'||text[i+5]==' '||text[i+5]=='T'||text[i+5]=='t') 
if (text[i]=='\t'||text[i]=='\n'||text[i]==' '||text[i]=='F'||text[i]=='f')
if (text[i+1]=='F'||text[i+1]=='f'||text[i+1]=='O'||text[i+1]=='o')
if (text[i+2]=='N'||text[i+2]=='n'||text[i+2]=='O' ||text[i+2]=='o')
if (text[i+3]=='N'||text[i+3]=='n'||text[i+3]=='T'||text[i+3]=='t')
if (text[i+4]=='\t'||text[i+4] =='\n'||text[i+4]==' '||text[i+4]=='T'||text[i+4]=='t')
if (text[i]=='F'||text[i]=='f'||text[i]=='O'||text[i]=='o')
if (text[i+1]=='N'||text[i+1]=='n'||text[i+1]=='O' ||text[i+1]=='o')
if (text[i+2]=='N'||text[i+2]=='n'||text[i+2]=='T'||text[i+2]=='t')
if (text[i+3]=='\t'||text[i+3] =='\n'||text[i+3]==' '||text[i+3]=='T'||text[i+3]=='t')
if (text[i]=='N'||text[i]=='n'||text[i]=='O'||text[i]=='o')
if (text[i+1]=='N'||text[i+1]=='n'||text[i+1]=='T'||text[i+1]=='t')
if (text[i+2]=='\t'||text[i+2] =='\n'||text[i+2]==' '||text[i+2]=='T'||text[i+2]=='t')
if (text[i]=='N'||text[i]=='n'||text[i]=='T'||text[i]=='t')
if (text[i+1]=='\t'||text[i+1] =='\n'||text[i+1]==' '||text[i+1]=='T'||text[i+1]=='t') 
if (text[i]=='\t'||text[i]=='\n'||text[i]==' '||text[i]=='T'||text[i]=='t')

Almost Duplication Complexity

Even worse than duplication, because you're not sure.

What is going on here?

OB=false;   
OBF=false;  
OBFO=false; 
OBFON=false;
FONT=true;

OB=false;   
OBF=false;  
OBFO=true;  
OBFON=false;
FONT=false;

OB=false;
OBF=false;
OBFO=false;
OBFON=false;
FONT=false;

OB=false;
OBF=true;    
OBFO=false;  
OBFON=false; 
FONT=false;

OB=false;   
OBF=false;  
OBFO=false; 
OBFON=false;
FONT=true;

OB=false;   
OBF=false;  
OBFO=false; 
OBFON=false;
FONT=true;

OB=false;  
OBF=false; 
OBFO=false;
OBFON=true;
FONT=false;

OB=false;    
OBF=false;   
OBFO=false;  
OBFON=false; 
FONT=false;

Are you sure?

Nesting Complexity

Deep nesting is complex.

if (text[i]=='<')
  if ((text[i] == 'f' || text[i] == 'F') &&  i < text_len)
    if ((text[i] == 'o' || text[i] == 'O') &&  i < text_len)
      if ((text[i] == 'n' || text[i] == 'N') &&  i < text_len)
	if ((text[i] == 't' || text[i] == 'T') &&  i < text_len)
	  if (text[i] == ' ' &&  i < text_len)
	    if (text[i] == '>' && i < text_len)
	      else if (text[i] != ' ' && i < text_len)
	    else if ((text[i] != 't' && text[i] != 'T') && i < text_len)
	 else if ((text[i] != 'n' && text[i] != 'N') && i < text_len)
     else if ((text[i] != 'o' && text[i] != 'O') && i < text_len)
   else if ((text[i] != 'f' && text[i] != 'F') && i < text_len)

2 Complexity

Sometimes doing two things is too complex.

bool evalCharacter(char ch, char val) {
  if ( val == ' ' ) {
    if ( (ch == ' ') || (ch == '\t') || (ch == '\n') )
      return true;
    }
  else
    if ( toupper(ch) == toupper(val) )
      return true;
  return false;
  }

What does this predicate do?
Should it do that?
- You can only ever check for one or the other.

How to Cope

Develop a critical eye for complexity.
Simplify, simplify, simplify.
Don't start typing until the algorithm's done.
Do code reviews.
- 2 or 3 people, all with code.
- Each explains their code to the others.
- The others ask questions, point out problems.
- Only the explainer takes notes.
Do algorithm reviews too.

This page last modified on 27 February 2001.

newlines
16	120	7	42
54	136	16	44
55	155	16	57
67	175	17	60
76	179	20	64
78	187	23	67
87	188	31	77
88	269	32	89
93	272	33	94
99	294	34	132
105	321	37	134
110		41

newlines
16	120	7	42
54	136	16	44
55	155	16	57
67	175	17	60
76	179	20	64
78	187	23	67
87	188	31	77
88	269	32	89
93	272	33	94
99	294	34	132
105	321	37	134
110		41

newlines
16	120	7	42
54	136	16	44
55	155	16	57
67	175	17	60
76	179	20	64
78	187	23	67
87	188	31	77
88	269	32	89
93	272	33	94
99	294	34	132
105	321	37	134
110		41