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 sometimes hear people say how they can distinguish between good and bad coders of a certain programming language or even programming in general simply by looking at the code they write.

I'm relearning C, and I was just wondering whether my program follows best practices in terms of my commenting, the way I set it up, and generally the way I've incorporated the for and while loops along with macros (whether it be better to use a goto statement here, or a while loop instead of a for loop there, etc).

#include <stdio.h>


#define SIZE (sizeof(words[0])/sizeof(words[0][1]))
#define ROWS 10
#define COLUMNS 20


int main()
{
//The maxmimum letters in a words is 20 and the max words is 10
char words[ROWS][COLUMNS]={0},character;
int count_1=0,count_2=0,i,j;

printf("Enter something:  ");
character=getch();
putch(character);
while(character!='\r')
{
words[count_1][count_2]=character;
//Allows the next index to contain the value of the next inputted character
count_2++;
if(character==' ')
{
//index of inputted characters reset to 0
count_2=0;
//Allows the next row in array to be changed as one row will already satisfy a word
count_1++;
}

character=getch();
putch(character);
}

printf("\n");
//Starts from the last word
for(i=count_1;i>=0;i--)
{
for(j=0;j<SIZE;j++)
{
if(words[i][j]!=0)
printf("%c",words[i][j]);
}
}
}

I know the next bit is a bit opinionated, but does my code give an idea of where I'm at with logic and programming as a whole, or is the first part just some kind of myth (i.e. you can't judge a person by such a small piece of code)?

Note: I deliberately avoided the use of functions and strings. I got this task from C Programming: A Modern Approach, and I'm only attempting to solve a programming task simply by using only the features I've learned thus far in the book.

share|improve this question
4  
Does your original code not have indentation? You should add it here, so that your code is clearer. –  Jamal Mar 18 at 22:09
    
Does this program work as intended? It isn't reversing the sentence for me. –  syb0rg Mar 18 at 23:10
    
@syb0rg It echoes the input until you enter '\r'. Note that '\r' is difficult to type via keyboard on some machines (on Windows, pressing the Enter key gives '\n'). –  ChrisW Mar 18 at 23:48
    
@ChrisW I know; I modified the program to use \n instead, and it didn't reverse my input sentence. –  syb0rg Mar 18 at 23:49
1  
@syb0rg If I enter abc def ghi . it echoes that and then prints .ghi def abc which is what I'd expect from inspection. –  ChrisW Mar 18 at 23:54
show 1 more comment

3 Answers 3

up vote 10 down vote accepted

I sometimes hear people say how they can distinguish between good and bad coders of a certain programming language or even programming in general simply by looking at the code they write.

I think that's true, to some extent. How else (except by the code they write) would you judge a programmer's ability?

Distinguishing programmers from their code is just the beginning though: see this answer for example.

Professional-level programming might also be categorized e.g. with Programmer Competency Matrix

I was just wondering whether my program follows best practices

Not all the best practices; it could still be improved, for example:

  • Avoid macros (#define)
  • Format it well (use indentation)
  • Give good user output (better than "Enter something: ")
  • Use subroutines (don't put everything in main)
  • Don't overflow your input buffer (e.g. if the end-user enters more than 10 words, or words more than 20 characters long)
  • Use library methods appropriately (e.g. not printf("%c" when you want to output a whole string and not just one character)
  • Use good identifier names (e.g. I'd prefer row and col, or x and y, or imax and jmax, instead of count_1 and count_2)
  • Be flexible in handling input correctly (e.g. I'd prefer arbitrary-length words and arbitrary-many words, which would require you to use a heap-based list or array of variable/infinite-length strings, instead of a fixed-size array)

I know the next bit is a bit opinionated, but does my code give an idea of where I'm at with logic and programming as a whole

I think so. The code matches where you said you're at, which is that you are "relearning C".

Working a 40-hour week makes about 2000 hours per year.

So an adult professional programmer will have spent many 1000s of hours of practice and studying.

share|improve this answer
    
The only thing here that makes me wonder is how to "use subroutines here"... the entire code can be written as (bare functionals here only) "char* words[WORD_MAX]; int i; for( i = 0; i < WORD_MAX; i++ ) if ( scanf( "%ms", &words[i] ) == EOF ) break; for( i--; i >= 0; i-- ) printf( "%s ", words[i] );" - I don't see much space for subroutines here... –  vaxquis Mar 19 at 3:10
add comment
  • Prefer to have logically-unrelated variables once per line, which helps improve readability and allows you to add separate comments if necessary.

    From Code Complete, 2d Edition, p. 759:

    With statements on their own lines, the code reads from top to bottom, instead of top to bottom and left to right. When you’re looking for a specific line of code, your eye should be able to follow the left margin of the code. It shouldn't have to dip into each and every line just because a single line might contain two statements.

    This line of yours, for instance:

    int count_1=0,count_2=0,i,j;
    

    would look like this (grouped):

    int count_1=0, count_2=0;
    int i, j;
    

    or this (un-grouped):

    int count_1=0;
    int count_2=0;
    int i;
    int j;
    

    For i and j in particular, you should instead have them initialized inside the for loop statement if you're using C99 (which you should). If you're not, then declare them in the lowest scope possible, right above the outer loop.

  • Prefer to add whitespace as needed, primarily between operators and after commas. Going back to the aforementioned line:

    int count_1=0,count_2=0,i,j;
    

    At a glance, that could easily look like one long variable name. It's quite unreadable, and the line length savings isn't worth it.

    That line would now look like this:

    int count_1 = 0, count_2 = 0, i, j;
    

    Now it's clear that you have four separate int variables. Make sure you do this in every instance, not in just this particular line.

  • @ChrisW has already mentioned this, but it's well worth repeating: indent your code. There's zero indentation here, so it makes me think that you've missed one of the basics of writing code. There are plenty of resources that mention how indentation should be done in a given language.

  • In C, a function should have a void as a parameter if it has no others:

    int main(void) {}
    
  • Unlike in C++, where return 0 at the end of main() is implied, it should be present in C.

share|improve this answer
    
My guess is that the OP isn't using an IDE; many programmers use a source code editor, which understands the source code syntax and indents semi-automatically. –  ChrisW Mar 19 at 0:11
    
What's wrong with "int count_1 = 0, count_2 = 0; int i, j;"? Splitting the variables to logically-related groups is one thing, but declaring i and j on separate lines is an overkill in my opinion... –  vaxquis Mar 19 at 1:48
    
@vaxquis: Although this was mainly used as an example, that is true, and I meant to add that i and j can be initialized in the for loop statement (assuming C99). I'll do that shortly. –  Jamal Mar 19 at 1:50
    
yup, and that's the preferred way of doing it so that their scope is limited to the loop (which is the usual case here); –  vaxquis Mar 19 at 1:53
    
@vaxquis: Indeed. Thanks for reminding me! –  Jamal Mar 19 at 1:54
add comment

You, mainly:

  • chose non-optimal algorithm do to the task at hand,
  • used non-standards-conformant functions/declarations (getch, #define for constants),
  • didn't use indentation,
  • used an unusual naming scheme for variables,
  • used an initializer for an array that should be used for 1-D arrays only (size mismatch otherwise),
  • didn't use scanf() or any other sensible (buffer-overflow protected) input

So yes, that gives the reader an idea of your coding skills and experience.

As for the braces, as Linus Thorvalds (AFAIR) once said, "They are matter of style. Everybody who does them in a way different than me has none." (obvious irony here) - Brace Wars are over for me, I know the One True Brace may be nice, but I still omit extraneous braces in short snippets like this one.

Let us say we'd like to use char-by-char input here. The following is a rewritten version of your program: compare this with yours to understand what the differences are.

// hopefully we're getting somewhere...

#include <stdio.h>

const int WORD_MAX = 10,
          CHAR_MAX = 20;

int main() {
  char words[WORD_MAX][CHAR_MAX];
  char ch;
  int word_idx = 0, char_idx = 0;
  memset( words, 0, sizeof(char)*WORD_MAX*CHAR_MAX );

  printf( "Enter a phrase: " );
  ch = getchar();
  putchar(ch);

  while( ch != '\n' ) {
    words[word_idx][char_idx] = ch;
    if( ch == ' ' ) {
      char_idx = 0;
      word_idx++;
    } else
      char_idx++;
    ch = getchar();
    putchar(ch);
  }
  words[word_idx][char_idx] = ' ';

  printf( "Reversed input: " );

  for( int i = word_idx; i >= 0; i-- )
    printf( "%s", words[i] );

  putchar( "\n" );

  return 0;
}

However, the example above still has an important problem, which is that the end-user can overrun the input array by entering words that are too long, or by entering too many words.

That (failing to detect 'invalid' data entered by the user) is usually considered a major (unacceptable) bug in commercial-grade software, so you should probably need to fix that too.

Here's another version, which fixes that, and which is even simpler because it handles words (strings) instead of individual characters; note that since the char* used for strings and alloc'ed by scanf points to data is no longer after the output, the end of the code should be the place to free() the memory alloc'ed; the free() call is not explicitly needed here, but the required code is still attached in comments for the reference.

// GNU CC required || use a port of libc with scanf supporting 'm' format

#include <stdio.h>

const int WORD_MAX = 10;

int main() {
  char* words[WORD_MAX];

  printf( "Enter a phrase: " );

  int i;
  for( i = 0; i < WORD_MAX; i++ )
    if ( scanf( "%ms", &words[i] ) == EOF )
      break;
    else
      printf( "%s ", words[i] );
  i--;
  // int j = i;

  printf( "\nReversed input: " );

  for( ; i >= 0; i-- )
    printf( "%s ", words[i] );
  // for( ; j >= 0; j-- )
  //   free(words[j]);
  putchar( '\n' );

  return 0;
}

Like a wise man once said, "learn through discovery". Try to find why and how you can improve.

share|improve this answer
2  
Something wrong with your example is that it still allows the user to overrun the array if the input is too long. –  ChrisW Mar 19 at 2:44
    
that's why I stated "didn't use scanf() or any other sensible input" and then "let us say we'd like to use..." but true, this shouldn't be hanging like this, fix in a sec. –  vaxquis Mar 19 at 2:45
    
If the input is too long you could truncate the input, split the input, print an error message and abort; or use the heap to support 'infinite' length input. –  ChrisW Mar 19 at 2:49
2  
You have a memory leak, and it might be nice to mention which standards have been "broken." (In particular, I'm eyeing the m modifier in scanf`.) –  Corbin Mar 19 at 4:29
6  
@vaxquis What happens when 6 months from now someone pulls the input reading into a function, the run time of the program is extended to more than 1 run, and suddenly they have a mysterious memory leak? (A very contrived situation, but the principle is the same--bad habits bite you down the road.) Trade offs between academic purity and real-world concerns are certainly valid, but I believe you should explicitly point out when those trade offs are being made rather than silently putting them into example code. The OP probably isn't as knowledge as you and doesn't know the drawbacks. –  Corbin Mar 19 at 17:13
show 7 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.