Simple programm to initialize class objects

[planetarydefense]

Dear Collegues,

im new at this platform. Eventhough i had some programming experiences at college before, i haven't worked on c++ for the last couple of years. Since quarantine days, i decided to refresh my knowledge and coding just random stuff. So i coded a small programm, where u can initialize simple class objects and i would like to know the critique and views of programmers on this. I guess, the the 3 methods "initializeObjects", "createObject()","void printObjects(object objects[], const unsigned int numberofobjects_)" should be implemented in the object.cpp file and i'm actually thinking of different solutions for it(I suppose it is against the basic understanding and principle of object oriented programming to define a lot of functions on global scale?). What do u think should be coded more different or altered? Thanx in advance!

//object.h
#include <iostream>
#include <string>

#ifndef object_h
#define object_h

class object
{
  private:
  std::string objectname_;
  int objectage_;

  public:
  object();
  object(const std::string objectname, const int objectage);
  ~object();
  std::string  getObjectName();
  void setObjectName(const std::string name);
  void setObjectAge(const int age);
  int getObjectAge();
};

#endif

//object.cpp
#include <iostream>
#include <string>
#include "object.h"

object::object() : objectname_(" "), objectage_(0)
{
}

object::object(const std::string objectname, const int objectage)
{
  objectname_ = objectname;
  objectage_ = objectage;
}

object::~object()
{
}

std::string object::getObjectName()
{
  return objectname_;
}

int object::getObjectAge()
{
  return objectage_;
}

void object::setObjectName(const std::string name)
{
  objectname_ = name;
}

void object::setObjectAge(const int age)
{
  objectage_ = age;
}

//main.cpp
#include <iostream>
#include <string>
#include "object.h"
#include "object.cpp"

object * initializeObjects(const unsigned int numberofobjects)
{

    object *objects=NULL;
    objects = new object[numberofobjects];

    unsigned int counter;

    for(counter=0;counter<numberofobjects; counter++)
      {
      std::string name_;
      int age_;
      std::cout<<"Please enter the name for the "<<counter+1<<". "<<"object"<<std::endl;
      std::cin>>name_;
      objects[counter].setObjectName(name_);
      name_.clear();
      std::cout<<"Please enter the age for the "<<counter+1<<". "<<"object"<<std::endl;
      std::cin>>age_;
      objects[counter].setObjectAge(age_);
      age_=0;
      };
  return objects;
}

object createObject()
{
  object newobject_;
  object *newobject = &newobject_;
  std::string name_;
  unsigned int age_;
  std::cout<<"Please enter the name of the object"<<std::endl;
  std::cin>>name_;
  newobject->setObjectName(name_);
  std::cout<<"Please enter the age of the object"<<std::endl;
  std::cin>>age_;
  newobject->setObjectAge(age_);
  return newobject_;
}

void printObjects(object objects[], const unsigned int numberofobjects_)
{
  for(unsigned int counter=0; counter<numberofobjects_; counter++)
    {
    std::cout<<objects[counter].getObjectName()<<' '<<objects[counter].getObjectAge()<<std::endl;
    };
}

int main()
{
  const std::string create_ = "init";
  const std::string exit_ = "exit";

  unsigned int numberofobjects_;

  std::cout<<"Welcome. How many objects would u like to initialize?"<<std::endl;
  std::cin>>numberofobjects_;
  std::cout<<"\n";
  object * newobject;

  newobject = initializeObjects(numberofobjects_);
  std::cout<<"\n";
  printObjects(newobject, numberofobjects_);

  delete[] newobject;
  std::cout<<"\n";

  object newobject2;
  newobject2 = createObject();
  return 0;
}

[Repeater]

I see use of pointers and manual memory management in this (new, delete); this is, broadly speaking, bad. This is something that should be done only when necessary, and in this case it's very much not necessary.

I see arrays being used here. Much the same comment; should be used only when you have to, and here, you definitely do not have to.

  // BAD
  object *objects=NULL;
   objects = new object[numberofobjects];

  // GOOD
  vector<object> objects(numberofobjects);

As you noticed alreday baout your code; your printObjects function is uncoupled from the Object definition (likewise createObject). This is very C style code. It's bad. If the Object changes, this function needs to be undated as well, even though it isn't a class member. It also relies heavily on being passed an array and a number of objects to print out. This is both binding the hands of users, and is also just dangerous.

Objects should look after themselves. Object should know about its own internals, other functions shouldn't have to care; the Object should contain a member function that will output its values to any given output stream.

#include "object.cpp"
This is a massive misunderstanding of how C++ programs are built. You get away with it in this very simple program, but as soon as you have a program in which more than one source file needs to use your objects, the program will become impossible to build. You are not meant to #include all the source code into one file. You are meant to compile object.cpp on its own, separately, and main.cpp on its own, separately, and then the linker joins them together.


What you are currently writing is C++ only in a literal meaning. You're actually programming "C with Classes". This is a common mistake beginners make, but the longer you go like this - thinking in C and making use of some C++ features - the harder you will find it to actually write good C++.

[Thomas1965]

You don't need a destructor for this simple class.
https://www.fluentcpp.com/2019/04/23/the-rule-of-zero-zero-constructor-zero-calorie/

[planetarydefense]

Thank u for the quick response. It is was on my mind the whole time that this simple programm looks more like a c code, and that i broke some main important principles by declaring the functions outside of the object.h.

The reason why i allocated space for a pointer, which points to an array of objects, is that i wanted to practice on dynamic spaces. I think that it is always the easy way to use given library functions, instead of coding the own functions and decide how and where space is gonna be used..

i appreciate your detailed explanations and i’ll consider all the necessary analysis for the future, especially using the vector class, which allocates and reserves memory automatically.

my next step is to decide how to develop the programms input commands, should i create a new command class, with subclasses? Would it be an appropriate way to use this method or could i define some constant strings and use them for all purposes?

[Repeater]

I think that it is always the easy way to use given library functions

Not just easy. Safe, fast, efficient, made to work with the other containers and algorithms, just better in so many ways.

should i create a new command class, with subclasses?

This sounds like you're wondering about the "command pattern"; http://gameprogrammingpatterns.com/command.html

Create classes if you need them. It is as much an error to go class crazy and make them for everything, as it is to never use them.

if you need command objects because they will have to be passed around to functions and objects, or you need to store them, or some other reason to have them, then you should. If you are simply going to take something a user types, some simple input, and immediately act on it, and then never need that input again, then creating a whole class for it is making your code more complicated, making your design more complicated. Use classes where it fits the design; where it makes the code easier to write, easier to understand.

[Thomas1965]

The reason why i allocated space for a pointer, which points to an array of objects, is that i wanted to practice on dynamic spaces.

You really need to forget what you learned about C.
C++ is a totally different language which has only a common syntax in common with C.
In C++ you should not use new or delete - if you really need a pointer use std::shared_ptr or std::unique_ptr. If you need a dynamic array use std::vector. It will safe you a lot of headache.

[planetarydefense]

alright, i have changed the programm code, and tried to code it more in c++ style than c.
Ive already tried to write a usefull makefile but didnt succeed till now. Would like to know your opinions. Thanx in advance!

//object.h
#include <iostream>
#include <string>
#include <vector>

#ifndef object_h
#define object_h

class object
{
  private:
  std::string objectname_;
  int objectage_;
  std::vector<object> newobjects_;

  public:
  void setObjectName(const std::string name);
  void setObjectAge(const int age);
  void addVectorObject(object& myclass);
  void deleteVectorObject(object& myclass);
  void printObjects();
  
  std::string  getObjectName();
  int getObjectAge();
  std::vector<object> getVector();

};

#endif

//object.cpp

#include <iostream>
#include <string>
#include <vector>
#include "object.h"


std::string object::getObjectName()
{
  return objectname_;
}

int object::getObjectAge()
{
  return objectage_;
}

std::vector<object> object::getVector()
{
  return newobjects_;
}

void object::setObjectName(const std::string name)
{
  objectname_ = name;
}

void object::setObjectAge(const int age)
{
  objectage_ = age;
}

void object::addVectorObject(object& myclass)
{
   this->newobjects_.push_back(myclass);
}

void object::deleteVectorObject(object& myclass)
{
  this->newobjects_.pop_back();
}

void object::printObjects()
{
  for(unsigned int counter=0; counter<this->newobjects_.size(); counter++)
    {
    std::cout<<newobjects_[counter].objectname_<<std::endl;
    std::cout<<newobjects_[counter].objectage_<<std::endl;
    };
}

//main.cpp

#include <iostream>
#include <string>
#include "object.h"
#include "object.cpp"

int main()
{
  object vectorList;
  std::string name_;
  unsigned int age_;

  do
  {
  std::cout<<"Please enter the name of the object"<<std::endl;
  std::cin>>name_;
  std::cout<<"Please enter the age of the object"<<std::endl;
  std::cin>>age_;
  object newobject_;
  newobject_.setObjectName(name_);
  newobject_.setObjectAge(age_);
  vectorList.addVectorObject(newobject_);
  }
  while(name_!="exit");

  std::cout<<vectorList.getVector().size()<<std::endl;

  vectorList.printObjects();

return 0;
}

[Repeater]

It seems that every one of your objects contains a vector of objects. This doesn't make much sense. Why does every object contain a vector of objects, in which every object in that vector will itself contain a vector of objects, and every object in that vector inside an object will itself contain a vector of objects, and so on, and so on...

This design seems very odd.

In you initial design, each object did NOT contain more objects. That made a lot more sense.

[planetarydefense]

ok, i thought the same at the beginning that it would make no sense to declare a private vector attribute, so i changed the code now, and the vectorList is defined directly in main.cpp. I think that it looks weird, but couldn't figure out better solutions.. any ideas? I thought about defining this vectorList variable in a method of object.h as well..

//object.h
#include <iostream>
#include <string>
#include <vector>

#ifndef object_h
#define object_h

class object
{
  private:
  std::string objectname_;
  int objectage_;

  public:
  void setObjectName(const std::string name);
  void setObjectAge(const int age);
  static void printObjects(const std::vector<object>& objects);

  std::string  getObjectName();
  int getObjectAge();

};

#endif

//object.cpp
#include <iostream>
#include <string>
#include <vector>
#include "object.h"


std::string object::getObjectName()
{
  return objectname_;
}

int object::getObjectAge()
{
  return objectage_;
}

void object::setObjectName(const std::string name)
{
  objectname_ = name;
}

void object::setObjectAge(const int age)
{
  objectage_ = age;
}


void object::printObjects(const std::vector<object>& objects)
{
  for(unsigned int counter=0; counter<objects.size(); counter++)
    {
    std::cout<<objects[counter].objectname_<<std::endl;
    std::cout<<objects[counter].objectage_<<std::endl;
    };
}

//main.cpp
#include <iostream>
#include <string>
#include "object.h"
#include "object.cpp"

int main()
{
  std::string name_;
  unsigned int age_;

  std::vector<object> vectorList;
  do
  {
  std::cout<<"Please enter the name of the object"<<std::endl;
  std::cin>>name_;
  std::cout<<"Please enter the age of the object"<<std::endl;
  std::cin>>age_;
  object newobject_;
  newobject_.setObjectName(name_);
  newobject_.setObjectAge(age_);
  vectorList.push_back(newobject_);
  }
  while(name_!="exit");

  std::cout<<vectorList.size()<<std::endl;

  object::printObjects(vectorList);

return 0;
}

[Repeater]

The sensible place to put the vector it is where it is now.

Onwards; don't build the Objects slowly. Build them so that they're complete from the start. Rather than creating one and then calling functions to slowly set its internal values, give it a constructor that accepts the name and age, and sets them with an initializer list.

object::object(const std::string name, const int age)
  : objectname_(name),
    objectage_(age)
{}

You can also build it directly into place in the vector, using emplace_back , rather than creating it and then copying it into the vector.

As a design note, it makes little sense for the object class to contain a function whose job is to accept a vector of object items for them to be printed out. It makes sense for an object to have a function that outputs itself, just itself. It feels like you're trying to put everything inside classes, no matter how little sense it makes. Have you been programming Java? I see that sometimes from Java programmers moving to C++.

[planetarydefense]

no, i have never programmed in Java. Frankly i had 2 courses in 2 semesters, first course in c at the first semester, and the second one in c++ at the 2. semester.
Ive programmed more examples in c, maybe thats the reason for the first code on the top. However i like c++ more.

Already changed the code and for the printObject method, ive used a *this pointer in order to print the objects. Is that an efficient way?

//object.h
#include <iostream>
#include <string>
#include <vector>

#ifndef object_h
#define object_h

class object
{
  private:
  std::string objectname_;
  int objectage_;

  public:
  object(const std::string objectname, const int objectage);
  void setObjectName(const std::string name);
  void setObjectAge(const int age);
  void printObject();

  std::string  getObjectName();
  int getObjectAge();
};

#endif

//object.cpp
#include <iostream>
#include <string>
#include <vector>
#include "object.h"

object::object(const std::string objectname, const int objectage)
{
  objectname_ = objectname;
  objectage_ = objectage;
}

std::string object::getObjectName()
{
  return objectname_;
}

int object::getObjectAge()
{
  return objectage_;
}

void object::setObjectName(const std::string name)
{
  objectname_ = name;
}

void object::setObjectAge(const int age)
{
  objectage_ = age;
}


void object::printObject()
{
    std::cout<<this->objectname_<<std::endl;
    std::cout<<this->objectage_<<std::endl;
}

//main.cpp
#include <iostream>
#include <string>
#include "object.h"
#include "object.cpp"

int main()
{
  std::string name_;
  unsigned int age_;
  std::vector<object> vectorList;

  while(name_!="exit")
  {
  name_.clear();
  age_=0;
  std::cout<<"Please enter the name of the object"<<std::endl;
  std::cin>>name_;
  std::cout<<"Please enter the age of the object"<<std::endl;
  std::cin>>age_;
  vectorList.emplace_back(name_, age_);
  }

  std::cout<<vectorList.size()<<std::endl;
  std::cout<<"\n"<<std::endl;

  for(unsigned int counter=0; counter<vectorList.size(); counter++)
    {
    vectorList[counter].printObject();
    };
return 0;
}

[Thomas1965]

Your code looks much better now.
Just a few hints:
- don't include .cpp files, just add them to the project
- on line 92 you could use the range based for loop, make sure you compile with the C++11 flag
https://en.cppreference.com/w/cpp/language/range-for
- don't compare unsigned int with size_t(line92)
- don't use std::endl, use '\n' instead. There is no need to flush cout after each output.
- it's better to pass strings as const ref -> void object::setObjectName(const std::string& name)

[Repeater]

ive used a *this pointer in order to print the objects. Is that an efficient way?

It makes no difference in terms of efficiency, but it sure looks odd. Why not just this?

void object::printObject()
{
    std::cout<< objectname_ <<std::endl;
    std::cout<< objectage_ <<std::endl;
}

As Thomas say, stop doing this:
#include "object.cpp"
Stop stop stop.

This

  for(unsigned int counter=0; counter<vectorList.size(); counter++)
    {
    vectorList[counter].printObject();
    };

is rather old-school.

In modern C++, this is a more expressive option:

for (auto const & element : vectorList)
{
  element.printObject();
};

This does the job needed:

object::object(const std::string objectname, const int objectage)
{
  objectname_ = objectname;
  objectage_ = objectage;
}

but an initialiser list would be better. It would also allow you to make the class member variables const, if its not expected that any object changes its name or age after being created.