Ricky wrote:[color=blue]
> thanks for the replysir.
> i have made this code so far in declaring the NFA. In fact i do know what am
> i supposed to do , i just cant get it right.
> please have a look at the following code and tell me am i in the right way,
> or can i do the covertion from this structure, and if possible (not trying
> to bother you) one kind of solution.
> thanks much[/color]
I will review your code, but I have no clue what a NFA is. Perhaps you
should not use acronyms or abbreviations, but spell them out.
[color=blue]
> struct TLink
> {
> int linkNode;
> int stringMax;
> int string[5];
> };[/color]
Why are there 5 int values for the string?
The '5' is a _magic_ number and should be a named constant, such
as:
const unsigned int MAX_NUMBERS_IN_STRING = 5;
Also, do you want signed or unsigned integers. In many communications
scenarios, numbers are unsigned to allow for more range. If the
number is not negative, declare it as unsigned.
[color=blue]
> struct Node
> {
> int isFinalState;
> int max_links;
> TLink Link[5];
> } NFA[5];[/color]
1. The member "isFinalState" implies a boolean (true / false) type.
You should name it that way:
bool isFinalState;
2. What is with the number '5'? Magic number again. Convert to
a named constant. See above.
[color=blue]
> struct DLink
> {
> int meCilinLidhet;
> int stringNo;
> int node[5];
> };[/color]
1. What is with the number '5'? Magic number again. Convert to
a named constant. See above.
2. What is the difference between a TLink and a DLink?
Your names are not very descriptive.
[color=blue]
> struct NodeD
> {
> int stringNode;
> int isFinalState;
> int max_links;
> DLink Link[5];
> } DFA[5];[/color]
1. What is with the number '5'? Magic number again. Convert to
a named constant. See above.
1. The member "isFinalState" implies a boolean (true / false) type.
You should name it that way:
bool isFinalState;
[color=blue]
>
> int max_states;
> int maxNode;[/color]
Do these variables _need_ to be global?
[color=blue]
> void NFA_input()
> {
> int i, j, k, f;
> cout<<"\nHow MAny states does the NFA has =";
> cin>>max_states;[/color]
1. Since the number of states is dynamic, you may want to
consider allocating them from dynamic memory (i.e. using
'new') and using a dynamic container, such as a vector
or list.
2. You don't check for invalid input. What happens if
the user enters "apple" or "3.14159"?
(I believe the input operation will set the stream's
fail bit or bad bit).
[color=blue]
> for (i=0; i<max_states; i++)
> {
> cout<<"\nIs State q["<<i<<"] final State ? (yes=1, No=0)";
> cin>>NFA[i].isFinalState;[/color]
Or:
unsigned int yes_no;
NFA[i].isFinalState = yes_no == 1;
What happens when max_states is 10? You only have 5
elements allocated in your array.
[color=blue]
> cout<<"\nHow many transitions does this State have ?";[/color]
I suggest you flush the output buffer before requesting input:
cout.flush();
[color=blue]
> cin>>NFA[i].max_links;[/color]
What happens if the number of transitions is 30?
You only have 5 allocated.
What happens when the user enters a negative number, such
as -2? You did allow that by specifying max_links as an
int and not an unsigned int.
[color=blue]
> for (j=0; j<NFA[i].max_links; j++)
> {
> cout<<"\nThe "<<j+1<<" transition is linking state q["<<i<<"] with
> state =";
> cin>>NFA[i].Link[j].linkNode;
> cout<<"\nHow many strings make this transition=";
> cin>>NFA[i].Link[j].stringMax;
> for (k = 0; k < NFA[i].Link[j].stringMax; k++)
> {
> cout<<"\nEnter those strings=\n";
> cout<<"delta["<<k+1<<"]= ";
> cin>>NFA[i].Link[j].string[k];[/color]
Possible array overflow: you only have 5 allocated.
What happens when there are 6, or zero?
[color=blue]
> }
> }
> }
> }
>[/color]
[snip -- printing function]
[color=blue]
> int main()
> {
> clrscr();[/color]
This is not a standard C++ function. Do you really need the
screen cleared? The program will be more portable with out it.
For example, in some operating systems, you could redirect the
output of your program into a file. How does clearing the
screen work with redirecting the output?
[color=blue]
> char line[80]="-------------------------------------------------";
> NFA_input();
> cout<<"\nTranitions of the NFA are: \n";
> cout<<line;
> print_NFA();
> cout<<"\n"<<line;
> cin.get(); cin.ignore();
> return 0;
> }[/color]
Summary
-------
I believe you don't have an efficient data structure. Your
implied requirements allow a dynamic amount of states and
transitions. Fixed sized arrays are very bad for dynamic
quantities of elements. Your I/O is not checked for
boundary conditions or stream failures. Your naming
conventions could use improvement for better readability.
You need to use pointer when dealing with dynamic structures.
If you insist on arrays, place a pointer in your structure
and allocate the array at run-time. You will need to
remember the size of the array because the C++ language
does not provide a sizeof() operation for dynamic arrays.
A std::vector is much safer, and also handles dynamic
expansion.
Your nodes have a common set of members (maybe methods
too), you should factor out the commonalities into
a base class:
struct BaseState
{
bool is_final_state;
unsigned int max_states;
};
struct Node
: public BaseState
{
TLink * links;
};
struct DNode
: public BaseState
{
int stringNode;
DLink * links;
};
--
Thomas Matthews
C++ newsgroup welcome message:
http://www.slack.net/~shiva/welcome.txt
C++ Faq:
http://www.parashift.com/c++-faq-lite
C Faq:
http://www.eskimo.com/~scs/c-faq/top.html
alt.comp.lang.learn.c-c++ faq:
http://www.raos.demon.uk/acllc-c++/faq.html
Other sites:
http://www.josuttis.com -- C++ STL Library book
