Validity of macro for printf() call in a corporate environment

Question

I wrote this program:

#include <stdio.h>
#define printS for(i=0;s[i];i++){printf("%c",s[i]);}

int i;
char s[] = "Hello World!\n";

void main()
{
  printS;
}

It outputs exactly what I want it to but I have no clue if this would be acceptable to use if I were working for a company.

Would this be considered a poorly written piece of code in a corporate environment?

Answer 1

• Although the C standard can allow void main() under certain implementations, it's best to use int main() whenever possible. See this for more info on the return type of main().

• A loop is not a good use of a macro:

  #define printS for(i=0;s[i];i++){printf("%c",s[i]);}
  

  It should instead be a function:

  void printS()
  {
      for (i = 0; s[i]; i++)
      {
          printf("%c",s[i]);
      }
  }
  

  #define is a pre-processor directive (or a macro) that will replace the designated piece of code with something else. While you can use this with loops, it's still discouraged as there are cleaner alternatives in C, such as functions.

• Your two variables above main() are in global scope, which is almost always discouraged. Anything in global that's not a constant can be modified anywhere in the program, which can introduce bugs and hurt maintenance. You should keep variables as close in scope as possible, such as in functions. You can then pass them to other functions as needed.

  Assuming this is pre-C99, i should be declared right before the loop. Otherwise, it should be initialized within the loop statement.

• Functions in C that take no arguments should have a void parameter:

  void main(void)
  

• Consider adding more whitespace between operators for more readability.

  Using your single-line loop as an example:

  for(i=0;s[i];i++){printf("%c",s[i]);}
  

  You could have something like this:

  for (i = 0; s[i]; i++) { printf("%c", s[i]); }
  

Implementation with changes applied:

#include <stdio.h>

void printS(char s[])
{
    int i;
    for (i = 0; s[i]; i++)
    {
        printf("%c", s[i]);
    }
}

int main(void)
{
    char s[] = "Hello World!\n";
    printS(s);
}

Answer 2

Apart from the usage of a macro where a function would have been sufficient (don't use macros if there is another language construct that can do the job!) and the global variable (never do this unless it is absolutely needed), there is one big thing that has not yet been mentioned:

Why do you print the string by calling printf() for every single character?

You simply want to output a string without any surrounding format, so even printf("%s", s) would be too much because it would "parse the format string".

In this simple example it would suffice to use puts(s).

Answer 3

Using a macro is OK in C89 which did not have inline functions. But your macro has extremely bad style and I believe it would be considered unacceptable in most, if not all, places:

• It messes with a static variable i
• It doesn't have () , yet it executes statements
• It doesn't play nicely with surrounding code
• It evaluates s more than once

For example

int i = 3;

if ( test )
    printS;
else                  // oops, compilation error - invalid "else"
    something_else();

printf("%d\n", i);    // oops, not 3

To write this macro properly (leaving out for now the fact that the actual code could be improved - I want to demonstrate how to use a variable evaluated more than once):

#define PRINTS(s) do { int i; for (i = 0; (s)[i]; ++i) printf("%c", (s)[i]); } while (0)

Now you can safely use it exactly like you would use a function:

int i = 3;
char const *s = "lol";

if ( condition )
    PRINTS("kek");     // prints "kek", not "lol"
else
    other_stuff();

printf("%d\n", i);    // 3

The purpose of using ALL CAPS is to warn the reader that it is a macro and not a function, so it might evaluate the argument twice. Personally I use ALL CAPS if it does evaluate the argument twice, and a normal function name if it doesn't, although different places have different standards.

Now: it would be much better to make the macro only evaluate its argument once. That makes it a lot more flexible. Of course the simplest way to do this is to call puts(s); or printf("%s, (s));. Another way (let's say the situation was more general than this) would be to factor out the check of (s)[i]:

#define printS(s) do { char const *p; for (p = (s); *p; ++p) printf("%c", *p); } while (0)