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.

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?

share|improve this question
7  
Curios why you would ask this. Why would you use a macro like that? IS there a reason for using that technique over a normal function? –  Loki Astari Jun 14 at 19:34
    
Well, I was experimenting with how to use #define as well as how to create macros. I had put a bunch of similar macros in a separate header file to experiment with using the linker also. I just figured it would be better to put it all in one file for the sake of asking this question. –  Magister Ludi Jun 14 at 19:39
    
This code is not long enough to be poorly-written. It's one-off, experimental throwaway code where by definition anything goes. Your use of a macro is a little janky. Apart from the declaration of main, though, there are no real problems. –  tmyklebu Jun 15 at 2:19
4  
The succinct answer to your question is "Yes" (it would be considered a poorly written piece of code, even in a corporate environment, let alone an open source project). Using that macro would be a red flag warning sign that 'this programmer is not ready to work on corporate code'. While macros can be useful, this is most certainly not an example of a useful macro. –  Jonathan Leffler Jun 15 at 15:18
    
It would be considered poor style for code that I write at home for my own entertainment. –  gnasher729 Jun 16 at 0:31
add comment

3 Answers 3

up vote 19 down vote accepted
  • 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);
}
share|improve this answer
    
Thank you, from now on I will always make main an int, I will make loops into functions instead of macros, and I will try to keep variables within the scope of their use. –  Magister Ludi Jun 14 at 19:51
1  
You don't have to put every loop inside a function. My own rule of thumb is "when you want to repeat some code, extract it into a function". (DRY - Don't Repeat Yourself is an often mentioned principle.) –  marczellm Jun 14 at 21:07
2  
Returning void from main depends on your definition of 'standard'. To the extent that Microsoft promulgates de facto standards and explicitly documents the use of void main(), it is (very sadly) standard. The C standard permits it as an implementation-defined alternative. See What should main() return in C and C++? for full details. –  Jonathan Leffler Jun 15 at 15:15
    
@Jamal You seem to have some misconceptions about macros. They're essentially just search-and-replace before compilation, you can't actually evaluate any code before compilation with macros. The snippet will get substituted in wherever you use the macro and the code will get compiled like any other. There's also no technical reason whatsoever why you couldn't put a loop in a macro. –  Aleksi Torhamo Jun 15 at 19:05
1  
@AleksiTorhamo: I am aware of that, but I thought that not everything should be a macro. Like the others have said, there are better ways of handling the loop. –  Jamal Jun 15 at 19:17
add comment

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).

share|improve this answer
    
I had originally had it as directly printing the string itself, rather than each different character. I changed it to this because I figured it would be more flexible if I decided to change it later. Also, I have never heard of the puts function, thank you for linking to it. –  Magister Ludi Jun 14 at 20:47
    
@MagisterLudi: That may also depend on what they expect (if they want the simplest implementation, or if they just want to know if you know how to use loops properly). Otherwise, if that function will only print a string like that, then puts() would still be preferred. You could also call that function within main(). –  Jamal Jun 14 at 21:46
1  
Note that using puts(s) would generate two newlines in the output. Using fputs(s, stdout) would work; using putchar() in the original loop would work. –  Jonathan Leffler Jun 15 at 15:17
add comment

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)
share|improve this answer
    
Regarding the second argument in the for loop "(s)[i]", why did you put parenthesis around the s? –  Magister Ludi Jun 25 at 18:22
    
@Snowbody my post explains the problem with bare { } –  Matt McNabb Jun 25 at 21:07
    
@MagisterLudi so that [i] is applied to the whole argument and not part of it, e.g. if you went printS(p + 1) then without the parentheses it would expand to p + 1[i] –  Matt McNabb Jun 25 at 21:09
    
@MattMcNabb Could you point specifically to whre you explain it? In all the instances shown in your post it seems to me that bare {...} would behave exactly the same as the do{...}while(0)? –  Snowbody Jun 26 at 3:43
1  
@Snowbody see the if...else construct in my first code sample. It doesn't compile if printS is {.....} –  Matt McNabb Jun 26 at 3:47
show 3 more comments

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.