4

I've been working in a rather large codebase filled to the brim with small classes such as

class Person
{
    public string name;
    public int age;
    public int height;
}

As a mainly front-end experience developer I'm finding a hard way to determine the best practice when returning multiple values from methods. For example in Javascript all one would need to do is return a js object. I know one could use keywords such as "out" or "ref" and receive a similar result, but I find using them hinders readability.

So lets say we're finding a person's height and age from a database based on their name. And because the codebase is old and disorganized, you're not allowed to rely on Entity or other frameworks for model classes.

Would you rather create a method like this:

public Person getPerson(string name)
{
    Person p = new Person("Samuel");
    p.height = db.getHeight("Samuel");
    p.age = db.getAge("Samuel");
    return p;
}

or just give the class a constructor like so:

class Person
{
    public string name;
    public int age;
    public int height;

    public Person(string name)
    {
        this.name = name;
        this.age = db.getAge(name);
        this.height = db.getHeight(name);
    }
}

Personally, I find that the class with the constructor is better design since you're keeping all the object's logic encapsulated within itself. However, a lot of the codebase uses the method-based approach. I don't really need to worry about going against the grain, my supervisor is pretty open to changes. I would just like to know what the best approach would be.

Thanks!

Improve this question
1
  • 1
    FYI in your first example, your "getPerson" function doesn't use the supplied "name" parameter Commented May 21, 2021 at 17:29

7 Answers 7

Reset to default
13

None of the approaches is advisable (although some may disagree and have a valid point). In fact, it doesn't belong to the Person to know anything about the database. Instead, the class should be a possibly immutable object, which takes the name, age and height from the caller; it belongs to the caller, then, to know how to get those values.

Even more worrisome, your Person assumes that the age and the height should be loaded from the database. While it might be true for a proof of concept in a range of tens of lines of code that you just drafted to test something, and will rapidly throw away, it won't be for anything larger than that. As soon as you will add unit tests, you'll have to mock the database. And sooner or later, you'll find other scenarios: for instance an instance of the Person class constructed from the values entered in a form, or from the values coming from the cache.

Instead:

  • Person should be limited to its own responsibilities. This includes keeping the name, age, and height values, validating the inputs (for instance by throwing an ArgumentOutOfRangeException when the specified age is a negative integer), and possibly ensuring that the values remain unchanged (i.e. be immutable).

  • The database calls should be in a dedicated class which connects your application to the database.

  • If the data source can easily be mapped to Person, the class which connects your application to the database can map a column from a table, or the fields from a document, to an instance of the Person class. Otherwise, a dedicated class should handle the mapping if it has some complex rules in it.

Improve this answer
2
  • 4
    I’d suggest “generally not advisable” instead of “wrong”. Although I would not recommend them either, in all objectivity active records are a defendable approach for simple domains with applications that require just a couple of CRUD operation and the object’s purpose is mainly to be a proxy for what happens in the db. Commented May 21, 2021 at 11:14
  • I agree with Christophe. I would like to add that while the active record is an old, but still useful pattern, last decade has seen the widespread adoption of ORM libraries, therefore a tight coupling with the database and the model classes has become quite common. The important thing is to separate properly the model classes from the control classes and always keep in mind the separation of responsibilities. Commented May 23, 2021 at 20:27
3

The problem with your example is that it is very light on context and details, making it hard to understand how this slots into the bigger picture of your project. In general, the better approach here is to use combination of both.

public Person getPersonFromDatabase(string name)
{
    Person p = new Person(
                   "Samuel", 
                   db.getHeight("Samuel"), 
                   db.getAge("Samuel")
               );
    return p;
}

class Person
{
    public string name;
    public int age;
    public int height;

    public Person(string name, int age, int height)
    {
        this.name = name;
        this.age = age;
        this.height = height;
    }
}

Each has their own responsibility:

  • Person defines the data structure of a person, regardless of where the data will be sourced from
  • getPersonFromDatabase defines how you can fetch a person's details from the database
    • Note that the method's implementation depends on the data structure as defined by Person. So if Person changes tomorrow (e.g. a new field is added to the class and its constructor), getPersonFromDatabase will break until it knows how to provide that extra data field to the Person constructor. This is by design, because it means that you can ensure consistency about the Person data object even when making changes to existing code.

If you want to get a person from the database based on their name, use getPersonFromDatabase. If you want to create a person from another data source (e.g. user input), then directly construct a Person object from that.

Improve this answer
2
  • Absolutely right - the Person constructor shouldn’t know about the database at all. Why should it? Commented May 21, 2021 at 12:50
  • Good answer. Somewhat tangentially, I would strongly recommend making the fields private and also final (unless there's a need to modify them in the application.) Commented May 21, 2021 at 15:20
1

The two methods you show are not quite equivalent: the first one does not touch the state of the object which contains it. It might as well be a static method that gets some data from a database then returns it in an object. The second method is a constructor which directly sets members of the containing object.

In general, I'd prefer the constructor-based approach when it is unlikely that the values passed into the constructor will need to be modified from outside for the lifetime of the object.

The other approach, which really looks more like a utility-method in a static class can also be useful. Sometimes a utility method is used to enhance/enrich/update an object. Sometimes the data being returned is not an actual object so there is no class which could have a constructor. In your case, I think the constructor-based approach makes more sense. ...Of course, this assumes that the database connection will already be ready to use in the constructor.

Improve this answer
1

If you have a class with just 3 properties and no behavior, what you have is best described as a record not an object. If you have a class with 3 properties and a constructor that just sets those properties or gets the properties from a db (where you appear to be using a global, you should really be injecting it into the constructor) and then sets the properties, you still just have a record with no real behavior and thus not an object in OOP terms.

Constructors are not behavior, they don’t have external effect, and whichever one you use, you should end up with the same effective record.

In short, it doesn’t make a difference in OOP terms.

Now, in practice, you might want to have 2 or even 3 constructors, simply to allow for different ways of doing things. An empty constructor, where the record is initialized with data after construction, a constructor that takes your 3 properties as parameters and sets the properties, and a constructor that takes an object (and whatever other terms you need) which returns your data (in your case that appears to be whatever db is and a string).

Given you aren’t doing OOP at this point, the real question is just how are you using these “objects”? Are they DTO’s being used to transport data into and out of your system (perhaps being serialized or deserialized to json), are they being used to pass data around between various layers in your application? Part of a service oriented architecture? How it is used may decide how it is best constructed. Or it may be entirely irrelevant, that will depend upon your usage. Just don’t look to OOP principals for how to deal with something that isn’t OOP.

Improve this answer
0

If you call a constructor a new object will be created. If you call a function returning an object it can return an existing object if getPerson is called twice with the same arguments.

Sometimes you want objects that have an identity, and the constructor doesn’t give you that.

Improve this answer
0

Using "properties" in an "object" goes fundamentally against object-oriented principles. It makes the object a data structure, which is something that doesn't (shouldn't) exist in an object-oriented design.

So in short, none of those options is object-oriented, nor a good practice to do. Separating the data from its behavior just results in less maintainability, since stuff that often changes together is separated.

To be object-oriented you have to hide the data and offer behavior. Behavior that is specific to your application and your domain. Be it something that changes it (promoting an employee, canceling a credit card), or just presenting on a UI.

Loading data without knowing what the use-case is will only introduce problems like under-, or over-fetching, unoptimized queries etc. All that is unnecessary, just offer behavior, and have the behavior do database queries/update/etc as needed.

Improve this answer
3
  • 1
    @DocBrown Thanks for the catch, I meant to say "principles" instead of "practice". I corrected my answer. I agree that the "practice" (i.e. general consensus) does not follow the principles. I hope we agree that having data structures and their function separately goes against the very nature of object-orientation? Or do we? Commented May 22, 2021 at 18:36
  • 1
    Well, there are different schools of thought, and I think your point of view is a pretty dogmatic one. So I think it would be way more honest if you would mark what you wrote above clearly as your very personal point of view - I don't see where your correction takes that into account. And though I agree that the core the idea of object-orientation is to bundle data and their function together, I don't think that "real OO" requires to put *every * kind of possible operation into an object, and that the usage of properties is "fundamentally against object-orientation" ... Commented May 22, 2021 at 22:07
  • ... it may go against a fundamentalistic idea of object-orientation, of course. Commented May 22, 2021 at 22:13
0

The classic answer is that a Constructor makes it obvious what is happening. It initializes (at least partly) a new Object and returns it.

Advantages:

  1. Clear
  2. Subclasses may call it in constructor chaining. super(arg1, arg2) Depending on your views of inheritance, this can be a good or a bad thing.

Disadvantages:

  1. If there are many variations (many options for various parameters coming in) it may be confusing which version is which. This is also affected by how strongly typed is the language, how it supports overloading, etc.

A Method Returning an Object, a.k.a. a Factory, helps with that disadvantage, since you can vary the name of the method. e.g.

  PersonFromDatabase(Database d)
  PersonFromNameAndID(Name n, ID id)

One can also futz with Abstract Factories and similar patterns to avoid coupling and give more flexibility and configuration. Whether these are worth the effort and complexity, well, depends. Note that this makes subclasses harder to handle.

Note: I agree with the comments that your example is flawed. But, ignoring that, this is an attempt to answer the question.

Improve this answer

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.