Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

My exercise was:

Write a program that reads input as a stream of characters until encountering EOF. Have the program print each input character and its ASCII decimal value. Note that characters preceding the space character in the ASCII sequence are nonprinting characters. Treat them specially. If the nonprinting character is a newline or tab, print \n or \t, respectively. Print 10 pairs per line, except start a fresh line each time a newline character is encountered.

This is my code (regarding the special characters, I defined only \n and \t):

#include <stdio.h>
int special_chars(int ch);

int main(void)

{
    int x;
    printf("please enter a some characters, and ctrl + d to quit\n");

    special_chars(x);

    return 0;
}

int special_chars(int ch)

{
    int pairsNum = 0;

    while ((ch = getchar()) != EOF)// testing charecters while not end of file.
    {

    if (ch == '\n')// testing if a control charecter, and printing its
     {
        printf("\\n ");
        pairsNum++;
     }

    else if (ch == '\t')
     {
        printf("\\t ");
        pairsNum++;
     }

    else
    {
        printf("%c,%d ", ch, ch);
        pairsNum++;
    }

    if (pairsNum == 10)// counting the number of outputs, and printing a newline when is 10 (limit).
        printf("\n");
    }
    return ch;
}
share|improve this question
2  
Surprised this works: printf("%c,%d ", ch, ch); %c is expecting a char while %d an int. While ch is always an int. –  Loki Astari Feb 1 '13 at 19:12
    
Because in C style formatted printing the 'verbs' like c or d don't expect anything, but take at least a word from the stack and format it according to their meaning/specs: c -> character, d -> decimal representation of the int/word. –  Ekkehard.Horner Feb 1 '13 at 21:57

2 Answers 2

up vote 4 down vote accepted

A few issues, in no particular order:

  • put main last to avoid the need for a prototype.
  • main normally takes argc/argv parameters
  • special_chars should be static - but it is debateable whether a separate function is necessary in a short program like this.

  • main defines variable x and passes it to special_chars. This is unnecessary. Just define ch in special_chars and define the function as taking void (i.e. nothing)

  • why does special_chars return a value?
  • indenting is wrong
  • too many blank lines (8)
  • missing {} brackets after if (pairsNum == 10)
  • mixed naming styles (pairsNum should be pairs_num or special_chars should be specialChars - I prefer the former)
  • special_chars is a bad name for the function.
  • pairsNum is an odd name; n_pairs would be my choice.
  • you don't print a final \n at the end of the program
  • you don't start a new line after reading a \n, as required - it appears that you do when you type at the keyboard because entering a \n causes a new line. But if you redirect input into your program it does not. Also you don't start a line after each ten, only after the first ten. e.g. if your program executable is called program

    $ cat x
    abcdefgh
    ijklmnop
    qrstuvwxyz
    
    $./program < x
    please enter a some characters, and ctrl + d to quit
    a,97 b,98 c,99 d,100 e,101 f,102 g,103 h,104 \n i,105 
    j,106 k,107 l,108 m,109 n,110 o,111 p,112 \n q,113 r,114 s,115 t,116 u,117 v,118 w,119 x,120 y,121 z,122 \n $ 
    
share|improve this answer

Interpretation of the problem specification

The desired output format is a bit underspecified, but I think you could have done a better job interpreting it. In addition to the bug what @WilliamMorris spotted (where you output a line break after the 10th character, but not after the 20th, 30th, etc.), the output is, frankly, a mess:

$ ./program < /etc/hosts
please enter a some characters, and ctrl + d to quit
#,35 #,35 \n #,35  ,32 H,72 o,111 s,115 t,116  ,32 
D,68 a,97 t,116 a,97 b,98 a,97 s,115 e,101 \n #,35 \n #,35  ,32 l,108 o,111 c,99 a,97 l,108 h,104 o,111 s,115 t,116  ,32 i,105 s,115  ,32 u,117 s,115 e,101 d,100  ,32 t,116 o,111  ,32

Wouldn't this look so much nicer? (It uses 78 characters per line — just right for a standard 80-column terminal.)

$ ./cr21174 < /etc/hosts
 #  35   #  35  \n  10   #  35      32   H  72   o 111   s 115   t 116      32
 D  68   a  97   t 116   a  97   b  98   a  97   s 115   e 101  \n  10   #  35
\n  10   #  35      32   l 108   o 111   c  99   a  97   l 108   h 104   o 111
 s 115   t 116      32   i 105   s 115      32   u 117   s 115   e 101   d 100
    32   t 116   o 111      32   c  99   o 111   …

The specification says that you should treat non-printing characters specially, but other than for newline and tab, it doesn't say exactly how. I would interpret it to mean that you should output some kind of placeholder (preferably one that preserves the nice alignment).

Prompting

It is not always appropriate to prompt the user to enter input like that. For one thing, the prompt makes no sense if the program is receiving its standard input via a pipe or redirection. Furthermore, you shouldn't mingle the prompt with the proper output of the program, in case the user wants to capture the printout.

if (isatty(STDIN_FILENO))
{
    fprintf(stderr, "please enter a some characters, and ctrl + d to quit\n");
}

… where isatty(3) is declared in <unistd.h>.

I assume that you are targeting a Unix-like environment, since you hard-coded "ctrl + d". If you wanted to include Windows as well, you would need a compatibility shim:

#if defined(_WIN32) || defined(_MSDOS)
#   include <io.h>
#   define isatty(x) _isatty(x)
#   define STDIN_FILENO _fileno(stdin)
#   define EOF_SEQ "ctrl + z"
#else
#   include <unistd.h>
#   define EOF_SEQ "ctrl + d"
#endif

…

if (isatty(STDIN_FILENO))
{
    fprintf(stderr, "please enter a some characters, and " EOF_SEQ
                    " to quit\n");
}

Implementation

special_chars() should not take a char parameter. Your compiler should have warned you that x was uninitialized. (You do compile with warnings enabled, I hope?) On the other hand, what would be a reasonable parameter is the number of pairs per line.

It is customary to put main() at the end of the program, so that you don't have to forward-declare the functions that it needs to call.

#include <stdio.h>
#include <unistd.h>

void special_chars(int pairs_per_line)
{
    int ch;
    for (int count = 1; (ch = getchar()) != EOF; count++)
    {
        char *delimiter = (count % pairs_per_line == 0) ? "\n" : "  ";
        if (ch < ' ')  // Non-printing characters
        {
            char esc;
            switch (ch)
            {
                case '\n': esc = 'n'; break;
                case '\t': esc = 't'; break;
                default:   esc = '?';
            }
            printf("\\%c %3d%s", esc, (int)ch, delimiter);
        }
        else
        {
            printf("%2c %3d%s", ch, (int)ch, delimiter);
        }
    }
    putchar('\n');
}

int main(void)
{
    if (isatty(STDIN_FILENO))
    {
        fprintf(stderr, "please enter a some characters, and ctrl + d to quit\n");
    }
    special_chars(10);
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.