0
\$\begingroup\$

I have created two simple classes: resource and container. below is the full code. actually container keeps resources. and in the beganing it will get all the resources and store them in datalist of resources. The other function is simply prints out all the resources in container. Please review my code and see if its ok just review it and let me know what are the wrongs and rights in the code, thanks

////
// Created by noone on 18/8/21.
//
#include <iostream>
#include <cstring>
#define Max 10
class resource{
private:
public:
    int id;
    char name[10];

    resource(int id,char name1[10]):id(id)
    {

        std::memcpy(name,name1,Max);
    }
    resource& operator= (const resource& arr2){
        if (this == &arr2){ return *this; } //make sure you aren't self-assigning
        //same as copy constructor from here
        id=arr2.id;
        std::memcpy(name,arr2.name,Max);
        return *this;
    }


};
class container
{
public:
    container(resource *new_resource,int i)
    {
            size=0;
            size=i;
            datalist=(resource *)malloc(sizeof(resource)*i);
            for(int x=0;x<i;x++)
            {
                datalist[x].id=new_resource[x].id;
                std::memcpy(datalist[x].name,new_resource[x].name,std::strlen(new_resource[x].name));

            }


    }
    void print_all_resources()
    {
        std::cout<< "total "<<size<<" resources found"<<std::endl;
        for(int i=0;i<size;i++)
        {
            std::cout<<"Printintg first resource"<<i+1<<std::endl;
            std::cout<<datalist[i].name << " | "<< datalist[i].id<<std::endl;
        }
    }

private:
    resource *datalist;
    int size;
};

int main()
{
    resource resource1(10,"abc");
    resource resource2(20,"xyz");
    resource r[]={resource2,resource1};

    container c(r,2);
    c.print_all_resources();

}
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

If you used a string, you would not need to write the operator= at all. You would not need the copy constructor either, which is missing from your code.

You ask

can u [sic] show it my resource constructor expect two parameters if I use new then how to call the constructor of resource with two parameters? datalist=new resource[i]; <<---error , resource constructor expects two parameter:id,name1 what do u say about this?

If you needed to use new, you would pass the arguments like this:
resource* p = new resource(name,id);

However, I think what you really want here is just a vector<resource>. You don't need to write a container class that stores a dynamically allocated array and its size — there are containers ready available in the standard library.

Here is my take on it:

  • there is no Max.
struct resource {
   std::string name;
   int id;
   };

std::ostream& operator<< (std::ostream& o, const resource& res);  // write this.

You could write using resource_container = std::vector<resource>; but your code can use any kind of container freely with resource, and algorithms don't care which container you used, so this is not a type to be defined at all.

Note that resource doesn't need the copy constructor, destructor, or assignment operator, since the string already takes care of itself properly. There is no special housekeeping needed: this is the rule of zero.

I don't need a constructor, either. Since all the data members are public, you can just use the aggregate initializer. This is OK since this is, in fact, a simple bag of data, with no special checking that the values are in agreement or anything like that — any values for the individual fields will work.

Note that with a vector you don't call new at all. You just push_back.

I think the problem you were asking about is that making an array requires a default constructor. Your use of malloc avoids the issue but does not fix anything: creating a "blank" array of n resources leaves them uninitialized in your code, or default initialized in fixed code, but those array elements are not really ready to use, and you need to assign over them. With the vector you don't have an array of n items in some blank state; you don't have an item at all until you are ready to add it! See the difference? With a vector, at no time do you have extra unused elements at the end of the array; they are all valid values.

vector<resource> Ct;
Ct.push_back({"name",27});
Ct.puch_back({"another_with_long_name",42});

You could also create a vector all at once:

vector<resource> Container2 {
   {"name",27}, {"another_with_long_name",42}
};

But if you don't plan on adding more entries afterwards, you don't even need a vector. A plain array will do. As you have in the original for r, but without re-copying the values, and even doing it all at compile-time for zero run-time overhead!

constexpr resource r[] {
   {"name",27}, {"another_with_long_name",42}
};

Notice how this is exactly the same as for the vector example.

You want a function ready-made for printing the entire collection?
Note that it doesn't matter what kind of container you use — this works for the plain array, and vector, and many others.

template <typename R>
void print_all_resources (const R& collection)
{
    for (auto& r : collection) {
    std::cout << r.name << " | "<< r.id << '\n';
    }
}
\$\endgroup\$
12
  • \$\begingroup\$ If you needed to use new, you would pass the arguments like this: resource* p = new resource(name,id) I do not want single object contained in p array. What I wanted was multiple resources to be accessed by p +i where i is index of a resource in p array \$\endgroup\$
    – user786
    Aug 19, 2021 at 4:49
  • \$\begingroup\$ The point I am using not using string and vector so I can create my own container instead of relying on stl containers. Although I like to use the iterator from stl embedded in my container class as struct \$\endgroup\$
    – user786
    Aug 19, 2021 at 4:51
  • \$\begingroup\$ resource* p = new resource(name,id)` I do not want single object contained in p array. What I wanted was multiple resources to be accessed by p +i where i is index of a resource in p array. Need to allocate all received resources and allocate by calling resource constructor so allocate array of sizes i.of resources. \$\endgroup\$
    – user786
    Aug 19, 2021 at 4:57
  • \$\begingroup\$ @user786 I explained this in my Answer, "making an array requires a default constructor...." \$\endgroup\$
    – JDługosz
    Aug 19, 2021 at 6:37
  • \$\begingroup\$ Ok. Can u please tell can i attach my stl iterator with my container class \$\endgroup\$
    – user786
    Aug 19, 2021 at 7:12
1
\$\begingroup\$

Your code is a weird mix of C and C++. It's essentially C with classes (which is to say, it's not very good C++ code).


  1. Use constexpr instead of #define to define constants. constexpr retains type information.

  2. Use std::string instead of a char array.

  3. Implement the rule of 0-3-5. Although you have defined a copy assignment operator for resource, you haven't defined a copy constructor, or any move constructors/assignment operators. You also haven't defined any constructors/assignment operators for container.

  4. Instead of manually managing memory through malloc, consider using std::vector. It does all the heavy lifting for you and is safer.

  5. Even if you want to avoid std::vector and manage memory yourself (to learn), you should use new and delete instead of malloc and free.

  6. Speaking of free, I don't see it. You're leaking memory since you don't deallocate memory that you allocated through malloc. Ideally, you'd want to create a destructor that would call free when the container object goes out of scope (gets destroyed). This is a very important concept in C++: Resource Acquisition Is Initialization (RAII)

  7. Instead of defining a method called print_all_resources, a more idiomatic approach would be to overload operator<<.

  8. Instead of creating a temporary array and passing it to the container constructor, you can use initializer list to pass an arbitrary amount of resources to the constructor.

    container(std::initializer_list<resource> rsc)
    {
       resources.insert(resources.end(), rsc);
    }
     private:
        std::vector<resource> resources;
    

    and call it like this:

    resource r1;
    resource r2;
    container c({r1, r2});
    
  9. Prefer to use std::copy over memcpy.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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