Assignment 1 Code Review

26 September 2002 - CS 509

Assignment 1 Code Review

• The Test Cases

• Design

• Statistics

• Follow Directions

• Procedure Size

• Strings vs. Character Arrays

• Use Named Constants

• Understand Your Tools

• Other Irritants

The Test Cases

• The empty test parsess < /dev/null.

  • Your code should do nothing gracefully.

• The garbage test parsess < parsess.

  • Your code should handle any outrage.

• The bad-specs test parsess 0, ,0 2-1,0 100,100.

  • Your code should validate input arguments.

• Simple tests; row, column, submatrix, single element.

  • Your code should handle the simple, obvious cases.

• char * abuse tests.

  • Prefer strings; they're simpler and safer.

Test Your Code

• Test your code - it's important.

  • Five people submitted code that produced no right answers.

• The tests don't have to be complicated.

  • Simple tests applied consistently do the trick.

  • Good coding habits keep you out of obscure danger.

• Testing your own code is hard.

  • Your own child never needs a spanking.

  • Get someone else to do it.

• Code a little, test a little.

Design

• Algorithm design, not program design.

  • Think before you type.

  • Everything follows from a thought-out plan.

• Understand the problem before attempting a solution.

  • Read and understand the problem.

    • What is an empty row

  • Understand the problem's background too.

    • But don't get carried away; solve the problem in front of you.

• Good programmers solve problems, they don't write code.

  • No amount of typing can replace a good idea.

A Solution

• What's the problem?

  • The information's coming in line by line.

  • You need it all to make a decision. Or do you?

• The big idea: You may not know where the columns separators are, but you know where they aren't.

  • All columns left after input's over must be column separators.

  • A simple, line by line solution.

• Everything after the big idea are just details.

Line Statistics - Newlines

Line Statistics - Semicolons

Procedure Statistics

Follow Directions

• No spreadsheet is o.k.

  $ parsess < /dev/null
  Invalid Input
  
  $ 
  

• No specs are o.k.

  $ parsess < /dev/null
  Requires minimum one argument .... 
  
  $ 
  

• Put blank lines between spreadsheet sections.

  $ echo aaaa bbbb | ./parsess 0,0 0,1
    aaaa
     bbbb
  
  $ 
  

A Tale of Two mains()

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

  compute_columns();

  find_ss_cells(argc, argv);

  return 0;
 }

Procedure Size Matters

• Small procedures are easy to design, write, and understand.

  • "Small code" means at most 20 to 30 lines.

  • If you can't write a simple, complete sentence describing function, it's probably too big.

• There may be a lot of small procedures, but

  • You don't have to understand them all at once.

  • You can corral them in files, classes, and name spaces.

Prefer Strings to Character Arrays I

• Character arrays are easy to use incorrectly.

  char local_argv[25]
  
  for (p = 1; p < argc; p++) {
    strcpy(local_argv, argv[p])
    // whatever
    }
  

• This is what's known as a buffer overflow error.
  A vulnerability has been discovered in Alsaplayer. By specifying an overly long "add-on path", it is possible for an attacker to overrun the buffer, potentially allowing for execution of attacker-supplied code.

  --- Security Focus, 20 September 2002

• Buffer overflow errors are going to cost you. Dearly.

Prefer Strings to Character Arrays II

• Character arrays are hard to use correctly.

  string str
  char ch[200]
  while(true) {
    if (!cin.eof()) {
      cin.getline(ch, 200, '\n')
      str = ch
      // whatever.
      }
  

  This code fails for 200+ character input lines.

  What possible explanation is there for code like this?

• Prefer strings to character arrays.

  string str
  while (getline(cin, str)) {
    // whatever
    }
  

Use Named Constants

• An unnamed constant is a magic number.

  • It just appears; no reason, no explanation.

    if (r_bstr[i] < 48 || r_bstr[i] > 57)
    

    48? 57? What are these? Are you sure?

• Explanations don't really help.

  if (str1[loc] == 32) // 32 is ASCII char sp
  

  Oh? Is everything ASCII?

• They are hard to change.

  char ch[200]
  while cin.getline(ch, 200, '\n')
    ...
    f(200)
    ...
  

  Have you changed all proper values?

  Have you left alone all other values?

Understand How EOF Works

• Read before test, not after.

  while !cin.eof()
    getline(cin, s )
    if (s.size() != 0)
  

  while true
    if !cin.eof()
      getline(cin, line)
  

• Follow the idiom.

  while getline(cin, s)
    // whatever
  

Understand How Stream I-O Works

• One test is enough.

  while getline(std::cin, s)
    if std::cin.eof()
      break
  

• Understand your tools.

  while cin.good() && !cin.eof() && cin.get(ch)
    if EOF == ch
      break
  

  • good() implies !eof().

  • ch is not set to EOF.

• Don't be obscure (and redundant).

  if cin.peek() == EOF
    exit(-1)
  while !cin.eof()
    if cin.peek() == EOF
      break
    getline(cin, spread)
  

Understand Your Tools

• Blank padding strings.

  if (len < recLen)
    for (int j = 0; j < (recLen - len); j++)
      line = line + ' ';
  

  This is complicated, expensive, and slow.

  space = ss_vec[j][k+1].length - ss_vec[j][k].length;
  (ss_vec[j][k].cell).append(space, ' ');
  

  This is not.

Other Irritants

• Disable debugging print statements.

  $ echo aaaa bbbb | parsess 0,0   
  
  first_row = 0
  last_row = 0vs[0] = aaaa bbbb
  
    ! Out of range spec
  
  $
  

• Eliminate pointless chatter.

  $ parsess < /dev/null
  No Element-Specs supplied. Nothing to do...
  ...Doing nothing. ba'Bye
  
  $ 
  

Points to Remember

• Understand the problem

  • You can't solve it if you can't understand it.

• Think before you program.

  • A little thinking replaces a lot of typing.

• Test your code.

  • Find your errors before others do.

• Use good programming practice.

  • Small procedures, named constants.

• Know your tools.

  • They are there to help you.

This page last modified on 26 September 2002.