I'm aware none of my classes have validation, I kept these out to keep my code clear and concise.
[OCP 17 book] | [OCP 11 book] | [OCA 8 book] [OCP 8 book] [Practice tests book] [Blog] [JavaRanch FAQ] [How To Ask Questions] [Book Promos]
Other Certs: SCEA Part 1, Part 2 & 3, Core Spring 3, TOGAF part 1 and part 2
Carey Brown wrote:
I have a problem with something referring to "status" returning a "type".
Your Domestic/International classes can leak a mutable reference to documents.
Jeanne Boyarsky wrote:Things I'd change:
Rename ID variable to id. It looks like a constant right now because it is in all capitals. This would also make iD into id which looks better in the constructor Similarly, firstname should be firstName to follow camel case and make the code easier to read. I'll let you guess what I think about lastname Assuming you are using Java 7 or higher, new ArrayList<String> can be new ArrayList<> - the diamond operator at work! I can't tell one thing. If StudentStatus's retrieve document logic makes a copy of the Collection, you are good. If not, you'd need to make a copy before turning it.
The code is good. It is clear and easy to read. Good job.
Junilu Lacar wrote:Why do you wrap the documents field in an ArrayList when you pass it to unmodifiableCollection?
Junilu Lacar wrote:The collection returned by unmodifiableCollection is already, well, unmodifiable, so I don't see a need to add an additional layer with an ArrayList
Junilu Lacar wrote:That comment was in reference to your Domestic where you returned this.documents
Andy Rosemond wrote:Goal: Keep the Student immutable without violating SOLID.
Liutauras Vilda wrote:In my opinion an interface of StudentStatus has something what it shouldn't have.
In effect of that, constructor injection of StudentStatus to Student class polutes Student class with something what Student shouldn't know about.
I wouldn't expect Student class to be responsible for retrieving any kind of documents. What these documents are?
I think your design violates very first principle of SOLID you mentioned.
Junilu Lacar wrote:One thing that certainly doesn't look right is that both Domestic and International have a StudentType type attribute. It's strange because StudentType appears to define both values of Domestic and International. Why would you have a class Domestic with a StudentType type attribute that can only logically be the value of StudentType.Domestic? Likewise, it doesn't make sense to have a class International whose StudentType type attribute can only logically be the value StudentType.International. At best, there's a redundancy between each subtype and its StudentType attribute.
It's similar to what's wrong with this design:
That isn't right. Assigning a sex attribute of Gender.MALE to a class named Man is redundant and so is assigning Gender.FEMALE to the sex attribute of a Woman class.
You should probably choose to use the subtype or the attribute to differentiate between Domestic vs. International, not both.
Junilu Lacar wrote:Please clarify what you mean by "user" vs. "client". Are you using "user" to refer to a person who is using the program and "client" to refer to code that uses the Student and related classes/types?
There are a number of ways you can design your API so that you can distinguish a Domestic vs. International student. Again, it's easier to evaluate the appropriateness of a choice if you see it in context.
// maybe like this
Student s = createRandomStudent(); // don't know if Domestic or International;
// alternative #1
if (s.getStatus() == DOMESTIC) { ... }
// alternative #2
if (s.hasStatusOf(DOMESTIC)) { ... }
// alternative #3
if (s instanceof Domestic) { ... }
Now that I can actually see these choices in front of me, I definitely find #3 yucky. So now it's down to #1 or #2. There might be other alternatives you can think of. If there are, write more example code so you can visually compare them with other choices.
Liutauras Vilda wrote:In my opinion an interface of StudentStatus has something what it shouldn't have.
In effect of that, constructor injection of StudentStatus to Student class polutes Student class with something what Student shouldn't know about.
I wouldn't expect Student class to be responsible for retrieving any kind of documents. What these documents are?
I think your design violates very first principle of SOLID you mentioned.
Andy Rosemond wrote:
Let me answer what I meant by immutable and keeping line with SOLID.
When I first started this project I had everything under Student(name, ID, and Status) and I had a private method check the type of student to ensure that domestic students can’t be passed documents by throwing an exception. However, as you can imagine, this wasn’t a good idea.
1. More status types can be added, which can be accompanied by new rules.
2. I would have to change my student class every time a new type was added.
All of the above violated the S and the O in SOLID.
This means I have to rethink my design, so I decided to abstract away the StudentStatus and make that an interface(although it might be more appropriate as an abstract class). This means, Student doesn’t have to change every time a new status was added. The class implementing the interface would have to be responsible for enforcing the rules.
Now, if you think about it, I could have had an abstract class Student and sub classes Domestic and International, but the problem is, if more types were added, one of those new types might be a branch of international, which means Student > Internation > NewInternationalType.
I don’t like deep inheritance, because I’ve read Effective Java and enough articles to become worried about inheritance. Now, of course, inheritance has it’s place, BUT, I don’t think this is one of those places.
Junilu Lacar wrote:In my example above, if I try to add a new StudentType enum value, I would look to make sure the Student class wouldn't have to change, and that there should be little to no changes made to existing implementations of Rule. If I see that this is the case, then I would say "Ok, this is good enough for now" and move on until I encounter another difficulty.
I think my approach is really not to start with the mindset of "I don't want to violate any principles." Rather, my mindset is "I don't want my code to be difficult to understand or change. I want my code to be clean and clear so I can easily understand it. If it's easy to understand, it's easier to figure out a way to change/refactor. If I get a new requirement and it's difficult to adopt my design to those new requirements, then I'm going to look for a principle that was violated and refactor accordingly."
This is similar to the approach Joshua Kerievsky takes in his book Refactoring to Patterns. Kerievsky says you shouldn't start with a pattern in mind and say "I'm going to use this pattern to create a solution." His approach is to just write clean, well-factored code, then his solution really fits the context of a know pattern, he'll try to refactor his code towards the known pattern.
Andy Rosemond wrote:
Liutauras Vilda wrote:In my opinion an interface of StudentStatus has something what it shouldn't have.
In effect of that, constructor injection of StudentStatus to Student class polutes Student class with something what Student shouldn't know about.
I wouldn't expect Student class to be responsible for retrieving any kind of documents. What these documents are?
I think your design violates very first principle of SOLID you mentioned.
But if you look at it from a real world perspective, the students are aware of the documentation(Passports, Driver's License, etc..). They are also aware that they gave those documents to the institution that requested them. If if a new student type were added, I no longer have to refactor my Student class or my interface.
You also have method retrieveStatus(), which returns student type. That is mystifying.
Andy Rosemond wrote:If you're referring to the fact that it's really a getter in hiding, then how would I display to the User(the person using the software, in this case the registration employee at the insistution responsible for handing Student registration among other things) that the Student standing in front of them in is fact an international student?
Liutauras Vilda wrote:
I might would think initially towards such implementation, which is way easier to understand and you don't pollute student with responsibilities which aren't his:
This implementation doesn't necessarily mean is correct either.
Andy Rosemond wrote:
So when you add a new Student, you would call the StudentType getter to ensure they are in fact international, so like this:
For Domestic, the UCLRegisterOffice might have an overload add method to accept domestic as well(minus documents), with almost he same logic.
Junilu Lacar wrote:
Andy Rosemond 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.
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).
Don't get me started about those stupid light bulbs. |