0

I've made a simple code, that generates some random users and process upon them to form some trivial business charts. The code was working perfectly with the list STL, but I had to implement the linkedlist myself, that I found the problem.

The program runs perfectly .. it even produces the perfect desired output but it crashes after the last line of code, I tried to figure out where the problem is but failed.

Here is my code

#include "Class.h"
#include <stdlib.h>
class Node
{
    private:
        customer data;
        Node *next;
    public:
        /*Nodes constructors*/
        Node(){next=nullptr;}
        Node(customer X)
        {
            data=X;
            next=nullptr;
        }
        /*Data setters and getters*/
        void set_Data(customer X)
        {data = X;}
        customer get_Data()
        {return data;}
        /*next setters and getters*/
        void set_next(Node * X){next=X;}
        Node* get_next(){return next;}


};

class List
{
    private:
        Node * head;
        Node * tail;
        int counter;
    public:
        /*Constructors*/
        List(){head=nullptr;tail=head;counter=0;}
        /*Checks if the list is empty*/
        bool isEmpty()
        {
            if (head==nullptr)
                return true;
            return false;
        }
        /*front manipulation*/
        void add_Front(customer item)
        {
            if (isEmpty())
            {
                head = new Node(item);
                tail = head;
                counter++;
            }
            else{
            Node * nw= new Node(item);
            nw ->set_next(head);
            head=nw;
            counter++;
            }
        }
        void pop_Front()
        {
            if (isEmpty())
                return;
            if (head==tail)
            {
                delete head;
                delete tail;
                counter--;
                return;
            }
            Node * temphead=head;
            head=head->get_next();
            delete temphead;
            counter--;
        }
        /*End Manipulation*/
        void add_End(customer X)
        {
            if(isEmpty()){
                add_Front(X);
                counter++;}
            else
            {
                Node * temp=new Node(X);
                tail->set_next(temp);
                tail=temp;
                counter++;
            }
        }

        /*freeing the whole list*/
        void Clear()
        {
            while (!isEmpty())
                pop_Front();
        }

        /*Destructor*/
        ~List(){Clear();}

        /*Extras*/
        int get_Size(){return counter;}
        customer get_Front(){return head->get_Data();}
        customer get_End(){return tail->get_Data();}

};

    using namespace std;
bool generate_pie(int slices_number,string slices_names[],int slices_values[],string title);
bool Age_Pie(List Data,int AgeCategory);
int main()
{
    List Data;
    int numberofelements;
    cout<<"How many customers you wanna randomly generate? : ";
    cin >> numberofelements;
    srand(time(NULL));
    for (int i=0; i<numberofelements; i++)
    {
        customer temp;
        temp.random_customer();
        Data.add_Front(temp);

    }
    Age_Pie(Data,1);
    return 0;

}

bool Age_Pie(List Data,int AgeCategory)
{
    int Product_Percentage[6]={0};
    int tempsize= Data.get_Size();
    for (int i =0; i<tempsize; i++)
    {

        customer temp = Data.get_Front();
        Data.pop_Front();
        if (temp.get_age()==AgeCategory)
        {
            switch (temp.get_interrest())
            {
            case 1:Product_Percentage[0]++;break;
            case 2:Product_Percentage[1]++;break;
            case 3:Product_Percentage[2]++;break;
            case 4:Product_Percentage[3]++;break;
            case 5:Product_Percentage[4]++;break;
            }
        }
        else
            Product_Percentage[5]++;
        Data.add_End(temp);
    }

    string Products[]={"Product 1","Product 2","Product 3","Product 4","Product 5","Didn\'t choose"};
    generate_pie(6,Products,Product_Percentage,"The Age Category "+to_string(3)+" Chose these products");


}


bool Product_Pie(List Data, int Chosen_product)
{
    int AgeCategory_Percentage[5]={0};
    int Datasize = Data.get_Size();
    for(double i=0; i<Datasize-1;i++)
    {
        customer dummy = Data.get_Front();
        int temp_interrest = dummy.get_interrest();
        int temp_agecat = dummy.get_age();
        Data.pop_Front();
        if (temp_interrest==Chosen_product)
        {

        switch (temp_agecat){
        case 0:AgeCategory_Percentage[0]++;break;
        case 1:AgeCategory_Percentage[1]++;break;
        case 2:AgeCategory_Percentage[2]++;break;
        case 3:AgeCategory_Percentage[3]++;break;
        }
        }
        else
            AgeCategory_Percentage[4]++;

        Data.add_End(dummy);
    }
    string Ages[]={"18 To 25","26 To 40","41 To 61","Above 60","Not Chosen"};

    generate_pie(5,Ages,AgeCategory_Percentage,"Product #"+to_string(Chosen_product+1)+" Statistics");
    return true;
}


bool generate_pie(int slices_number,string slices_names[],int slices_values[],string title)
{
    /* the function takes the number of pie slices and its names with values
     * and the pie chart title
     * writes an HTML with JS that creates the chart
     * using the googlecharts API*/

    ofstream html;
    html.open ("report.html");
    html << "<html>\n\t<head>\n"
            "\t\t<!--Load the AJAX API-->\n"
            "\t\t<script type=\"text/javascript\" src=\"https://www.gstatic.com/charts/loader.js\"></script>\n"
            "\t\t<script type=\"text/javascript\">\n\n"
            "\t\t// Load the Visualization API and the corechart package.\n"
            "\t\tgoogle.charts.load('current', {'packages':['corechart']});\n\n"
            "\t\t// Set a callback to run when the Google Visualization API is loaded.\n"
            "\t\tgoogle.charts.setOnLoadCallback(drawChart);\n\n"
            "\t\t// Callback that creates and populates a data table,\n"
            "\t\t// instantiates the pie chart, passes in the data and\n"
            "\t\t// draws it.\n\t\tfunction drawChart() {\n\n"
            "\t\t\t// Create the data table.\n"
            "\t\t\tvar data = new google.visualization.DataTable();\n"
            "\t\t\tdata.addColumn('string', 'Category');\n"
            "\t\t\tdata.addColumn('number', 'Percentage');\n"
            "\t\t\tdata.addRows([\n";

            for (int i=0;i<slices_number;i++)
                html << "\t\t\t\t['"<<slices_names[i]<<"', "<<slices_values[i]<<"],\n";

            html <<"\t\t\t]);\n\n"
            "\t\t\t// Set chart options\n"
            "\t\t\tvar options = {'title':'"<<title<<"','width':400,'height':300,is3D: true,};\n\n"
            "\t\t\t        // Instantiate and draw our chart, passing in some options.\n"
            "\t\t\tvar chart = new google.visualization.PieChart(document.getElementById('chart_div'));\n"
            "\t\t\tchart.draw(data, options);\n"
            "\t\t  }\n"
            "\t\t</script>\n"
            "\t</head>\n\n"
            "\t<body>\n"
            "\t\t<!--Div that will hold the pie chart-->\n"
            "\t\t<div id=\"chart_div\"></div>\t\n"
            "</body>\n"
            "</html>";

    return true;
}

You can just skip the generate_pie function as it is working already. I feel like it's not appropriate to post the whole code but I'm getting started with Stackoverflow. Thanks.

Has QUIT--Anony-Mousse
  • 76,138
  • 12
  • 138
  • 194
Michel Mina
  • 105
  • 2
  • 2
  • 6
  • You're passing `List` objects by value, but you did not adhere to [the rule of 3](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three), You probably won't notice anything until the destructor for those `List` objects are being called. That's when all havoc will break loose. – PaulMcKenzie Apr 23 '16 at 21:49
  • 2
    So the code runs perfectly except that it crashes. – Pete Becker Apr 23 '16 at 21:49

1 Answers1

1

Your pop_Front is broken in the case where there is one element in the list:

    void pop_Front()
    {
        if (isEmpty())
            return;
        if (head==tail)
        {
            delete head;
            delete tail;
            counter--;
            return;
        }
        Node * temphead=head;
        head=head->get_next();
        delete temphead;
        counter--;
    }

you try to delete the same address twice. What it should be is:

    void pop_Front()
    {
        if (isEmpty())
            return;
        if (head==tail)
        {
            delete head;
            head = tail = nullptr;  // optional
            counter--;
            return;
        }
        Node * temphead=head;
        head=head->get_next();
        delete temphead;
        counter--;
    }

Broken demo: http://ideone.com/afg7se Working demo: http://ideone.com/mJ489F

--- EDIT: Per Paul McKenzie's comment

Age_Pie(Data,1);

This passes a copy of your list to the Pie function, which by default is going to be all the same nodes pointing to the same places. That means that on destruction of the temporary copy, it will deallocate the entire of your list, so you end up deleting everything multiple times.

You need to either pass by reference/pointer, or you need to implement a copy constructor or operator= for your List class.

--- EDIT 2:

Updated the live demo to include a copy operator:

    /*copy operator*/
    List& operator=(const List& rhs)
    {
        Clear();
        Node* temp = rhs.head;
        while (temp) {
            add_End(temp->get_Data());
            temp = temp->get_next();
        }
        return *this;
    }
kfsone
  • 23,617
  • 2
  • 42
  • 74
  • It's just `delete tail` misses a comment: "Make sure the beast is really dead!" :-] – bipll Apr 23 '16 at 21:08
  • but there still a deallocation problem, idk where it comes from, still with this fix the code crashes at the end :S – Michel Mina Apr 23 '16 at 21:42
  • @MichelMina Your `List` class is totally broken due to missing assignment operator and copy constructor. You are passing `List` objects by value, and without those functions, your code will not work. – PaulMcKenzie Apr 23 '16 at 21:44
  • @PaulMcKenzie yeah I figured it out, it fixed a part but still there are other faults, I am trying to figure out what else is needed, can you help further? – Michel Mina Apr 23 '16 at 21:47
  • @PaulMcKenzie I removed all of that portion of the code while making an MVCE, edited my answer. Good catch. – kfsone Apr 23 '16 at 21:48
  • ahaa I understood this and modified it to be bool Age_Pie(List *Data,int AgeCategory) { int Product_Percentage[6]={0}; int tempsize= Data->get_Size(); for (int i =0; iget_Front(); Data->pop_Front(); Now the problem is here Age_Pie(*Data,1); its not working as I should implement how to use * i guess? any Idea how to implement this? – Michel Mina Apr 23 '16 at 21:54
  • @MichelMina See the most recent EDIT and changes to the live demo. That should fix it for you. But, be aware, passing by value means a deep copy which is expensive. – kfsone Apr 23 '16 at 21:55
  • @kfsone any Idea how can I edit my code to pass by reference ? – Michel Mina Apr 23 '16 at 21:57
  • @MichelMina change the fingerprint of Age_Pie, for example, to `bool Age_Pie(List& Data,int AgeCategory)` (note the &) – kfsone Apr 23 '16 at 22:08
  • Done that, and the call should be Age_Pie(Data,5) or Age_Pie(&Data,5)? – Michel Mina Apr 23 '16 at 22:37
  • `Age_Pie(Data, 5)`; you don't need to do anything special at the call site to pass by reference. – kfsone Apr 23 '16 at 22:37