2

I'm following a course on Udemy and the teacher said that is a best practice return the update object in an add method. This is the example:

public Recipe addIngredient(Ingredient ingredient) {
    ingredient.setRecipe(this);
    this.ingredients.add(ingredient);
    return this;
}

This method is a method of the class Recipe. In my opinion this is not needed. I think that is more logical to have a void method or maybe return the result of the line:

this.ingredients.add(ingredient); // ingridients is a HashSet

On the other hand with this approach we have the following feature:

tacosRecipe.addIngredient(new Ingredient("Ancho Chili Powder", new BigDecimal(2), tableSpoonUom))
    .addIngredient(new Ingredient("Dried Oregano", new BigDecimal(1), teapoonUom)); 

we can populate the HashSet by calling the method addIngredients in a row.

What is your opinion? Would you choose clarity (the method doesn't return the update object) or the benefit that gives (calling addIngredients in a row)?

Edit: someone suggested to me that Better to have 2 methods with clear meaning, or just 1 dual use method? could be an answer to my question. But the question is different. I'm not asking if I should have two method (one get and one add). Does make sense have a get method that return this? Is not a singleton then this doesn't make sense to me. The question is: Is it worth the line return this? The teacher said that do this is a best practice and maybe I missed something.

I want to point out that the goal to do this is not avoid having an output argument (see page 41, Common Monadic Form, last paragraph, book: Clean Code). I'm updating the object of the class itself, a Recipe object. However, it also true that I update an input argument (there is a bidirectional relationship between these two classes). To show that this method not only add an ingredient to the Recipe object, but also update the ingredient I could refactor this method as:

public Ingredient addIngredient(Ingredient ingredient) {
    ingredient.setRecipe(this);
    this.ingredients.add(ingredient);
    return ingredient; // <------ Here the refactoring
}

However I'm not sure that this method is good enough (Is it the Single Responsibility Principle respected?).

So let's simplify this. I suppose for a moment that a bidirectional relationship doesn't exist. In this case I can rewrite the method in a way that we can focus more on my question:

public Recipe addIngredient(Ingredient ingredient) {
    this.ingredients.add(ingredient);
    return this;
}

Now, does make sense return the object?

5
  • Does this answer your question? Better to have 2 methods with clear meaning, or just 1 dual use method?
    – gnat
    Jul 12 at 9:39
  • Sorry Pieter but this doesn't return an Ingredient. The method, as I wrote, reside in the Recipe class and it return a Recipe object. I cannot inspect the ingredient just added without use the list of ingredients. Jul 12 at 9:40
  • Looks to me like a builder pattern, which if used should follow standard naming conventions
    – Chris Neve
    Jul 12 at 9:41
  • @gnat even if the example in that question is different (in this case I'm returning the instance of the object on which the method is called) I think this could be a reasonable answer for the single responsibility principle. I'm saying that this is different because doesn't make sense have a get method for the instance itself (I'm not using the Singleton pattern). So the question is slight different, I'm not asking to have 2 method but simply avoid to return the object instance. It's not clear to me why the teacher said that this is a good practice. Jul 12 at 9:49
  • @ChrisNeve I don't think so. Recipe it's a class and ingredients is simply its property. Return the object which calls the method doesn't make sense to me. Jul 12 at 9:54
6

What your teacher is promoting is the "Builder Pattern" (or "Fluent Interface") wherein many methods can be called on an object in one statement:

Recipe breakfast = new Recipe(); 

breakfast 
   .addIngredient( "Bacon" )
   .addIngredient( "Eggs" )
   .addIngredient( "Mushrooms" )
   ;

The code using the recipe1 object has no idea how the Recipe object stores its ingredients and does not care. It tells the Recipe object to add another ingredient and leaves it at that.

Your alternative "one-liner" breaks this encapsulation, as you clearly indicate in your own sample:

recipe1.ingredients.add(ingredient); // ingridients is a HashSet

Whilst that comment may be true on the day you write this code, it might not be true a day / week / month / year later! The internal construction of the Recipe class could be completely re-written (re-factored) on you without your knowledge. Using only methods defined on the Recipe class itself, you are insulated against this sort of change, which would break your one-liner completely!

1
  • Regarding the second part of you answer, you are absolutely right! I didn't think about a drastic future change. I should thought about that of course! The teacher didn't mention the builder pattern. In the end this is useful "only" to have this feature (i.e. have many methods called on a single object). Well I think that this makes sense and the code respect the SRP I suppose...Thanks. Jul 12 at 11:07
1

One additional thought:

If you want to use your array of ingredients at multiple places, the classical arrCopy = arrOriginal does not COPY the array (at least in most coding languages) but just copies the reference to the array. As a result all changes on that array will be reflected also on all of those "copies".

As a result, if such an array is propagated through multiple methods or even classes, it really gets hard to understand why the data changes.

ingredients = [...];
checkIngredients(recipe, ingredients);

What would happen now, if checkIngredients will add all the missing ingredients for a recipe? You have to read that method to know that.

To tackle that problem, many people follow the path of "Immutable" Objects. That means, you do not change an object, but return a new object with the changed content

ingredients = [...];
ingredients = checkIngredients(recipe, ingredients);

gives you at least a hint, that something happens with the data.

As with all approachs, it is not always wise to follow them blindly. But there are quite a lot of circumstances, where immutable objects may save your day.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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