• Post Reply Bookmark Topic Watch Topic
  • New Topic

Code Review  RSS feed

 
Andy Rosemond
Ranch Hand
Posts: 31
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I posted my code on Stack Exchange code review since yesterday, but so far no one has reviewed it. I was wondering if anyone would review it, and give me feedback.

I re-read the SOLID principles and refactored my code. I wanted to keep my Student class immutable, but that meant I would have to change the class every time a new Student type was introduced, that violated the S in SOLID. Here is what I came up with.






My requirements are:

    Domestic students don't require documentation
    International student do require documentation (passports, etc..)

Domestic Implementation:



International Implementation:



If there is any new type of status, it can be added my implementing StudentStatus, no need to modify the Student class.


Use:

 

I'm aware none of my classes have validation, I kept these out to keep my code clear and concise.

Goal: Keep the Student immutable without violating SOLID.
 
Andy Rosemond
Ranch Hand
Posts: 31
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'm aware none of my classes have validation, I kept these out to keep my code clear and concise.


Obviously, I mean the example code above I posted, my actual code will have validation.
 
Carey Brown
Saloon Keeper
Posts: 3243
46
Eclipse IDE Firefox Browser Java MySQL Database VI Editor Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

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
author & internet detective
Sheriff
Posts: 37393
531
Eclipse IDE Java VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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.
     
    Andy Rosemond
    Ranch Hand
    Posts: 31
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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.


    How about this?

    Domestic/International:




    International:
     
    Andy Rosemond
    Ranch Hand
    Posts: 31
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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.


    Fixed!

     
    Junilu Lacar
    Sheriff
    Posts: 11432
    175
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Why do you wrap the documents field in an ArrayList when you pass it to unmodifiableCollection?
     
    Andy Rosemond
    Ranch Hand
    Posts: 31
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Junilu Lacar wrote:Why do you wrap the documents field in an ArrayList when you pass it to unmodifiableCollection?


    So that it remains immutable. If I don't the client can modify the collection after object construction.
     
    Junilu Lacar
    Sheriff
    Posts: 11432
    175
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    The collection returned by unmodifiableCollection is already, well, unmodifiable, so I don't see a need to add an additional layer with an ArrayList
     
    Andy Rosemond
    Ranch Hand
    Posts: 31
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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


    I had it without the ArrayList before, just



    but a comment above mentioned it was still mutable.
     
    Junilu Lacar
    Sheriff
    Posts: 11432
    175
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    That comment was in reference to your Domestic where you returned this.documents
     
    Andy Rosemond
    Ranch Hand
    Posts: 31
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Junilu Lacar wrote:That comment was in reference to your Domestic where you returned this.documents


    AH! thanks.
     
    Liutauras Vilda
    Marshal
    Posts: 4795
    330
    BSD
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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
    Sheriff
    Posts: 11432
    175
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Design principles are meant to provide guidelines for keeping software soft, that is, easy to change. When you violate a design principle, some aspect of your design becomes a little more difficult to change. Context matters though. When you're still learning about your problem, you shouldn't let principles like OCP keep you from changing your design. Like I said previously, our first design choices are usually not the best ones and that's only natural because we don't know enough about our problem when we make those initial choices.

    This is why I like writing tests to drive my design thinking and choices. Tests are a good way to see how your design decisions work in practice. Tests give you a better idea of the contexts that may require you to change your code down the road. For example, when I first learned about LSP, I had a tendency to create a lot of interfaces because of their flexibility. Flexibility is good but too much of a good thing isn't good either. The cost of flexibility is complexity and tests can help you get a better feel for the costs you may have to incur. So you have to find a balance.

    Is there a compelling reason for making the StudentStatus interface? Do you have examples of client code that uses StudentStatus that made you think this would be a good abstraction? What led you to decide that Domestic implements StudentStatus and International implements StudentStatus? I'm trying to imagine the test code that can be written based on this design choice:

    I can't really make sense of the semantics of the above code, yet the design tells me that's something I can write. (EDIT: I realized why, see my next reply below). Semantics are just as important, if not more, as mechanics so just because it may seem like having Domestic implement an interface like StudentStatus would make it convenient to write some kind of code, you also have to consider whether the resulting code makes sense in the abstract. In this case, I'd say that convenience came at the cost of poor semantics and that's not a very good tradeoff.

    In contrast, when the design says ArrayList implements List, it's easy to imagine why you'd want to write this:

     
    Junilu Lacar
    Sheriff
    Posts: 11432
    175
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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
    Sheriff
    Posts: 11432
    175
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Andy Rosemond wrote:Goal: Keep the Student immutable without violating SOLID.

    I think it's good you're trying to adhere to principles like SOLID but somehow I feel like you're going at it from the wrong angle and I think your goal statement has something to do with it.

    The phrase "immutable without violating SOLID" seems to put immutability at odds with adhering to SOLID principles. In other words, it seems to imply that keeping a class immutable can somehow violate one or more SOLID principles. I don't see that there's any significant correlation between immutability and keeping with SOLID principles.

    How do you see keeping immutability as being directly related to either violating or adhering to one or more of the SOLID principles?
     
    Junilu Lacar
    Sheriff
    Posts: 11432
    175
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Also, keep in mind that SOLID are not the only principles you want to consider. There's also DRY, SLAP, and GRASP to name a few.
     
    Campbell Ritchie
    Marshal
    Posts: 56201
    171
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    You also got some suggestions on the other website; maybe you would like to say what you think of them.
     
    Andy Rosemond
    Ranch Hand
    Posts: 31
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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.
     
    Andy Rosemond
    Ranch Hand
    Posts: 31
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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.



    I think I grasp what you mean here, I have 2 mechanisms that do the same thing, both the class and the enum are telling the client exactly what student type is being created. However, remember I want to forbid the client from passing documents to a Domestic student.

    How would you refactor my code such that:

    1. The user not client can see the student is in fact a domestic or international(For example, in a GUI, how would I grab the international/domestic attribute to display it, if it wasn't an enum).
    2. Keep client code from passing documents to a Domestic Student.

    The way I have it now, I don't have to check in my student class what student was created, the interface and it's implementation worry about that. Given that I've tried, how would you refactor it? Domestic students don't get a documents collection passed in the constructor, because my requirements forbid it, that responsibility falls to the Domestic class implementation of StudentStatus.
     
    Junilu Lacar
    Sheriff
    Posts: 11432
    175
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    One thing I'd do to start getting an idea of how to refactor the code is to write some tests. Test help you develop a better idea of the context in which your class will be used. Seeing sample code is better than just imagining how your class might be used. Even if the sample code is wrong, it's easier to see what's wrong when it's staring you in your face. Your brain has a way of smoothing out the rough edges of ideas when they're just floating around in your imagination.

    To experiment with some possible usage scenarios, I'd write some tests using JUnit.

    That could be one starting point. Looking at those test names, I might ask, "What does code to create domestic vs. international students look like? How would this code enforce the rule? How do I handle errors?"

    The test code would be examples of what your client code would look like.  When you've written some of the ideal client code, then you have a better idea of how your class looks from the outside (its API) and that gives you a better idea of how it would look inside (its internal structure/design).
     
    Junilu Lacar
    Sheriff
    Posts: 11432
    175
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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?
     
    Junilu Lacar
    Sheriff
    Posts: 11432
    175
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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.

    Here are some things I might try:

    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.
     
    Andy Rosemond
    Ranch Hand
    Posts: 31
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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?


    Yes, that's what I mean.

    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.

    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.


    Given this code, I like alternative #1, because I’m not using it to determine any type of decision. Maybe a combination of #1 and #2, the getter for display in a GUI and #2 if someone in the institution might what a list of domestic/international students, I don’t have to write logic in client code to work that out, I can just call hasStatusOf which will return a boolean. My concern is displaying the type in the GUI. Why?

    When I was in school, from time to time we had to visit registration members to help with timetables, enrollment, etc... Depending on if the student was international or domestic, the payment might be handled differently. Before meeting with the registration employee it was often good practice to have your Student ID out, because they would enter your ID #  in a search field and pull up your data. Ideally, I would want the data to show that the student in front of them is international, that way if they can’t handle a certain matter(Regardless of domestic/international some situations the regular registration employee can do it), the employee sitting behind a desk meant for international students can handle it. 
     
    Andy Rosemond
    Ranch Hand
    Posts: 31
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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.


    Who should be responsible? Obviously, as I said in my other comment, a Student has documents. My primary concern is, if I wanted to display the documents in GUI.

    Given this situation:

    Suppose I'm an international Student at a registration desk enrolling in a new semester. Now, the employee that handled my first semester enrollment is not there. So now I'm face to face with another registration employee and as institution procedure he wants to verify that the documents I provided the first time I enrolled, are still valid. The registration employee can pull up my profile and see that I used my passport, and maybe driver's license to complete my registration the first time. Obviously the employee doesn't care about the code, all he cares about is what he sees on the screen.  

    However, if we wanted to think about code, I would have Student object with the name, and status, and documents that the student provided. To display, I would need a getter to return a List of documents that the student gave the institution. In your case, where would the getter go if not in StudentStatus?
     
    Junilu Lacar
    Sheriff
    Posts: 11432
    175
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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.

    The one thing with What-Ifs is that once you start down that road, it can quickly become turtles all the way down.  At some point, you have to consider what Sandi Metz tweeted: Don't write code that guesses the future, arrange code so you can adapt to the future when it arrives.

    When you try to write code that can be very flexible, you're trying to guess the future. Arranging code means to make it more or less as loosely coupled for what you need it to do today and as clean and clear so that it's going to be easier to understand and refactor in the future. Flexibility has an overhead cost of complexity. And sometimes, the future that we imagine might come never does. So we waste a lot of time and effort trying to prepare for every possible future scenario (that's a lot of turtles).

    Let's assume that you still think it's prudent to anticipate new student types in the near future. In fact, let's assume that you were already told that this might happen in a year or so but not right now. For now, we just want to be able to handle Domestic and International. Ok, fine.

    For me, the simplest solution is still to have a StudentType enum that defines the values {DOMESTIC, INTERNATIONAL}. Later on, I can modify the enum to include additional types. You might think that would constitute a violation of OCP but changing code is not what the principle is mainly about. OCP is about limiting ripple effects of changes.  If you can add more enum values later on without those changes rippling all throughout your application in the form of additional, unexpected changes you have to make, then OCP was irrelevant. If you do get a lot of ripple effects, then OCP was probably meaningfully violated.

    The choice to put validation code pertaining to requiring documents based on StudentType is probably the biggest concern, not because it violates OCP but because it will add a specialized responsibility to the Student class, which is more a violation of SRP. The lifetime/scope of Student is different from that of a documentation requirement rule, so I would probably first try to delegate that responsibility to another related class, say something like a Rule class. I might try out this design:

    Then in the Student class, you could write something like this:

    Now I can write any kind of Rule that pertains to Student objects.  Even this design is too big of a step but it demonstrates my point of using SRP as the motivation for refactoring to limit ripple effects of changes in one part of the design/code.

    Again, you need to experiment with any ideas like this. I like writing lots of tests that draw up different usage scenarios. I look at the test code and see if it represents a reasonable and coherent example of client code that I would like to write myself or that others would find natural and intuitive to write.
     
    Junilu Lacar
    Sheriff
    Posts: 11432
    175
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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
    Ranch Hand
    Posts: 31
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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.


    I agree with this! 

    As a comment mentioned on the other page, abstracting away StudentStatus might not be the best solution. The reason is, if my interface where to change then I would also have the change the classes that implement that interface, and the Student class. Right now the Student class has methods that just call the interface methods retrieveStatus and retrieveDocuments, but if added a new method to my interface, then Student would have to be changed to accommodate the interface. In a way I'm back to square one, planning and testing out potential ideas. 

    This is just a hobby project I'm using to learn OOP and design, but in production code, this would be a nightmare. 
     
    Liutauras Vilda
    Marshal
    Posts: 4795
    330
    BSD
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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.

    Being aware isn’t the same as being responsible for. Single responsibilty principle says class should have only one reason to change. For what reasons student class may change?

    Imagine register office from now on does not issue any documents to students, instead, they give a one off temporary key to connect to cloud where you can download all documents you are associated with. Would that influence your student class? Why? What student has to do with the way register office works?

    Who would you expect to implement such change, so the student would be immutable from such work procedures changes?

    Apart from the fact that documents are very ambiguous term or maybe too broad at worst, what they have to do with student status? Are there any strong relations between the two? If there are, why Domestic doesn’t have them? Half of the student statuses you have, documents do not apply, so you polute them with functions you are not going to need them.

    You also have method retrieveStatus(), which returns student type. That is mystifying.

    I’ll participate more tomorrow, been away today.



     
    Andy Rosemond
    Ranch Hand
    Posts: 31
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    You also have method retrieveStatus(), which returns student type. That is mystifying.


    If you're referring to the fact that a method with the term Status in it returns a type, I've corrected it with a more appropriate name.

    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?

    How would you refactor the code?
     
    Liutauras Vilda
    Marshal
    Posts: 4795
    330
    BSD
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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?

    To me looks that you over-complicated that mechanism with Student class. You implemented Student class such way, in particular talking about student type at the moment, that it would be open for extension but closed for modification (2nd principle in SOLID). You retrieving this status from somewhere else, meaning it could retrieve something else every time you call this function and that implies that implementation of retrieving student status might change over time, so you wouldn't need to modify it. But does the student type really changes across the study years, International <-> Domestic; Domestic <-> International? I'd think - no. So why just don't have student type as its property? Also why not merge first name and last names to a one parameter? It is mandatory to provide full name. In case you need first and last names only in some cases, you can handle that in Student class.

    Now you have an immutable Student class, which is simple to understand, contains it seems what it supposed to contain and has responsibilities of providing information about the student.

    Who are creators, owners of documentation? Who are responsible to create them? Who are responsible to give it to you if you lost them? Who the police would ask the documents if they would need to get some information about you? Probably register office. How register office can know how to find the documents associated with you? Well, student has some identifier, right? You also have full name in case search needs to be performed.

    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.

    If student is aware of documentation, that doesn't mean you need to load everything to Student class what student is aware of. According to your logic... student is also aware of his library pass, that way you'd need also getter for that in case librarian would ask you for such. But in real you don't need that, you give your ID only, and access database checks if your ID has associated access to library premises or not. You are also aware of your student finance documents and all invoices you have been issued, so you'd need attributes for that also. But in real you don't need that either, university's financial department stores all that information, you just need to give them your student ID and they would retrieve it for you. Physically having something as documents, from systems perspective doesn't mean you need to provide all getters and setters to a particular entity for that, that way you'd just pollute your class with details which may change at any given point in time. When such changes happen, what you want to achieve, is that it changes only in the departments where these changes need to be actually handled.

    Program always to interface isn't a design prerequisite. Design prerequisite is to have everything simple to understand and simple to change when the changes are needed. When such changes need to be made, you want those changes appear to be in one particular place, but not spread across all the places within your system.

    If you'd leave your student class as you have now, it would get broken when the process of issuing documentation would change. So you'd need to modify all your StudentStatus implementations + Student class and that violates Single Responsibility Principle, because Student class has more than one reason to change, its changes can be influenced by other departments which student has not much to do with them.

    When changes required to be made in many different places because of your design - your system becomes fragile and rigid, so every time you change something in place A, you'd risk to break things in place B, C and D. It would be even hard to predict where things would break, hence you wouldn't want to touch those parts. In effect of that you'd likely need to introduce another interface leaving all those old functions unused and deprecated. But as you see it is sounds plainly wrong already.
     
    Andy Rosemond
    Ranch Hand
    Posts: 31
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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.


    Given this code, and I like it 

    Suppose you wanted to add an international Student(Ideally, this information would be stored in a database somewhere, but this is a hobby project after all, so local storage is all we have).  UCLRegisterOffice would hide a Map, and given Student is immutable you can use it as the key, or even the Student ID(I doubt that changes over the lifetime the student is at the school).

    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
    Sheriff
    Posts: 11432
    175
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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.

    One important principle that you haven't talked about much is DRY (Don't Repeat Yourself). There's a common misconception that DRY is about mechanical character-for-character duplication of code. While it often can be the main problem, mechanical duplication isn't what DRY is about. DRY is really about duplication of ideas. Duplicating an idea across your code can lead to mechanical letter-for-letter duplication (e.g., when you practice copy-paste programming) but violations of DRY can be more insidious and harder to detect when the same idea is expressed in different ways throughout your code.

    In the code you gave, the red flag that DRY might be violated is this: "overload... with almost the same logic." 

    For one thing, the motivation/intent of that code is hidden in the implementation details. The intent is to require documents for some and disallow it for others. That intent is not explicitly stated though and is buried in a formula that checks the student.type against a specific value. Someone coming in fresh probably won't be able to understand the intent without some parsing and reading between the lines. Try to find how many more times you are going to write this kind of logic in your program to accomplish the same thing. If there are multiple instances of this kind of expression spread throughout your code then any changes in the rule will have to be reflected in all those different places. That's why the opposite of DRY code is WET code (We Enjoy Typing or Write Everything Twice).

    One first step to refactoring this is by extraction and abstraction - turning a formula like if (student.getType() == INTERNATIONAL) that only implies the rule to something that's more explicit. Extract Method is a refactoring commonly associated with WET code/designs.
     
    Junilu Lacar
    Sheriff
    Posts: 11432
    175
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    In his book Understanding the Four Rules of Simple Design, Corey Haines gives an example of how mechanical character-for-character duplication may not constitute a violation of DRY.

    The book studies coding and design alternatives for Conway's Game of Life.

    One rule of the game is that if an empty cell has exactly 3 live neighbors, that cell will be live in the next generation. Another rule is that a live cell lives on to the next generation if it has 2 or 3 live neighbors.

    At first glance, you might think that there is some duplication that needs to be refactored away in this code:

    When you see neighbors == 3 twice in that expression, you might think of extracting it out or creating a symbolic constant. However, the first neighbors == 3 represents "fertility" whereas the second one represents "stability" so these are really two different ideas that just happen to boil down to the same expression. There is no violation in DRY in this case. However, the code could still be refactored to :

     
    Andy Rosemond
    Ranch Hand
    Posts: 31
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Junilu Lacar wrote:


    Let the refactoring begin...

    I like the first option after the OR.

    Question:
    First let me ask this, I violated encapsulation because I used the getter(StudentType) to make a decision or calculation that would be better inside the Student class rather than the outside(in main, or another class), correct?

    Refactored Code:



    Just out curiosity and for general learning this code:



    This implies a private method within the UCLRegistar class, but won't you still have to call student.GetType()? or are you saying within that private method, do one of these:



     
    Liutauras Vilda
    Marshal
    Posts: 4795
    330
    BSD
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Andy Rosemond wrote:

    That could be simplified as:

    But then I'd question myself, 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
    Sheriff
    Posts: 11432
    175
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    The options I gave are just experiments. Or maybe call them hypotheses from which I could conduct experiments. I said multiple times that context matters. Starting with those options as hypotheses, I would try to expand the code out so I can see more context in which they would be used. The first option was really the worst one and I intentionally did that to bolster my assertion that your first design choice isn't always the best one. Once you realize that though, it's easier to find better alternatives. I actually like starting from a poor choice because it gives me a baseline for determining if other choices are better or worse.

    Another thing I have repeatedly said is that there are no absolutes. Choosing to go with student.requiresDocuments may be good enough for now, if that's what the current contexts for use tell you works. Later on, that choice might prove to be more problematic if the context changes and revisiting the decision and motivations might lead to a different choice. Having clear and well-factored code makes the transition easier. Principles are not hard and fast rules though, that's why they're called principles and not recipes.
     
    Junilu Lacar
    Sheriff
    Posts: 11432
    175
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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).
     
    Andy Rosemond
    Ranch Hand
    Posts: 31
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    The first option was really the worst one and I intentionally did that to bolster my assertion that your first design choice isn't always the best one. .

    When you say first option was the worst you mean this?

     
    Andy Rosemond
    Ranch Hand
    Posts: 31
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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 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.
     
    • Post Reply Bookmark Topic Watch Topic
    • New Topic
    Boost this thread!