1

Let's say I have a class that's a bit more than a pure Data object, i.e. it holds some data and some basic queries about that data. (In this case, it's a class representing a timetable)

Now, I want to run certain validation checks on that Data object, and I feel like this is a responsibility separate from that of representing the timetable. Thus, there should be a class ScheduleValidator or something to that extent.

However, the validator will of course query a lot of things from the timetable class (checking for certain clashes, for certain capacity overloads etc) in order to decide if the schedule is valid or not according to some rules. Would this count as feature envy? If not, why not? If so, what to do about it?

Lagerbaer
  • 526
  • 3
  • 9

3 Answers3

8

This is not feature envy but questionnable design:

  • Feature envy is about methods of one class using data of another class "excessively". It is a symptom of a potential design issue, e.g. that methods should belong to the other class, or that methods don't use abstractions offered by the other class as they should.

  • Your design intends to use a pure data object. So ScheduleValidator has no other abstraction to use, no encapsulation to break, and probably has no data itself. In short, it's not envy but necessity.

  • The real question is why you want to have a pure data object? This looks heavily like an anemic domain model, which is an OO anti-pattern since it tries to separate what OO tries to unite.

Christophe
  • 74,672
  • 10
  • 115
  • 187
  • I mention in my post that my object isn't "pure" data. It can still answer business-logicy queries, hiding the internal way of how the events in the schedule are tracked. But now I also have the need of checking certain properties of the schedule. I feel like this is a separate responsibility and should be its own class, but then of course it would call a lot of the methods of the schedule object – Lagerbaer Dec 20 '19 at 16:01
  • @Lagerbaer I understood “a bit more than pure data” as “just a little more”, which could still raise the anemic suspicion. But if it provides the right level of abstraction and encapsulation, and if the validator is a separate responsibility (e.g. business rule beyond the simple data consistency check, that could evolve independently of the time table) then it’s ok (i.e. it’s necessity not envy). – Christophe Dec 20 '19 at 16:55
  • I'm certainly getting the terminology wrong and cause confusion that way. I realize now that "validator" has a specific connotation of checking internal consistency, whereas what I'm after is checking if a particular "internally valid" instance also satisfies certain external criteria – Lagerbaer Dec 20 '19 at 17:02
2

Let's say I have a class that's a bit more than a pure Data object...

That's good, because there is no such thing as a "data object". A data object is a record or struct. At least in an object-oriented environment those are a good indication of a bad design in themselves.

It is bad, because inevitably provokes either Feature Envy, as you point our or just plain violations of Encapsulation. Depends on the exact definition which, both or something else (for example Law of Demeter) is violated, but the point is that it is fundamentally incompatible with object-orientation, so some-or-other rule will be violated.

Validating some object from the outside is just not possible without intimate knowledge of the object's internals (as you also point out). Every time the object changes it is very likely that the validator needs to change. I.e. things that change together are not together, which is bad for maintenance.

The solution is not to "represent" a TimeTable, but to be a TimeTable. Hide the data and offer relevant business-functionality!

Robert Bräutigam
  • 11,473
  • 1
  • 17
  • 36
  • "Hide the data and offer relevant business-functionality!" Okay, but then in terms of single responsibilities, I still see a separation between the time table (which should offer queries into what's going on when etc) and something that checks if the schedule satisfies certain requirements. The requirements can change for reasons other than how representing the schedule might change. – Lagerbaer Dec 20 '19 at 15:57
  • @Lagerbaer I approach this much more pragmatically. If some functionality needs to use data/internal state from some object, then it belongs to that object. Until there is a time where it is forced out by some other requirement or constraint (for example coming from outside, or there being many different variants). In many cases you *don't* need a generic `TimeTable` (unless you are writing a library or something), it can do specifically what you want in your project. So the question is, is there an immediate need to do it separately? – Robert Bräutigam Dec 20 '19 at 16:46
  • Not a library. In this case, the TimeTable was meant to be an object living in "domain world" and is the output of an optimization algorithm that lives in "technical details of the solution" world. – Lagerbaer Dec 20 '19 at 17:00
1

Before giving my opinion and suggestion, it is better to apprehend the definition from the previous cases:

Methods that make extensive use of another class may belong in another class. Consider moving this method to the class it is so envious of.

https://blog.codinghorror.com/code-smells/

with that quote in mind and assuming a simple logic of your proposal summed in an example:

TimeTable.java

public class TimeTable{
 private int time1;
 private int time2;
 private List<Location> locations = new ArrayList<>();

//getters
}

SchedulerValidator.java

public class ScheduleValidator{
public List<Location> getLocationsByTime(int time){
 //some logic
}

public List<Location> getLocationsByTime(int begin, int end){
 //some logic
}
}

The above snippet in any form, is a great example of a FeatureEnvy on top of that against SRP Violation

To stay clean and complaint, the below solution meets all criteria:

public class TimeTable{
 private final int time1;
 private final int time2;
 private List<Location> locations = new ArrayList<>();

 //constructor

 public List<Location> getLocationsByTime(int time){
 //some logic
 }

 public List<Location> getLocationsByTime(int begin, int end){
 //some logic
 }
 //getters
}

public class ScheduleValidator{
 public void validate(int timeValue1, int timeValue2, TimeTable timeTable){
 //some validation against user inputs, in case throws an exception
 }
}

public class ScheduleChecker(){     
 public void check(int userProvidedTime1, int userProvidedTime2, TimeTable timeTable){
 //some validation against the time table list and user inputs
 }
}

//I assume this is where you will use all
public class ScheduleController{
 private final ScheduleValidator scheduleValidator;
 private final ScheduleChecker scheduleChecker;
 private final TimeTableRepository timeTableRepository;

 //constructor

 public String saveSchedule(int time1, int time2){
     TimeTable timeTable = timeTableRepository.find(1L);
     scheduleValidator.validate(time1, time2, timeTable);
     scheduleChecker.check(time1, time2, timeTable);
     //no exceptions further business logic carries on after this line
 }
}

This example is also testable, you can simply mock all easily and write Unit and Integration Tests.

Last of all, my suggestion is to approach breaking requirements down :

  • Validator: If you receive an input and test whether they have some dirty data entries,
  • Checker: Checks whether given user input corresponds to your system data
Tugrul ASLAN
  • 181
  • 3