Can someone help me understand this example program

This is a sample program that my tutor gave me. However, I have very much trouble understanding the second if statement. Would somebody plz be able to explain it to me, in not too difficult language

Code:

#include <stdio.h>        //Handles the keyboard input and screen output
#include <process.h>      //Handles DOS system commands ("cls", "pause", etc.)
#include <conio.h>        //Handles: getch(), getche().
#include <string.h>       //Handles string functions.

int main(void)
{
   //This part declares variables used by the entire program
   int Arabic=0;          //Integer declaration to store Arabic number
   char Roman[30];        //String declaration to store a Roman number

   //This part declares variables used and reads Arabic numbers
   int counter = 7;       //Declares the count of digits in Roman numbers
   char T_Roman [7] = "MDCLXVI"; //Lists all digits of Roman numbers
   int T_Arabic [7] = {1000, 500, 100, 50, 10, 5, 1};//Values of Roman digits
   int length=0;          //Stores the length of an input Roman number
   int in, list;          //Declare operational loop counters
   int last_in, last_list;//Store the previous digits being considered

   textcolor(6);          //This defines color of the writing for "cprintf"
   cprintf("This program converts Roman numbers into Arabic numbers.");//Colour

   //This part reads Raman numbers with "scanf" and "printf"
   printf("\nType a valid Roman number (e.g. MCMXCIX) up to 30 chars long: ");
   scanf("%s", Roman);    //Inputs a Roman number
   length = strlen(Roman);
   strupr(Roman);
   printf("The Roman number |%s| is %d characters long.", Roman, length);
   fflush(stdin);         //flushes the input buffer

   //This part contains the algorithm of converting Roman numbers into Arabic:
   in=0;
   last_in = length + 1;
   last_list = length + 1;
   while (in < length) //Loop along the chars of the Roman number
   {
      for(list=0; list < counter; ++list) //Loop along the Roman symbols
      {
         if (Roman[in] == T_Roman[list])
         {
           /*printf ("\nRoman digit |%c| matched to |%c| = %4d.",
                     Roman[i], T_Roman[j], T_Arabic[j]);  */
           if ((last_in == (in-1)) && (last_list > list))
                Arabic = Arabic - 2*T_Arabic[last_list];
           last_in = in;
           last_list = list;
           Arabic = Arabic + T_Arabic[list];
         }
      }
   in=in+1;
   }
   //This part displays the results
   printf("\nThe Roman number |%s| converts into Arabic %d.", Roman, Arabic);

   //This part finishes the program
   puts ("\nNormal end of program. BYE.");
   //gotoxy(20,7);      //Position at "character no", "line no"
   printf("\n");
   system("pause");     //DOS "Pause" command (for user to look at results)
}

Code:

if ((last_in == (in-1)) && (last_list > list))
  Arabic = Arabic - 2*T_Arabic[last_list];

Ooh, nifty. What this does is handle special cases such as CD by checking to see if the last converted character is farther along the search string, and thus lower in value, than the current character. That way the program knows to subtract the necessary amount so that the addition later will result in the correct value.

>This is a sample program that my tutor gave me.
Get a new tutor. Seriously, this code is A.W.F.U.L. On top of that, your tutor shows, in both the commenting and the code, a fundamental misunderstanding of his/her implementation and the C language in general. Even in my dark days I didn't write code this poorly, and I rank myself as one of the worst programmers ever back then.

Whats A.W.F.U.L stand for?

Amazingly, Woefully, ****ing Unbelievably Loathsome. But I'm open to suggestions for alternatives.

Originally Posted by Prelude

Code:

if ((last_in == (in-1)) && (last_list > list))
  Arabic = Arabic - 2*T_Arabic[last_list];

Ooh, nifty. What this does is handle special cases such as CD by checking to see if the last converted character is farther along the search string, and thus lower in value, than the current character. That way the program knows to subtract the necessary amount so that the addition later will result in the correct value.

>This is a sample program that my tutor gave me.
Get a new tutor. Seriously, this code is A.W.F.U.L. On top of that, your tutor shows, in both the commenting and the code, a fundamental misunderstanding of his/her implementation and the C language in general. Even in my dark days I didn't write code this poorly, and I rank myself as one of the worst programmers ever back then.

What is a better alternative to writing it this way? So far Iv'e been following in his footsteps by writing code like he does

>What is a better alternative to writing it this way?
The following is much better. It's portable, much safer, everything is well defined, there's a comment marking the confusing part of the algorithm, and I've stripped away unnecessaries.

Code:

#include <ctype.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

static const char T_Roman[] = "MDCLXVI";
static const long T_Arabic[] = {1000, 500, 100, 50, 10, 5, 1};

int main ( void )
{
  long Arabic = 0;    /* Arabic decimal conversion */
  char Roman[BUFSIZ]; /* Roman numeral input string */
  int in;             /* Index for Roman */
  int last = 0;       /* Last converted index */

  printf ( "Enter a Roman number (e.g. MCMXCIX): " );
  fflush ( stdout );

  if ( fgets ( Roman, sizeof Roman, stdin ) == NULL ) {
    fprintf ( stderr, "Invalid input\n" );
    return EXIT_FAILURE;
  }

  /* Strip the trailing newline */
  Roman[strcspn ( Roman, "\n" )] = '\0';

  for ( in = 0; Roman[in] != '\0'; in++ ) {
    char *find = strchr ( T_Roman, toupper ( Roman[in] ) );

    if ( find != NULL ) {
      int list = find - T_Roman;

      /*
        Special case for numbers like CD
        Example (C = 100, D = 500): (100 - (2 * 100)) + 500 == 400
      */
      if ( last > list )
        Arabic -= 2 * T_Arabic[last];

      last = list;
      Arabic += T_Arabic[list];
    }
    else {
      fprintf ( stderr, "Unrecognized roman numeral\n" );
      return EXIT_FAILURE;
    }
  }

  printf ( "The Roman number |%s| converts into Arabic %ld\n", Roman, Arabic );
  getchar();

  return EXIT_SUCCESS;
}

>So far Iv'e been following in his footsteps by writing code like he does
This is my biggest beef with the quality of most programming instructors. I feel that they should be experts in what they teach because they will be role models to their students. If the teacher is inadequate then the student will usually be inadequate as well unless they're lucky enough to find a better teacher.

Now, I'll tell you why the code your teacher gave you is bad, so that you can avoid such things in your own programs.

>#include <stdio.h> //Handles the keyboard input and screen output
This comment shows a lack of understanding about C I/O streams. The C standard doesn't require either a keyboard or a screen, yet stdin and stdout are still guaranteed to work properly. Remember that stdin is not always the keyboard and stdout is not always the screen.

>#include <process.h> //Handles DOS system commands ("cls", "pause", etc.)
This shows a lack of understanding about the implementation AND the C standard library. Presumably your teacher thinks that the system function is declared in process.h, or process.h somehow allows the system function to use shell commands in the command line prompt. While process.h does support nonstandard functions like spawn* and exec*, system is a standard function declared in stdlib.h. This header is not needed for the program and can be safely omitted.

>#include <conio.h> //Handles: getch(), getche().
I hate this header. I hate how teachers teach it and beginners rely on it. conio.h is a nonstandard header, so its contents are unpredictable on different implementations. It should be avoided, period. There are better alternatives if you need the functionality that conio.h sometimes supports.

On top of this, the comment is incomplete. The code also makes use of textcolor, gotoxy, and cprintf. All of those are well known conio.h functions. They are also well known to not work on most implementations. The only conio library I know of that is complete as expected is Borland's implementation, which happens to be the original creator of conio.h as I understand.

>int main(void)
With the poor quality of this code, I was very surprised to see the correct definition of main.

>//This part declares variables used by the entire program
>//This part declares variables used and reads Arabic numbers
I see no point for these comments. They just add clutter.

>int Arabic=0;
On some systems and int may not be large enough. The input string allows a long number, but an int is only required to hold values up to 32767. This isn't conducive to reliable software. A long would be a slightly better bet while remaining standard.

>//Integer declaration to store Arabic number
This is a silly comment. We know it's an integer and the name suggests an arabic number. Therefore, we can construe from the context precisely what the comment tells us. It's redundant and should be removed, or changed to be more informative.

>char Roman[30];
30 is a very arbitrary limit, I would like to see the reasoning behind it in a comment. I prefer to use BUFSIZ (declared in stdio.h) as my buffer sizes when a more appropriate size is unavailable. For strings this works nicely except in the case of extra long strings, which is a special case that requires special attention anyway.

>//String declaration to store a Roman number
Once again, the comment is redundant and should be removed or replaced with something more informative.

>int counter = 7; //Declares the count of digits in Roman numbers
Poorly named and poorly commented, perhaps n_digits or max_digits would be better. Your teacher had the right idea in saving the value in a variable so as to avoid hardcoded literals, but there are better ways to do it than this. One of them is a macro:

Code:

#define N_DIGITS 7

Another is a const variable rather than a non-const variable that cna be changed:

Code:

const int n_digits = 7;

And finally, this particular object is better suited outside of the local scope. Perhaps a static global variable? But in the end, the object is unnecessary as the list of roman numerals is a string. The null terminator can be used to mark the end of the string rather than an index.

>char T_Roman [7] = "MDCLXVI";
Very bad style. The number of roman numerals is 7 and the array is set to a size of 7, but using a string literal as an array initializer means that one extra character is there, '\0'. Now, this isn't really a problem because the null character is discarded if the array isn't large enough to hold it, but the null character is useful for marking the end of the array. So your teacher made his job harder as well as introduced annoying warnings to the compilation process.

>//Lists all digits of Roman numbers
Well, duh! That's exactly what the declaration said. There's even an initializer telling us what digits those are. So the comment not only reiterates what the declaration says, it also fails to give the same amount of information. I would toss this comment without hesitation.

>//Values of Roman digits
This suffers from the same problem as the previous comment. Redundant and it doesn't even give as much information as the declaration. Scrap it as well, it's just clutter.

>int length=0;
This is a good idea. Saving the length of a string lets you avoid calling strlen often, this is a valid performance concern. But I don't see the reason for initializing it to 0 unless the compiler (such as Borland) gives gratuitous warnings about uninitialized variables. This apparently isn't the case as the next declaration fails to initialize the variables. This screams inconsistency to me, and inconsistency is the devil.

>//Stores the length of an input Roman number
Finally, a comment that serves a purpose. Of course, a better name for the variable would make this comment unnecessary...

>int in, list; //Declare operational loop counters
What operations? What loops? Let's get some more information in these comments, or name the variables better.

>int last_in, last_list;//Store the previous digits being considered
My only valid complaint with this is that only one variable is needed. With a little thought, your teacher could have removed the need for last_in.

>textcolor(6); //This defines color of the writing for "cprintf"
This is nonportable, probably won't exist except on Borland compilers, and what does 6 mean anyway? Sure, I know that 6 is brown, but do you? For that matter, what if the implementation chooses to use a different color for 6? I don't recall it being standardized. Color in console mode programs is ostentatious anyway, I see no reason for it. If you want good looks then write a GUI, the command line is better suited for speed, power and flexibility. Nifty looks come a distant second.

>cprintf("This program converts Roman numbers into Arabic numbers.");//Colour
cprintf is nonstandard and nonportable. Fortunately, many implementions of conio.h will provide it, so it isn't as bad as some other functions. And the comment really bites.

>//This part reads Raman numbers with "scanf" and "printf"
This is a hint at the poor quality of the code. We know that roman numerals aren't numbers as defined by C, so you would have to read them as characters or strings. scanf is a poor choice for both. getchar and fgets are more appropriate, respectively. I also see a spelling error. If you teacher didn't bother to check his comments, how can I trust the code that is being commented?

>printf("\nType a valid Roman number (e.g. MCMXCIX) up to 30 chars long: ");
Starting with a newline isn't necessary, but ending with one usually is because it serves the purpose of flushing the output stream. If you omit it then you must call fflush on the stream for your code to be portable. The limit is off by one because scanf reads n - 1 characters and leaves room for a null character at the end. Also, telling the user what their limit is shows that the programmer was considerate enough to irritate the user by setting a limit in the first place, and can we really count on the user counting the characters they type to stay within the limit? No, they'll enter what they want and expect the program to handle it gracefully. Which the program does not do, as exemplified by the next line.

>scanf("%s", Roman); //Inputs a Roman number
scanf isn't very good at reading string data. It just causes too many problems because the author of the code does not expect the behavior. On top of this, using an unqualified %s is incredibly dangerous because if the user enters more than 29 characters then scanf will start writing to memory that the program may or may not own. If the memory is owned then an unrelated variable will probably get trashed, and if the memory is not owned, a segmentation violation is likely to occur. Neither of these are good, but because it's undefined behavior we can reasonably hope that something good comes of it. Of course, Prelude's law of brown stuff hitting the fan states that when you have undefined behavior, the very worst thing that could happen will, and then it will cause a chain reaction resulting in total meltdown. Not a good thing, in other words.

>strupr(Roman);
This is a nonstandard function. Fortunately, most implementations support converting a string to upper case. Unfortunately, they all have different names. Better to write your own or use toupper during processing.

>printf("The Roman number |%s| is %d characters long.", Roman, length);
This is an unnecessary print unless it is meant for debugging. In that case, it should be conditionally compiled as such. Also, because the string isn't ended with a newline, fflush should be called on stdout.

>fflush(stdin);
I said stdout, not stdin. There's quite a difference because fflush is only defined for output streams. Because stdin is an input stream, the result is undefined behavior and Prelude's law of brown stuff hitting the fan applies. Of course, your teacher could have made a typo, meaning to write stdout but actually writing stdin. I know I do that on a regular basis because I correct fflush ( stdin ) so much I have it on the brain.

>//flushes the input buffer
When the comment and the code agree, you can be sure that the author meant what he wrote. So your teacher meant to flush the input stream and (probably) unknowingly caused undefined behavior. The need to flush stdin could have been avoided by not using scanf for string input, but he could also portably and correctly have done it like this:

Code:

int ch;
while ( ( ch = getchar() ) != EOF && ch != '\n' )
  ;

Though I disapprove of flushing stdin because it's antisocial. What if you discard valid input that the user was expecting to take effect after the current input? I would be annoyed by having to re-enter my commands and data, and so would they.

>last_in = length + 1;
>last_list = length + 1;
There's really no need for this, he could have set both to be 0 and it would have worked just as well. This is one of the reasons why length could be removed from the program completely.

>while (in < length) //Loop along the chars of the Roman number
Okay, but if you look at where in is initialized and incremented, this loop could be replaced with a for loop and been more conventional. As it is the loop suggests that it was written by a beginner who didn't know that a for loop was equivalent to the

Code:

i = 0
while ( i < limit ) {
  /* Do stuff */
  i = i + 1;
}

construct. Now, this is another reason why length can be removed: Because strings in C end with '\0', we can simply test for that character:

Code:

for ( in = 0; Roman[in] != '\0'; in++ )

This is much easier and much cleaner. I wonder if your instructor knew about it or just chose not to use it for some odd reason.

>for(list=0; list < counter; ++list) //Loop along the Roman symbols
This is the consequence of having an array that was too small for the initializer. Now your teacher had to save the number of symbols and test for it explicitly instead of using the '\0' testing convention for strings. Do you think that the code would be cleaner if this convention were followed for both loops?

Code:

for ( i = 0; Roman[i] != '\0'; i++ ) {
  for ( j = 0; T_Roman[j] != '\0'; j++ ) {
    /* ... */
  }
}

I think so. It's definitely more consistent, and consistency is easier to follow than wildly fluctuating constructs that perform the same fundamental operation.

>/*printf ("\nRoman digit |%c| matched to |%c| = %4d.", Roman[i], T_Roman[j], T_Arabic[j]); */
This can be removed. It's clearly a debug statement that your teacher wasn't using if he left it commented out. Commented out statements add clutter, kill them unless you have a need for them in the near future.

>if ((last_in == (in-1)) && (last_list > list))
This is a prime example of being too clever for your own good. The idea is nifty (as I said before), but the implementation is stupid and overly complex. Because only last_list is really needed, the first part of the && can be removed, thus changing the statement to:

Code:

if ( last_list > list )

This is cleaner and easier to understand. A novice and easily work out what it does and from that, construe why it does it whereas before they would probably get bogged down in the overuse of parentheses and confusion about the two parts of the logical test.

>Arabic = Arabic - 2*T_Arabic[last_list];
This is a tricky hack, yet your instructor chose not to give you so much as a comment even attempting to describe it. Because of this I wonder if he even understands it himself. I also wonder why he didn't use the compound assignment operator a -= b instead of a = a - b. Judging by the complexity of the problem, it's about time to introduce those to his students, but that's just my personal opinion and it doesn't count as a complaint.

>last_in = in;
>last_list = list;
As state previously, only last_list is needed to do the job, last_in just adds complexity to a simple algorithm.

>Arabic = Arabic + T_Arabic[list];
Another personal opinion, but

Code:

Arabic += T_Arabic[list];

is easier to follow and more efficient.

>}
If the if statement is entered, then a match has been made. There's no point in continuing the inner loop when no duplicates are in the search string. A break here would be a good idea. The code would look like this (with my changes):

Code:

for ( i = 0; Roman[i] != '\0'; i++ ) {
  for ( j = 0; T_Roman[j] != '\0'; j++ ) {
    if (Roman[i] == T_Roman[j]) {
      if ( last > j )
        Arabic -= 2 * T_Arabic[last];

      last = j;
      Arabic += T_Arabic[j];

      break;
    }
  }
}

Easier to read, cleaner, less complex, and faster.

>//This part displays the results
Unnecessary comment, we can figure this out from the context.

>printf("\nThe Roman number |%s| converts into Arabic %d.", Roman, Arabic);
Again a leading newline and not a trailing newline. Apparently your instructor has never written code anywhere except DOS. The trailing newline or a call to fflush ( stdout ) is needed to flush the stream so that users can see it.

>//This part finishes the program
Also unnecessary, why not have something more informative such as how the code finishes the program, or why it does so?

>puts ("\nNormal end of program. BYE.");
I think your teacher lucked out here, the leading newline flushes the output stream and then puts also does it again after it prints the string. So all is well again.

>//gotoxy(20,7); //Position at "character no", "line no"
gotoxy is another of those conio.h functions that probably won't be available except on Borland compilers. And why does 20 mean character number and 7 mean line number? Where did those values come from and why are they hard coded? You can remove this line, not because it's commented out and not used, but because it's so disgusting.

>system("pause");
This is all kinds of wrong. First, while system is a standard function and thus portable, the argument you pass to it cannot be portable because it relies on the system command interpreter. If you move the code to a different system, the command interpreter will probably be different (or not available!) and system("pause") could do something completely different or break.

Second, system is unsafe because it doesn't verify that the pause program is the one supplied by the system. It may be a malicious program bent on wreaking havoc.

Third, system is sloooooooow. Dreadfully, painfully, agonizingly sloooooooow. This isn't really a problem if you call it once, like here, but system should really be avoided in favor of better alternatives. My code shows one of those alternatives. It requires the user to hit return, but it's portable and does the job admirably with no noticeable inconvenience to the user.

>}
He defined main correctly and then forgot to return a value. This is more undefined behavior. 0 is a portable return value for success, and two macros in stdlib.h can also be used; EXIT_FAILURE and EXIT_SUCCESS. Any time a function says it will return a value, it should.

Now you see why I called it A.W.F.U.L. Almost every single line had either a grievous error, misunderstanding or design problem. But it gets even better. There's one more problem that depends on whether he was using the old C standard or the new C standard. If he was using C89 (as is likely) then the // comments are illegal. If he was using C99 then I have no problem, but the chances of your teacher even knowing that such a standard exists (judging by his code), much less using it correctly, are slim indeed.

prelude is pretty good at this programming malarcky