Code Review

[Andy Rosemond]

Junilu Lacar wrote:

Liutauras Vilda wrote:
I'd question ... do I need to stuff that to student class? Today documents are needed, tomorrow might not, and that changes based on to compliance rules defined by the university.
Junilu showed one way which would handle that better in my opinion.

My thoughts exactly. It's difficult to express the heuristics involved but I'll try anyway. I think the student.requiresDocuments() option isn't very attractive to me because it assigns the responsibility of enforcing the documentation requirements rule to the Student. However, the Student is also the subject of the rule. That's kind of like having students check and grade their own exam papers. I think my aversion for that option stems from the difference in the scope of responsibility I see a Student object has and the scope of responsibility that includes enforcing a rule like "An international student requires documents."  It just feels like this should be the responsibility of something other than Student. That's why I hypothesized having some kind of Rule object to handle that responsibility might be a better choice. Again, this hypothesis needs to be explored and proven through experimentation (via unit testing).

I don't think I'm forcing the Student object to enforce the rule. The way I wrote the code, I'm just telling the institution that "Yes I'm an international student" nothing more. In reality, you as a Student will know this(probably have docs to prove it as well) The school decides to add it.

[Liutauras Vilda]

Andy Rosemond wrote:I don't think I'm forcing the Student Object to enforce the rule. The way I wrote the code, I'm just tell the institution that "Yes I am an international student" nothing more. In reality, you as a Student will know this(probably have docs to prove it as well) The school decides to add it.

You let student implement the definition what requires documentation and what's not and assigning this knowledge to himself (student). But does the student must know in which circumstances documentation is needed? Student just need to comply with whatever been defined already by somebody else. So it isn't student's knowledge. I mentioned before, if you are aware of something - doesn't mean you are responsible for.

documentRequirementsRule.appliesTo(student)

This example is much more flexible. But Junilu is right, don't throw text there and there, write some tests, implement one way, see if it makes sense, implement another way,.. At some point you'll start seeing differences. Probably you need to go at least several times down the wrong route to see what issues it might cause to you.

[Andy Rosemond]

Liutauras Vilda wrote:

Andy Rosemond wrote:I don't think I'm forcing the Student Object to enforce the rule. The way I wrote the code, I'm just tell the institution that "Yes I am an international student" nothing more. In reality, you as a Student will know this(probably have docs to prove it as well) The school decides to add it.

You let student implement the definition what requires documentation and what's not and assigning this knowledge to himself (student). But does the student must know in which circumstances documentation is needed? Student just need to comply with whatever been defined already by somebody else. So it isn't student's knowledge. I mentioned before, if you are aware of something - doesn't mean you are responsible for.

documentRequirementsRule.appliesTo(student)

This example is much more flexible. But Junilu is right, don't throw text there and there, write some tests, implement one way, see if it makes sense, implement another way,.. At some point you'll start seeing differences. Probably you need to go at least several times down the wrong route to see what issues it might cause to you.

How would this method look like without breaking encapsulation of the Student object?

documentRequirementsRule.appliesTo(student)

Don't you have to call student.GetType() to test to see if the Student is in fact International?

[Junilu Lacar]

The third option I gave can lead to some interesting test and implementation code.

@Test
public void it_must_comply_with_documents_rule() {
   Student student = // assign international student with docs
   assertTrue(student.compliesWith(DocumentRequirementsRule.INSTANCE));
}

The semantics of the API seem reasonably clear. This might be my first attempt to implement that in code:

public boolean compliesWith(Policy rule) {
    return rule.appliesTo(this));
}

This looks a lot like a callback to me and while it seems straightforward, I'd question why it's necessary to use double dispatching. If you end up making that kind of call anyway, then why not just have what's shown in the test below?

@Test
public void it_must_comply_with_documents_rule() {
   Student student = // assign international student with docs 
   assertTrue(DocumentRequirementRule.INSTANCE.appliesTo(student));
}

The point is you have to try a few things to really see what the consequences of each choice are with regard to multiple principles, not just one or two. So what if you have a getType() method? What are the pros and cons of doing that in the contexts that it will be used? More experimentation and exploration of the API via unit tests could lead to considering even more alternatives to try out.

[Liutauras Vilda]

Andy Rosemond wrote:How would this method look like without breaking encapsulation of the Student object?

Please explain how this breaks encapsulation?

[Andy Rosemond]

Liutauras Vilda wrote:

Andy Rosemond wrote:How would this method look like without breaking encapsulation of the Student object?

Please explain how this breaks encapsulation?

I'm just asking, I never said it broke encapsulation.

I though I broke encapsulation when I called the StudentType getter to make a decision. I moved that decision to the Student class, but I guess that wasn't the best idea. I'm just wondering how that method would look like without calling the StudentType on the Student object. At some point a decision has be made depending on the Student Type, so you either have a method tell you(requiredDocuments), or you call the getter and do the decision making right there.

[Junilu Lacar]

Andy Rosemond wrote:I don't think I'm forcing the Student object to enforce the rule. The way I wrote the code, I'm just telling the institution that "Yes I'm an international student" nothing more. In reality, you as a Student will know this(probably have docs to prove it as well) The school decides to add it.

When I said "making Student enforce the rule", that was in reference to this possible choice:

if (student.requiresDocuments()) { ... }

and an implementation that looks something like this:

public class Student {
    public boolean requiresDocuments() {
         return this.type = INTERNATIONAL;
    }
}

Maybe I should phrase in another way instead of "the Student enforces the rule" -- by this I really mean that the rule "International students require documents" has been codified in the Student object and thus you make the Student object "aware" or "responsible" for the rule. When we use the word "responsible" in this context, we're really alluding to the SRP because any change to the requirement rule would necessitate a change to the Student object.

Try to avoid conflating what happens with real-world objects and what happens with your software objects. One big trap in OO design is trying to make your object model reflect too much of the real-world. Software models are abstractions and sometimes responsibilities are assigned to objects in ways that don't reflect how real-world objects behave. You can use real-world entities and relationships as a guide or inspiration for your abstractions in software but don't try to model the real world exactly as it is. I've been down that road before and it often leads to confusing and convoluted designs.

For example, asking a Rule if a Student complies with it is not really something you can do in the real world. However, in software, the abstraction provided by someRequirement.isMetBy(student) can be perfectly valid.

It might be useful to be able to use some kind of Registrar object like this:

registrar.verify(student, registrationRules)

However, you also have to consider whether a separate Registrar class is really necessary. Does it have other similar responsibilities that would justify its existence?

Real-world relationships are far more complex than our software designs need to be. Our job in design is to come up with useful abstractions and define meaningful relationships that help solve the problem that is in scope. In other words, your design choices should be limited by the goal of boiling the pot, not the ocean.

[Junilu Lacar]

Andy Rosemond wrote:I though I broke encapsulation when I called the StudentType getter to make a decision.

Don't make it about you and the decisions you made. That inevitably leads to attachment and being defensive. Try to practice egoless programming.

Determining if breaking encapsulation has negative consequences or where to make the decision that a rule has been violated based on the student type is often something that's easier to figure out as you learn more about your software. I find the task of figuring out things like this is much easier when I can actually see code that has problems rather than just imagining what problems writing code in a certain way can lead to. That's why I keep harping on experimentation and testing.

I like to look at what I do when I design as "making the code tell me what it wants to do." If I write some code and it doesn't look good, I'm thinking "Why is this hard to change? What's causing this particular problem? What does the code want to do instead? What does the code want to look like?" I find the metaphor of sculpting marble useful: You chip away at a marble block to reveal the figure that was hidden inside all along. Treat the code like a lump of clay that you're working on a potter's wheel. You try to get a feel for its texture and weight and slowly mold it to what it wants to be.

Chipping away or holding and molding the material in your hands gives you a better feel for it. Our material is code and just as with clay or marble, the more you work it and rework it, the more you can understand and get a better sense of what it wants to be.

I hope that isn't too Zen for you but that's kind of how I approach design and refactoring.

[Junilu Lacar]

Being sensitive and attentive to violations of design principles is a good thing but at the same time, don't treat them as measures of design correctness that demand strict compliance. Instead, use principles as guidelines for improving your design's quality attributes like resilience, cohesiveness, coherence, and consistency. When considering the consequences of a design choice, ask how you can mitigate the negative effects while also taking advantage of anything positive you can gain.

So, rather than avoiding getType() and treating it like it's a very bad thing to do, try to understand how it might improve or degrade your design's resilience, cohesiveness, coherence, and consistency. If your design has these quality attributes it will be easier to understand and maintain and that's really what you're looking for, not so much strict adherence to context-free rules like "Avoid getters because they break encapsulation."

In the UI, it might be convenient if you could just write someStudent.getType() to display an image or some text. Then again, a design that allows you to make a call like textFor(student.getType()) or iconFor(student.getType()) might be more resilient to change. By abstracting away the implementation logic into a single method call, you make the intent clearer and also limit the negative consequences of breaking encapsulation with a getType() method. Extracting to a method also makes your code more coherent and consistent. If there are changes that need to be made, all the changes can be made in just one place and it's easy to figure out where that one place is.

My code smell alarm might start buzzing immediately if I see code like this:

if (student.getType() == INTERNATIONAL) 
   // show "International student"

else if (student.getType() == DOMESTIC)
   // show "Domestic student"

else 
   // show "Unspecified"

If this same logic has been copy-pasted in a number of places throughout the code base, that would be bad for consistency and coherence. However, if I see it wrapped in a method and saw that other code consistently called that method then no red flags would be raised.

In the persistence layer, there might be one call to student.getType() to assign a value used in a parameterized query that writes/reads a Student table in the database. Alternatively, something like toTypeCode(student.getType()) that's used consistently by various persistence layer objects would not be a bad thing either.

UI and persistence are infrastructure concerns and using a method like student.getType() to reflect the state of a Student object probably doesn't entail significant negative consequences related to breaking encapsulation. On the other hand, preserving encapsulation might be a bigger concern when it comes to domain and service objects. Deciding whether code like this:

if (student.getType() == INTERNATIONAL) {
    // do something specific to Int'l
} else if (student.getType() == DOMESTIC) {
    // do something specific to Domestics
} ...

has significant consequences depends on where it's coded and how many times it appears in domain/business layer classes. If you put this code in Student, you have to see if it will make Student less cohesive and thus more brittle/less resilient in the face of changing requirements. Those are the same considerations if you decide to put that code in something like a Policy object or maybe in a Registrar object.

It really boils down to choosing a design that it is consistent with your mental model, it fits the "story" that the code tells, and helps people understand and follow what's going on in the code.