Is it ok for 2 classes to have field pointing eachother?

Question

So I have classes User and Job:

class User {
 string user_name;
 Job* job;
};

class Job {
 string job_type;
 int salary;
 User* user;
};

Are there any problems with this kind of design? I have a large number of Jobs and Users and i want a fast access to a user's job or the user who is taking a job. Is it ok that these classes have fields poiting to each other?

Fyi your example is probably *begging* for [`std::shared_ptr`](http://en.cppreference.com/w/cpp/memory/shared_ptr) and [`std::weak_ptr`](http://en.cppreference.com/w/cpp/memory/weak_ptr) usage rather than toting around raw pointers like you are. Click those provided links to find out more about them. — WhozCraig, Nov 20 '16 at 05:08
I would use a radically different approach with an SQL database and three tables "user", "job_type" and "job", the latter establishing a relationship between the other two such that any user can have 0 to n jobs. In your C++ code, you would no longer have individual classes `User` and `Job` but immutable query-result classes resulting directly from appropriate JOINs. Any modifications happen via more or less explicit UPDATEs or INSERTs. I've found such architectures infinitely easier to use, precisely because they free you from this exact pointer problem. — Christian Hackl, Nov 20 '16 at 11:12

score 3 · Accepted Answer · answered Nov 20 '16 at 05:04

3

It's fine. You need to be careful to manage the pointers carefully to avoid leaks, for example. But it's perfectly reasonable.

answered Nov 20 '16 at 05:04

Waxrat

2,075
15
13

I upvoted but I would be more concerned about dangling pointers than memory leaks. If you are concerned about memory leaks it implies these pointers are owning raw pointers that have been allocated dynamically which is not generally a good idea. – Chris Drew Nov 20 '16 at 07:51
You could use smart pointers. Then you'd need to worry about leaks that could come from the circular references. That's where weak pointers come in. http://stackoverflow.com/questions/12030650/when-is-stdweak-ptr-useful – Waxrat Nov 20 '16 at 20:55

Chris Drew · Answer 2 · 2016-11-20T07:55:55.973

As long as User does not own the Job and Job does not own the User then this is fine.

For example, perhaps you create collections of Jobs and Users up-front and then want to create associations between them:

#include <string>
#include <vector>

class Job;

class User {
    std::string user_name;
    Job* job;
  public:
    explicit User(const std::string& user_name) : user_name(user_name) {}
    void setJob(Job& job) { this->job = &job; }
};

class Job {
    std::string job_type;
    int salary;
    User* user;
  public:
    Job(const std::string& job_type, int salary) : job_type(job_type), salary(salary) {}
    void setUser(User& user) { this->user = &user; }
};

void recruit(User& user, Job& job) {
  user.setJob(job);
  job.setUser(user);
}

int main() {
    auto jobs = std::vector<Job>{ {"Tinker", 10'000}, {"Tailor", 20'000}};
    auto users = std::vector<User> {User{"George"}, User{"Percy"}};
    recruit(users[0], jobs[1]);
    recruit(users[1], jobs[0]);
}

As long as the collections of Users and Jobs are destroyed at the same time there is no danger of dangling pointers. It might be preferable if the pointers were const pointers.

But if your intention is some sort of ownership then a smart pointer would be preferred.

score 0 · Answer 3 · answered Nov 20 '16 at 07:45

It is usual situation in the world of oop. For example, typical GUI architecture uses such approach in similar way, like that:

Widget knows about its parent. It keeps pointer to parent.
Compound widget knows about its z ordered children(widget)

I understand, that such example is far from your post. It just shows case.

I don't know exact architecture of your application. But maybe Job class should not know about user? It decreases extra relations and simplifies management of memory ( instead usage for better control shared and weak pointers)

score 0 · Answer 4 · answered Nov 20 '16 at 17:32

I am a bit surprised that most answers state that this design is ok.

I find it inacceptable unless there is a very good reasons (like performance requirements) to do it that way.

This is a circular dependency that impairs readability and testability of the code. But my main concern is that it will be very hard to keep the references consistent.

As an alternative, I would provide just one reference, say User has a pointer to Job. Now if you have a Job and want to find the corresponding User(s), you can still do a search over all Users and find the ones that have a pointer to this Job. Of course this is less efficient than a direct pointer but in many cases it will not matter. What's likely to be more important is that inconsistencies cannot arise.

Is it ok for 2 classes to have field pointing eachother?

4 Answers4