How to follow best practices, such as SOLID principles, to implement a certain functionality

[Stan Belen]

Hello

I am trying to implement functionality, an analogy of which will follow, that will meet best practices (SOLID principles, etc.)

Suppose we have a legacy DTO that we cannot change, Person:

public class Person {
  private String name;
  private List<PersonAttribute> personAttributes;
  ...
}

And, here is the PersonAttribute class:

public class PersonAttribute {
  private String label;
  private String value;
  ...
}

A Person's address is stored as one of the attributes, for example:

PersonAttribute address = new PersonAttribute();
address.setLabel("address");
address.setValue("123 Some Rd.");

Person person = new Person();
person.getPersonAttributes().add(address);

I need to create a method that will update a person's address and then do some other stuff, for example, call a Human Resources System API.

I was also told that the personAttributes field:

might be null

might not be null but it might not contain an address

might not be null and it might contain an address

One approach to implement this method can be:

public void updateAddress(Person person, String newAddress) {

  if (person.getPersonAttributes() == null) {
    PersonAttribute address = new PersonAttribute();
    address.setLabel("address");
    address.setValue(newAddress);
    person.setPersonAttributes(List.of(address));
  } else {
    Optional<PersonAttribute> optionalAddress = person.getPersonAttributes().stream()
      .filter(attr -> "address".equals(attr.getLabel()).findAny();

    if (optionalAddress.isEmpty()) {
      PersonAttribute address = new PersonAttribute();
      address.setLabel("address");
      address.setValue(newAddress);
      person.getPersonAttributes().add(address);
    } else {
      optionalAddress.get().setValue(newAddress);
    }
  }

  // do a bunch of other stuff and make the method big :)
}

One of the problems with this implementation is that we can't unit test the various conditions and their outcomes without having to not worry about the functionality that follows in this method.

So, returning back to my question, how to go about implementing these various functionalities to meet best practices (SOLID principles, etc.)? For example, one thing that stands out is the violation of the Single Responsibility principle.

Thank you for help in advance

[Junilu Lacar]

Stan Belen wrote:
One of the problems with this implementation is that we can't unit test the various conditions and their outcomes without having to not worry about the functionality that follows in this method.

Please explain what you mean by the italicized part of what I quoted above. What functionality are you referring to that you don't want to worry about? Because when you're unit testing something, you are worrying about the functionality that you're testing.

how to go about implementing these various functionalities to meet best practices (SOLID principles, etc.)? For example, one thing that stands out is the violation of the Single Responsibility principle.

Again, what are these various functionalities you're referring to? And what do you mean when you say they have to "meet best practices"?

To me, I don't look at a functionality as having to do anything other than to make sense where it belongs, and for it to make sense to be exercised the way I design it.

[Junilu Lacar]

Junilu Lacar wrote:To me, I don't look at a functionality as having to do anything other than to make sense where it belongs, and for it to make sense to be exercised the way I design it.

For example, I don't think this proposed design makes sense from an object-oriented programming perspective:

public void updateAddress(Person person, String newAddress) {
   ...
}

Why don't I think it makes sense? Because it smells of broken encapsulation. Why would the logic to update a person's address be separate from the address and the person objects?

Why isn't the design such that we can just write this:

person.setAddress(newAddress);

?

[Junilu Lacar]

Also, what's with this line in the example code?

Stan Belen wrote:  // do a bunch of other stuff and make the method big :)

If you're looking to follow "best practice", you would NOT be thinking about adding a bunch of other stuff into an already big method like that and make it even bigger.

[Junilu Lacar]

Here's something I just realized. It follows from what Joshua Kerievsky wrote in his book Refactoring to Patterns: rather than try to write a program with a pattern already in mind, just write the program to make sense. Then when you start seeing things in your code that correspond to known patterns, refactor your code toward those patterns (or away from anti-patterns).

This takes a certain level of skill and experience to fully understand.

Likewise, when it comes to design principles like SOLID, I don't really see them as "best practices" but rather as useful reminders of things that can get messed if I don't follow them. In other words, I don't start out designing a program so it conforms to all possible design principles. Rather, I familiarize myself with various design principles and try to recognize when some difficulty I'm experiencing when working with the code is due to a violation of a principle. Then I start to refactor the code to conform to the principle.

[Campbell Ritchie]

Stan Belen wrote: . . . A Person's address is stored as one of the attributes . . .

That looks like a Map of some sort. Please consider whether you can design such a Map.
Are you taking defensive copies of mutable fields?

might be null

might not be null but it might not contain an address

might not be null and it might contain an address

Decide how many attributes are essential, remembering some People lack some attributes, LandLinePhoneNumber being a common one.
Don't stand for information lacking those essential attributes. I would throw an exception if passed a null.

. . .

public void updateAddress(Person person, String newAddress) {
   // . . .
   // do a bunch of other stuff and make the method big :)
}

. . .

I am afraid, that looks wrong in two ways. That method should be called on the object and should therefore not need the Person parameter. What you wrote could readily be converted to a static method, which makes me suspicious about it.
You should also avoid big methods.

public void setNewAddress(Address a)
{
   attributes.putAddress(a); // or similar
}

[Campbell Ritchie]

Junilu Lacar wrote:. . . Why isn't the design such that we can just write this:

person.setAddress(newAddress);

?

Of course: Junilu said it long before I did.

[Stan Belen]

Thank you Junilu for getting back to me. Please see my answers below...

Junilu Lacar wrote:

Stan Belen wrote:
One of the problems with this implementation is that we can't unit test the various conditions and their outcomes without having to not worry about the functionality that follows in this method.

Please explain what you mean by the italicized part of what I quoted above. What functionality are you referring to that you don't want to worry about? Because when you're unit testing something, you are worrying about the functionality that you're testing.

I meant, suppose the example method, updateAddress(), that I wrote in my initial post, had to, in addition to updating the address, also make an API call to some external system, too, for example, a Human Resources system. Please see the following code; it's a copy of what I had initially, with the addition of making the API call:

public void updateAddress(Person person, String newAddress) {
 
  if (person.getPersonAttributes() == null) {
    PersonAttribute address = new PersonAttribute();
    address.setLabel("address");
    address.setValue(newAddress);
    person.setPersonAttributes(List.of(address));
  } else {
    Optional<PersonAttribute> optionalAddress = person.getPersonAttributes().stream()
      .filter(attr -> "address".equals(attr.getLabel()).findAny();
 
    if (optionalAddress.isEmpty()) {
      PersonAttribute address = new PersonAttribute();
      address.setLabel("address");
      address.setValue(newAddress);
      person.getPersonAttributes().add(address);
    } else {
      optionalAddress.get().setValue(newAddress);
    }
  }
 
  try {
    // CompanyRestClient is the company for which this code is being written for custom implementation for making API calls
    CompanyRestClient companyRestClient = new CompanyRestClient();
    companyRestClient.setUrl("https://some-external-human-resources-system.com/notify/address-updated/" + person.getName());
    companyRestClient.setCallMethod("GET");
    companyRestClient.call();
  } catch (CompanyRestClientException e) {
    log.error("Error occurred when calling Human Resources system", e);
  }
}

Now, returning back to your question, Junilu, the extra code that I don't want to worry about when I'm unit testing the functionality of updating the new address is the API call above. And, even, suppose I want to test the API call code (not actually making a real call) and I don't want to worry about some other code updating an address.

Junilu Lacar wrote:

Stan Belen wrote:how to go about implementing these various functionalities to meet best practices (SOLID principles, etc.)? For example, one thing that stands out is the violation of the Single Responsibility principle.

Again, what are these various functionalities you're referring to? And what do you mean when you say they have to "meet best practices"?

To me, I don't look at a functionality as having to do anything other than to make sense where it belongs, and for it to make sense to be exercised the way I design it.

Ok, so, regarding the functionality examples, as I elaborated in this post, are:

updating an address of a person

making a call to an external system, for example, a Human Resources system

By "meet best practices", Junilu, I mean such practices that lead to things like avoiding the creation of large methods, loosely coupled unit tests from the production code, cohesive classes, elegant, easier to maintain, etc. My goal is to get better at designing and implementing such code so that's why I mentioned things like SOLID principles, etc. Basically, through having discussions, to learn and get better at this.

Thanks

[Stan Belen]

Junilu Lacar wrote:

Junilu Lacar wrote:To me, I don't look at a functionality as having to do anything other than to make sense where it belongs, and for it to make sense to be exercised the way I design it.

For example, I don't think this proposed design makes sense from an object-oriented programming perspective:

public void updateAddress(Person person, String newAddress) {
   ...
}

Why don't I think it makes sense? Because it smells of broken encapsulation. Why would the logic to update a person's address be separate from the address and the person objects?

Why isn't the design such that we can just write this:

person.setAddress(newAddress);

?

I agree with you, Junilu, that this design smells. However, as I mentioned, this is a legacy DTO.

So, suppose it's used in a lot of places. Going even further, suppose that the project uses a JSON-based database and this is how it's stored in the DB. For example:

{  "name" : "Bob",  "personAttributes" : [     {        "label" : "address",        "value" : "123 Some Rd."      }    ] }

[Stan Belen]

Junilu Lacar wrote:Also, what's with this line in the example code?

Stan Belen wrote:  // do a bunch of other stuff and make the method big :)

If you're looking to follow "best practice", you would NOT be thinking about adding a bunch of other stuff into an already big method like that and make it even bigger.

I agree with you, Junilu, that we don't want large methods. At this point, the only idea that I have to prevent a large method is to create the following two interfaces:

interface PersonAddressModifier {
  void modifyAddress(Person person, String newAddress);
}

interface HumanResourcesApiCaller {
  void callApi(String personName);
}

And then, to prevent creating a large method, change it like so:

public void updateAddress(Person person, String newAddress) {
  personAddressModifier.modifyAddress(person, newAddress);
  humanResourcesApiCaller.callApi(person.getName());
}

The way that I see, the advantage of doing this is that now I can test, in isolation, both the functionality that modifies a person's address and the functionality that makes a call to the Human Resources system. However, I will not write a unit test for the modified method in this post, updateAddress(), because all that can be tested is the implementation using mock objects which will couple the unit test to the production code.

[Campbell Ritchie]

Stan Belen wrote:. . . this is a legacy DTO.

So, suppose it's used in a lot of places. . . .

The idea of encapsulation and loose coupling is that you can change the implementation of the object without affecting how it is perceived by other code via its interface.
Writing a static method and disguising it as an instance method does, of course, make that result so much harder to achieve.

[Junilu Lacar]

When something is difficult to test, you refactor it so that it's easy to test.  Mixing in a API call inside a method like updateAddress seems like a poor choice. I would separate those two concerns.

[Stan Belen]

Campbell Ritchie wrote:The idea of encapsulation and loose coupling is that you can change the implementation of the object without affecting how it is perceived by other code via its interface.
Writing a static method and disguising it as an instance method does, of course, make that result so much harder to achieve.

Campbell, I think I understand what you mean. I want to verify. So, the functionality of setting the Person's address should be in the Person class like this, right:

public class Person {
  private String name;
  private List<PersonAttribute> personAttributes;

  public void setAddress(String address) {
    if (personAttributes == null) {
      PersonAttribute address = new PersonAttribute();
      address.setLabel("address");
      address.setValue(address);
      personAttributes = List.of(address);
    } else {
      Optional<PersonAttribute> optional = personAttributes.stream()
        .filter(attr -> "address".equals(attr.getLabel()))
        .findAny();

      if (optional.isEmpty()) {
        PersonAttribute address = new PersonAttribute();
        address.setLabel("address");
        address.setValue(address);
        personAttributes = List.of(address);
      } else {
        optional.get().setValue(address);
      }
    }
  }
  ...
}

[Campbell Ritchie]

That looks so very complicated. You can probably dispense with the Stream altogether. If you have a Map, you can simply call put(...) onit. Remember there are methods like putIfEmpty(...) or similar in Map.

[Stan Belen]

Campbell Ritchie wrote:That looks so very complicated. You can probably dispense with the Stream altogether. If you have a Map, you can simply call put(...) onit. Remember there are methods like putIfEmpty(...) or similar in Map.

Campbell, I agree with what you are saying. However, the problem is that I'm dealing with legacy code. I'm using a Person/Address domain as an analogy for the problem that I am facing and trying to get help in resolving it. This is something that I should have made clear from the beginning. I'll specify these kinds of things going forward.

I'll write here the Person class again, but this time I'll isolate the problem and I'll write the constraints that I'm facing:

public class Person {

  List<PersonAttribute> personAttributes = new ArrayList<>();

  public void setPersonAttributes(List<PersonAttribute> personAttributes) {
    this.personAttributes = personAttributes;
  }

  public List<PersonAttribute> getPersonAttributes() {
    return personAttributes;
  }

}

The following are the constraints:

overall, the class Person is used in 4,616 places

the access modifier of the personAttributes field is default, however, it is only accessed directly by the setter and getter methods, Person::setPersonAttributes and Person::getPersonAttributes

overall, Person::setPersonAttributes is used in 129 places

overall, Person::getPersonAttributes is used in 172 places

So, yeah, unfortunately, the way that this class, Person, was implemented in regards to the personAttributes field in the year 2015 is not good because it allowed this field to leak out all over the codebase.

This is why, Campbell, the implementation of Person::setAddress "looks so very complicated" with using a Stream, null checks, etc.

Please let me know if there is another way that this problem can be dealt with. Thank you

[Stan Belen]

Junilu Lacar wrote:When something is difficult to test, you refactor it so that it's easy to test.

Junilu, I agree with this.

Junilu Lacar wrote:Mixing in a API call inside a method like updateAddress seems like a poor choice. I would separate those two concerns.

Junilu, the thing that I'm stuck on and would appreciate help with is suppose you need to implement the following requirement:

1. You will receive the ID and the address of a Person
2. Use the ID to retrieve the Person entity from the DB
3. Update the address of the Person
4. Save this information in the DB
5. Make a call to an external system, for example, a Human Resources system in regard to this change

How would one write unit tests for such a requirement?

The part that I'm stuck on is that somewhere in the code we will need to implement something like this:

public class PersonAddressUpdateService {

  private PersonRepository personRepository;

  private CompanyRestClient companyRestClient;

  public PersonAddressUpdateService(PersonRepository personRepository, CompanyRestClient companyRestClient) {
    this.personRepository = personRepository;
    this.companyRestClient = companyRestClient;
  }

  public void changeAddressProcess(long personId, String address) {
    updatePersonAddress(personId, address);
    callHumanResourcesSystem();
  }

  private void updatePersonAddress(long personId, String address) {
    Person person = personRepository.findPerson(personId);
    person.setAddress(address);
    personRepository.update(person);
  }

  private void callHumanResourcesSystem(long personId) {
    try {
      CompanyRestClient companyRestClient = new CompanyRestClient();
      companyRestClient.setUrl("https://some-external-human-resources-system.com/notify/address-updated/" + personId);
      companyRestClient.setCallMethod("GET");
      companyRestClient.call();
    } catch (CompanyRestClientException e) {
      log.error("Error occurred when calling Human Resources system", e);
    }
  }

}

How can I restructure this code to be able to unit test it?

[Junilu Lacar]

It's starting to feel like this conversation is going around in circles. Let's back up to the original post:

Stan Belen wrote:
I am trying to implement functionality, ...

Suppose we have a legacy DTO that we cannot change, Person:
...
I need to create a method that will update a person's address and then do some other stuff, for example, call a Human Resources System API.

You have this legacy design for Person and you need to add functionality so you can update the address attribute of Person. Then you also have to do other stuff.

If you want separation of concerns and single responsibility, then you'd separate the concern of updating the address from "other stuff", you wouldn't put it all in one method like you showed initially.

A unit test is focused on one small bit of functionality. It doesn't seem like you're looking to unit test but rather test ALL of the functionality. That isn't unit testing. Change that viewpoint first, then we can be on the same page as to how to proceed.

[Junilu Lacar]

Stan Belen wrote:How can I restructure this code to be able to unit test it?

Ironically, you'd write a unit test that would show you the ideal structure of the code so that it can be tested in isolation.

Let's see how you react to this first before I explain further.

[Stan Belen]

Junilu Lacar wrote:

Stan Belen wrote:How can I restructure this code to be able to unit test it?

Ironically, you'd write a unit test that would show you the ideal structure of the code so that it can be tested in isolation.

Let's see how you react to this first before I explain further.

Junilu, questions:

Question 1: do you mean to use test-driven development in our conversation?

Question 2: is it okay if I'll create a public github project for the purpose:

 a) the initial commit will be an analogy of what I am dealing with at work - the legacy code
 b) the subsequent commits will be the test-driven development red-green-refactor steps, that is, I'll make a commit for each red-green-refactor step
 c) I will use the commit message to explain what I am doing, why I'm doing, etc
 d) then I will be able to reference a point in which I will be stuck on, or will need a suggestion

Thanks for the help

[Junilu Lacar]

That is an excellent idea. And yes, I'm saying that you would test-drive the refactoring of the code.

[Stan Belen]

Stan Belen wrote:is it okay if I'll create a public github project for the purpose

Junilu Lacar wrote:That is an excellent idea. And yes, I'm saying that you would test-drive the refactoring of the code.

I've created the project github: https://github.com/sbelenky/person-example

Questions

Question 1: is it fine how I used TDD to implement Person::setAddress?

Question 2: how would you go about in implementing the requirements, described in the README.md of the project?

[Junilu Lacar]

Stan Belen wrote:
Questions

Question 1: is it fine how I used TDD to implement Person::setAddress?

Question 2: how would you go about in implementing the requirements, described in the README.md of the project?

Answer 1: Sure, you can add functionality by doing TDD. It looks like setAddress() is already implemented though. Is there anything else you need to do?

Answer 2: These are the "requirements" you wrote:

Suppose we are given the following assignment: Implement a service that takes as input a person ID and an address, and updates the person's address in DB and calls the HRService::updatePersonInfo

As requirements go, that's pretty sparse.

Looking through the test code and design, I'm seeing a number of problems with respect to following design principles. For example, the tests reveal that the design easily breaks encapsulation of the Person class. Did you notice that when you were writing the tests?

[Junilu Lacar]

Stan Belen wrote:Question 1: is it fine how I used TDD to implement Person::setAddress?

I think I misunderstood your question earlier. There's really no way for me to tell whether your use of TDD was fine. I would have had to be there when you did it. TDD is about the journey, not the destination. Judging TDD based on the end result is like judging a play based on the last minute of the final scene. I'd need to see the whole thing, not just the end result.

I do see that the test code seems clear enough that I can actually get a sense of what story you want to tell with them. However, the fact that the tests show how to break encapsulation of the Person class makes me think you could benefit from discussing the problems with the current design and then doing some more refactoring to address those problems.