-2
\$\begingroup\$

I am not sure whether it is the right way or am I deleting the memory properly. I have seen here some custom implementation of linked list, I want to do it simple way. Can anyone please suggest is it right or not? Should I use shared ptr, even if I do it, how to implement it correctly.

.hpp

class Camera {
    Camera()
    {}
    ~Camera()
    {}

}; typedef struct Node *NodePtr;

// Now declare the structure type itself
struct Node
{
    Camera_ x;
    NodePtr next;
};

.cpp

NodePtr head = NULL;
NodePtr* ptr = &head;
for (int n = 0; n<n1; ++n)
{
    *ptr = new Node;
    (*ptr)->x = Camera(); //storing camera objects
    ptr = &(*ptr)->next;
}

// terminate the list.
*ptr = NULL;

// walk the list,
for (NodePtr cp = head; cp->next != NULL; cp = cp->next)
{
    // doing camera functions

}
\$\endgroup\$
  • \$\begingroup\$ Are the cameras all identical objects? It's not clear what code is to be reviewed, as it looks like you have presented a sanitized version of your real code. \$\endgroup\$ – 200_success Feb 7 '17 at 17:27
  • \$\begingroup\$ I want to know is it the right way? No , there would be different camera objects. \$\endgroup\$ – Angelina Feb 7 '17 at 17:43
  • \$\begingroup\$ why not std::list? \$\endgroup\$ – pm100 Feb 7 '17 at 17:54
  • \$\begingroup\$ typedefs like NodePtr are an abomination: They just add more mental overhead. \$\endgroup\$ – Deduplicator Feb 7 '17 at 18:55
  • \$\begingroup\$ Check for cp != NULL instead of cp->next != NULL. \$\endgroup\$ – G. Sliepen Feb 7 '17 at 20:15
2
\$\begingroup\$

if you dont want to use the c++ standard data structures then here are a few comments

for (int n = 0; n<n1; ++n)
{
    *ptr = new Node;
    (*ptr)->x = Camera(); //storing camera objects
    ptr = &(*ptr)->next;
}

First, you are storing local (stack) camera objects in your linked list. This is asking for trouble if you try to do more with the list than use it all in one function. The stack memory will be released on function exit

You never delete the Node objects that you create. This is bad. You need a Node destructor plus you should encapsulate the list in a class that has a destructor too.

Or just use std::list

You also refer to a type Camera_, this is not defined - I assume this is a typo

| improve this answer | |
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.