Problem with User defined functions!

[Skhiburdhurs]

I have a problem converting some of my codes to a function like the adding a value at a queue. this is my original code and it is working fine. And please tell me if there are bad programming practices that i am doing.

#include <iostream>
using namespace std;
struct Node
{
    int data;
    Node *next;
};
int main()
{
    Node *ptr,*head,*tail,*temp;
    int choice,nodesize,push;

    cout<<"What is the size of the queue?: ";
    cin>>nodesize;
    for(int x=0;x<nodesize;x++)
    {
        if(x==0)
        {
            ptr=new Node;
            ptr->next=NULL;
            ptr->data=0;
            head=ptr;
            temp=ptr;
        }
        else
        {
            ptr=new Node;
            ptr->next=NULL;
            ptr->data=0;
            tail=ptr;
            temp->next=ptr;
            temp=ptr;
        }
    }
    temp=head;
    do
    {
        cout<<"Options"<<endl;
        cout<<"1.)Add"<<endl;
        cout<<"2.)Delete"<<endl;
        cout<<"3.)Exit"<<endl;
        cout<<"Choice: ";
        cin>>choice;

    if(choice==1)
    {
        cout<<"Enter the number to be added: ";
        cin>>push;
        temp=head;
        while(temp!=NULL)
        {
            if(temp->data==0)
            {
                temp->data=push;
                break;
            }
            if(temp->next==NULL&&temp->data!=0)
            {
                cout<<"Queue OverFlow!"<<endl;
            }
            temp=temp->next;
        }
    }
    else if(choice==2)
    {
        if(head->data==0)
        {
            cout<<"QUEUE EMPTY! "<<endl;
        }
        head=head->next;
        ptr=new Node;
        ptr->next=NULL;
        ptr->data=0;
        tail->next=ptr;
        tail=ptr;
    }
    else if(choice==4)
    {
        temp=head;
        while(temp!=NULL)
        {
            cout<<temp->data<<" ";
            temp=temp->next;
        }
    }

    }while(choice!=3);

    return 0;
}

And here is when i try to make a function about adding. P.S.(it is not finished yet until the deleting) I my program is crashing just when i choose to add. Please help me

#include <iostream>
using namespace std;
struct Node
{
    int data;
    Node *next;
};
void createNodes(int nodesize,Node *head,Node *tail,Node *temp);
void add(int push,Node *head,Node *tail,Node *temp);
int main()
{
    Node *ptr,*head,*tail,*temp;
    int choice,nodesize,push;

    cout<<"What is the size of the queue?: ";
    cin>>nodesize;
    createNodes(nodesize,head,tail,temp);
    do
    {
        cout<<"Options"<<endl;
        cout<<"1.)Add"<<endl;
        cout<<"2.)Delete"<<endl;
        cout<<"3.)Exit"<<endl;
        cout<<"Choice: ";
        cin>>choice;

    if(choice==1)
    {
        cout<<"Enter the number to be added: ";
        cin>>push;
        add(push,head,tail,temp);
    }
    if(choice==4)
    {
        temp=head;
        while(temp!=NULL)
        {
            cout<<temp->data<<" ";
            temp=temp->next;
        }
    }

    }while(choice!=3);

    return 0;
}

void createNodes(int nodesize,Node *head,Node *tail,Node *temp)
{
    Node *ptr;
    for(int x=0;x<nodesize;x++)
    {
        if(x==0)
        {
            ptr=new Node;
            ptr->next=NULL;
            ptr->data=0;
            head=ptr;
            temp=head;
        }
        else
        {
            ptr=new Node;
            ptr->next=NULL;
            ptr->data=0;
            tail=ptr;
            temp->next=ptr;
            temp=ptr;
        }
    }
}

void add(int push,Node *head,Node *tail,Node *temp)
{
  temp=head;
  while(temp!=NULL)
  {
      if(temp->data==0)
      {
          temp->data=push;
          break;
      }
      if(temp->next==NULL&&temp->data!=0)
      {
          cout<<"Queue OverFlow!"<<endl;
      }
      temp=temp->next;
  }
}

[MiiNiPaa]

Your head,tail,temp pointers are uninitialized and point to the random memory. After createNodes function they still point to random memory because you have changed copies of it in your function and not variables themselves.

please tell me if there are bad programming practices that i am doing.

1) using namecpace std; It will hit you when you least expect it.
2) You are manually managing your list and exposes all internals to the user. Not only your code is not reusable, but it is easy to make a mistake when using it, you cannot create multiple queues easily, and it is not exactly a queue, more like simple linked list.
You should make your queue a class, encapsulatig all internal working in it, exposing only public interface for inserting and deleting nodes. Look how it is done in standard containers. It is actually a good idea to mimic their interface to have compatibility with loads of templated code already written: code reusability.

[Skhiburdhurs]

so as for the code? should i make a global variable of the three pointers and remove them in the function parameters? and i am still familiarizing my self with using classes so i am just practicing with structures. thanks for the help sir! (i also did not get the "using namespace std; will hit me when i least expect it) =). anyways thank you once again!

and i thought that i made a queue because of the first-in-first-out concept. how can i make this to be an exact queue?

[MiiNiPaa]

(i also did not get the "using namespace std; will hit me when i least expect it

Tell me why folowing code outputs 57 and not 68 as I intended (as you can see it swaps values then increments them).

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19

#include <iostream> using namespace std; void swap(int *a, int *b) { int temp = *a; *a = *b; *b = temp; ++*a; ++*b; } int main() { int a = 7; int b = 5; swap(a,b); cout <<a<<b; }

57

should i make a global variable of the three pointers and remove them in the function parameters

You should pass parameters by reference if you intend to change them.

For starters unite your pointers in one struct. Make createNode to tane number of nodes parameter and return struct with all pointer initialized (here you should return by copy). This will be easier to modify to member functon access. By the way your queue should dynamically grow, so it should essentually have no max size. Otherwise it would be more efficient to just use an array (circular buffer).

[Skhiburdhurs]

to tell you honestly, i dont know why the output of your example program did not undergone any changes. my wild guess is that line 17 should be swap(&a,&b)?? thank you again for your time. =)

[MiiNiPaa]

line 17 should be swap(&a,&b)

Exactly! But it compiles. And not issues any warnings. It just calls std::swap which was dumped in global namespace by using namespace std; . Now imagine that in even moderately large program (about 2000 lines, ~5 files). You might spend days trying to find it. Now remove using namespace std;. It will give you an error and point you to that line, so you can spot your mistake.

And that only one example. Can you tell me how many common words are defined in std namespace? What worse, everything might work, until you add another standard header and compiler will decide that standard function is way better match than yours. Will you suspect the code which was working perfectly before? How long will it take you to spot such mistake?

So don't use it. Always be sure which function/class do you use.

[Skhiburdhurs]

Haha! Now I have a bit of understanding of what it is all about. All my teacher told us is just to use it without even telling us what it is. Thank you my good sir! :)