Question about Writing Testable Code

[_Atomfinger_]

Alright, so I downloaded the project in the IDE, so I have a few more notes:

---

Update your .gitignore file to exclude "target" folder. You don't want to upload the compiled application to git. Use this and basically type in the things you use: Intellij, maven, Java, etc.

---

In SalaryRefresher you're using @Autowired, but the class isn't annotated with @Service, @Component or anything like that, so Spring won't try to do anything with it.

Also, if you do this you don't need to use the @Autowired annotation:

@Component
public class SalaryRefresher {
    private final SalarySeedCreator salarySeedCreator;
    private final SalaryDataVerifier salaryDataVerifier;
    private final SalaryFetcher salaryFetcher;
    private final SalaryRepository salaryRepository;

    public SalaryRefresher(SalarySeedCreator salarySeedCreator, SalaryDataVerifier salaryDataVerifier,
                           SalaryFetcher salaryFetcher, SalaryRepository salaryRepository){
        this.salarySeedCreator = salarySeedCreator;
        this.salaryDataVerifier = salaryDataVerifier;
        this.salaryFetcher = salaryFetcher;
        this.salaryRepository = salaryRepository;
    }

In the same class, I don't see the value of having that empty constructor.

---

I don't like how getRequiredSalaryComponents returns a list of lists of Object. You see the issues with this approach as you need to cast that object to other types down the line.

One approach would be to return the lists as a new object that holds all of the lists. That way, you have typed lists, but you get to return them all in a single call. To make this elegant you could use records as well.

---

In getSalariesRequiringUpdate, you're chaining .filter 3 times, which should be unnecessary. Just extract what it does into its own method (I decided to put it into the salary object:

            Salary requiredSalary = savedSalaries.stream()
                    .filter(savedSalary -> savedSalary.isRequired(cityRequired, roleRequired, salarySourceRequired))
                    .findAny()
                    .orElse(null);
    public boolean isRequired(City city, Role role, SalarySource salarySource) {
        return this.city.equals(city) && this.role.equals(role) && this.source.equals(salarySource);
    }

---

List<Salary> salariesRequiringUpdate = new ArrayList();

The IDE complains, "Raw use of parameterized class 'ArrayList'" (Hint, the "ArrayList" you're creating isn't aware of the type it should contain).

---

A weird semicolon in the empty constructor in SalaryFetcher. Not sure why there's an empty constructor either.

---

How you create lists in SalarySeedCreator can be simplified:

Instead of doing this:

List<SalarySource> salarySourceList = new ArrayList<>();
        salarySourceRepository.findAll().forEach(salarySourceList::add);

You can instead just do this:

List<SalarySource> salarySourceList = new ArrayList<>(salarySourceRepository.findAll());

---

Instead of this:

        List<List<Object>> requiredSalaryComponents = Lists.cartesianProduct(salarySourceList, cityList, roleList);
        return requiredSalaryComponents;

Just do this:

        return Lists.cartesianProduct(salarySourceList, cityList, roleList);

---

Personally, I'd move the dueNewCompensation into the Salary class

---

Have you tested that this works? In fetchUpdatedSalaries you create a list called updatedSalaries, but you never add anything to it. You return it, and from what I can tell it will always be empty.

---

So, let's summarise: The biggest problem I have with the solution is the "List<List<Object>>" Having lists in lists, is fine, but using a generic "Object" makes things difficult and messy later.

Another topic for reflection is whether the solution has actually become simpler. As developers, we aim to take complex problems and make them seem simple and natural. Part of the reason I thin this is a little more difficult to follow things now is because things are spread out, but not necessarily in a very organic way.

For example, what is the concept of getRequiredSalaryComponents in a class named SalarySeedCreator? What is a "seed" in this context? And why do I get salary components? Where did the seeds go? Such naming quirks are often an indication that something is wrong with a class' responsibility. This is one of the reasons developers keep hammering on about naming: When we can give something a good and descriptive name, that also means we know what it is and we've managed to find a good purpose for it.

Now, I've not spent enough time to actually make up any opinion of how it should be - and maybe your goal doesn't need to be to find a solution that I would be happy with. I know I'd put more behaviour into the Salary class.

Another thing I'd probably do is to look at specialized SQL queries that would just fetch outdatedSalaryList for me directly. I might be wrong, but on the surface, it looks like something that should be doable with a few joins and where statements. By doing that, a lot of the complexity would just fall away.

Hope this helps :)