Please tell me why for loop never executes

[gautiergator]

For some reason the for loop never executes. Any idea why?
The program is supposed to convert a user entered roman numeral to decimal. I can place cout<<"Test"; in the loop or after it and neither will ever execute.
I have all of the code below.

romantype.h

#ifndef ROMANTYPE_H
#define ROMANTYPE_H
#include <cstring>

/*
 * No description
 */
class romanType
{
	public:
		// class constructor
		romanType();
		// class destructor
		~romanType();
		//printDecimal
		void printDecimal() const;
		//printDecimal
		void printRoman() const;
		//convertRoman
		void convertRoman();

        char myRoman[];
        int myDecimal;
};

#endif // ROMANTYPE_H

romantype.cpp

#include "romantype.h" // class's header file
#include <iostream>
#include <stdio.h>
#include <cstring>


//const static int M = 1000;
//const static int D = 500;
//const static int C = 100;
//const static int L = 50;
//const static int X = 10;
//const static int V = 5;
//const static int I = 1;


using namespace std;

// class constructor
romanType::romanType()
{
	char myRoman[] = "";
	myDecimal = 0;
}

// class destructor
romanType::~romanType()
{
	// code here
}

//Print Decimal
void romanType::printDecimal() const
{
    cout<<"The Decimal value is "<<myDecimal<<endl;                      
}

//Print Roman
void romanType::printRoman() const
{
    cout<<"The Roman numeral is "<<myRoman<<endl;                      
}

//Convert Roman
void romanType::convertRoman()
{
    int counter;
    int length;
    char charNow[1] = "";
    char charPrev[1] = "";
    int valueNow = 0;
    int valuePrev = 0;
    
    length = strlen(myRoman);
    
    for(counter=length; counter<1; counter--)
    {
         charNow[1] = myRoman[counter];
         cout<<myRoman[counter]<<endl;
         
         if (charNow == "M")
         {valueNow = 1000;}
         else 
              if (charNow == "D")
                 {valueNow = 500;}
         else
              if (charNow == "C")
                 {valueNow = 100;}
         else
              if (charNow == "L")
                 {valueNow = 50;}
         else
              if (charNow == "X")
                 {valueNow = 10;}
         else
              if (charNow == "V")
                 {valueNow = 5;}
         else
              if (charNow == "I")
                 {valueNow = 1;}
                                                                    
         if (valueNow < valuePrev)
            {myDecimal = myDecimal - valueNow;}
         else
            {myDecimal = myDecimal + valueNow;}

         valuePrev = valueNow;
    };//end for                          
}

and finally main.cpp

#include <cstdlib>
#include <iostream>
#include "romantype.h"

using namespace std;

char userIn;
romanType romanConverter;

int main(int argc, char *argv[])
{
    cout<<"Please enter a Roman Numeral: ";
    cin>>romanConverter.myRoman;
    romanConverter.convertRoman();
    romanConverter.printDecimal();
    romanConverter.printRoman();
    system("PAUSE");
    return EXIT_SUCCESS;
}

[Paul McKenzie]

For some reason the for loop never executes. Any idea why?

Why not print out the value of the loop counter variables? It isn't that difficult to figure out why a loop never gets done -- the only reason is that the condition to do the loop is false.

But first:

class romanType
{
	public:
//...
        char myRoman[];

This is illegal C++. Arrays must have a constant size defined. Please turn on the ANSI switch on your compiler, as this is not legal C++ code. Therefore that line "char myRoman[]" is meaningless with respect to the C++ language. It should be:

class romanType
{
	public:
//...
        char myRoman[3];  // or some constant expression

Bottom line -- some sort of constant expression must be in those brackets when declaring an array without initialization.

Look at the next piece of code you wrote:

romanType::romanType()
{
	char myRoman[] = "";

This declares a local char array that consists of a single NULL character. Then the local is destroyed. Is this what you wanted, or did you really want to initialize the (bogus) char myRoman[] that your class has defind as a member earlier?

    length = strlen(myRoman);
    
    for(counter=length; counter<1; counter--)

Now given the code above, how would you proceed in debugging it? It is very simple -- the middle condition tells you what must be true for the loop to continue. Obviously, counter is less than 1 when the loop is supposed to execute.

So what set counter? Look at the first condition (counter=length). So where is length determined? Look at the line above (length = strlen(myRoman)). So obviously, length must be less than 1 (more than likely 0). This means that strlen(myRoman) is 0, meaning that myRoman has no characters.

Regards,

Paul McKenzie

[gautiergator]

Thanks for the reply.

What I was actually trying to do with myRoman[] was using a string. I have tried using #include<cstring> and #include<string.h> but still can not figure out how to use a string type in C++. When I print the length of myRoman before the loop it gives me the correct length and I can print myRoman and it gives me exactly what I typed in before the loop.

However taking your advice I changed myRoman[] to myRoman[10] and changed char myRoman[] = "" to char myRoman[10] = " "; Changes shown below. I even changed the for loop to start at 10, go until counter is less than 1 and decrement by one each time but the execution still never gets into the loop at all. And it still does not ececute any code that is placed after the loop. I am stumped.

// class constructor
romanType::romanType()
{
	char myRoman[10] = "         ";
	myDecimal = 0;
}

//Convert Roman
void romanType::convertRoman()
{
    int counter;
    int length;
    char charNow[1] = "";
    char charPrev[1] = "";
    int valueNow = 0;
    int valuePrev = 0;
    
    length = strlen(myRoman);
    cout<<myRoman<<endl;
    cout<<length<<endl;
    
    for(counter=10; counter<1; counter--)
    {
         charNow[1] = myRoman[counter];
         cout<<myRoman[counter]<<endl;
         
         if (charNow == "M")
         {valueNow = 1000;}
         else 
              if (charNow == "D")
                 {valueNow = 500;}
         else
              if (charNow == "C")
                 {valueNow = 100;}
         else
              if (charNow == "L")
                 {valueNow = 50;}
         else
              if (charNow == "X")
                 {valueNow = 10;}
         else
              if (charNow == "V")
                 {valueNow = 5;}
         else
              if (charNow == "I")
                 {valueNow = 1;}
                                                                    
         if (valueNow < valuePrev)
            {myDecimal = myDecimal - valueNow;}
         else
            {myDecimal = myDecimal + valueNow;}

         valuePrev = valueNow;
    };//end for

[GCDEF]

romanType::romanType()
{
	char myRoman[10] = "         ";
	myDecimal = 0;
}

As was pointed out, you've declared another version of myRoman that exists only for the life of the function. If you're trying to give a value to the class member myRoman, you'll need to use strcpy and don't redefine myRoman in constructor. Since the class member myRoman still hasn't been initialized, strlen could return any ramdom value there.

[Paul McKenzie]

Thanks for the reply.

What I was actually trying to do with myRoman[] was using a string.

In C++, you have std::string for that. There is no need to be using non-standard C++ to do something as simple as create a string variable. You also don't need char arrays, but for some strange reason, new programmers are never taught this very simple concept.

#include <string>  // not <cstring> or <string.h>, but <string>

class romanType
{
    //...
    std::string myRoman;
};

Second, you didn't read my second comment more carefully:

// class constructor
romanType::romanType()
{
	char myRoman[10] = "         ";
	myDecimal = 0;
}

This creates a brand new local variable call "myRoman". What happens to local variables when the function returns? They are destroyed. That "myRoman" variable has nothing to do with the class member called "myRoman". It is a seperate and distinct variable. So all you did in that code was spin wheels and did nothing, since the "myRoman" in that function is gone by the time the constructor is finished.

As to std::string, it is the way C++ handles string types and is much easier and more intuitive to use than C-style strings. For example, you've totally messed up the way you compare C-style strings:

void romanType::convertRoman()
{
//...
    char charNow[1] = "";
//...         
         if (charNow == "M")
}

What exactly are you doing there? Why declare an array of 1 character? What's wrong with just declaring a single char? Second, even if the array were more than 1 character, you don't compare C-style strings with "==", You compare them with strcmp().

With std::string, it is much easier:

std::string charNow;
charNow = "M";
//...
if ( charNow == "M" ) // and it is
{
   // do something
}

Regards,

Paul McKenzie

[gautiergator]

Ahhh..gotcha

I was hearing but not listening.

I got it all to work now thanks to your help. My new code is below. This was my very first assignment in Data Structures with C++ class and I have only been in it for less than a week. I have never seen C++ before so this was a shock for me since I am a Delphi and VB programmer. All of the visual stuff has spoiled me I think. Thanks again for you help.

#include "romantype.h" // class's header file
#include <iostream>
#include <stdio.h>
#include <string>


using namespace std;

// class constructor
romanType::romanType()
{
	myRoman = "";
	myDecimal = 0;
}

// class destructor
romanType::~romanType()
{
	// insert your code here
}

//Print Decimal
void romanType::printDecimal() const
{
    cout<<"The Decimal value is "<<myDecimal<<endl;                      
}

//Print Roman
void romanType::printRoman() const
{
    cout<<"The Roman numeral is "<<myRoman<<endl;                      
}

//Convert Roman
void romanType::convertRoman()
{
    int counter = 0;
    int length;
    std::string charNow;
    int valueNow = 0;
    int valuePrev = 0;
    
    length = myRoman.size();
    
    for(counter=length; counter>0; counter--)
    {
         charNow = myRoman.substr(counter-1, 1);

         if (charNow == "M")
            {valueNow = 1000;}
         else 
              if (charNow == "D")
                 {valueNow = 500;}
         else
              if (charNow == "C")
                 {valueNow = 100;}
         else
              if (charNow == "L")
                 {valueNow = 50;}
         else
              if (charNow == "X")
                 {valueNow = 10;}
         else
              if (charNow == "V")
                 {valueNow = 5;}
         else
              if (charNow == "I")
                 {valueNow = 1;}
                                                                     
         if (valueNow < valuePrev)
            {myDecimal = myDecimal - valueNow;}
         else
            {myDecimal = myDecimal + valueNow;}

         valuePrev = valueNow;
    };//end for                          
}