locked
Solid principal violation RRS feed

  • Question

  • Hi All,

    As per the of SOLID principles, what’s wrong of the below class ? 

    I need your help to refactor below class.

    Employee.cs

    Thanks in advance!

    Thursday, April 23, 2020 1:03 PM

Answers

  • Unto itself I don't see any serious violation as all the members are focused on this employee which means you only open the class to add functionality to employee. To be honest finding any class that is SOLID and also follows OOP is hard. Academics have to cede to real world technologies. Even the posted samples of "SOLID" types tend to either be unrealistically simple or aren't actually completely SOLID.

    However from a maintainability point of view the fact that it requires access to a DB to get things like a manager's name is just wrong. I don't believe this type is properly encapsulated and that is where the refactoring should occur. But it depends where you're at in the layers. SOLID is a good principal in business types but isn't nearly as useful in UI or DTO types. DTOs are just that so they don't really need SOLID. UI types tend to have framework requirements which makes it hard.

    In your case I'd argue that the DB stuff should be removed altogether. This type seems to be having a clash in personality. For name it is assuming the value won't change for the life of the object but for manager and address it is assuming that it could change every time it is called so it keeps requiring the DB. Types should either be cached copies (most common) or completely dynamic (clients). So in this case I'd say the manager and address methods should be replaced with regular properties that are set when the object is created. 

    One of things to consider is how DDD you are going to get. One of the concepts in DDD is always valid. So you should never be able to set an employee to an invalid state. Hence why they tend to use methods for everything. Personally I think having an invalid employee is fine (otherwise how can you build progressive UIs) but validation does have to happen at some point.  So your persistence layer needs to validate changes to the manager/address info if needed.

    The only remaining member is SendCodeReview. This is where SOLID and OOP collide in my opinion. OOP says put functionality on the type so the method definitely belongs here. But implementing that method requires DB access so now you're mixing concerns again. I don't think there is a good answer here. DDD folks would mention using domain events to solve this problem. Other folks would recommend a service layer but then you're getting into anemic models which isn't good either.

    To me it is a matter of choice. In order to create an employee I need a DB object even if I never intend to send a code review. That seems wrong to me. I would potentially argue that this method should accept the DB/service that implements the functionality as a parameter and then it does any validation and calling itself. But another approach would be to move that into a service class or even an extension method. It depends on how "important" it is for the Employee type to be involved in its data.


    Michael Taylor http://www.michaeltaylorp3.net

    • Marked as answer by Pratap09 Friday, April 24, 2020 9:17 AM
    Thursday, April 23, 2020 2:15 PM
  • I think we probably would want to have a pure Employee class with just its common properties and EmployeeData class that will return the Employee with all the actual data using database access layer.

    For every expert, there is an equal and opposite expert. - Becker's Law


    My blog


    My TechNet articles

    • Marked as answer by Pratap09 Friday, April 24, 2020 9:17 AM
    Thursday, April 23, 2020 5:18 PM

All replies

  • Hello,

    I suggest reading and understanding the following then evaluate your code

    https://blog.bitsrc.io/solid-principles-every-developer-should-know-b3bfa96bb688


    Please remember to mark the replies as answers if they help and unmarked them if they provide no help, this will help others who are looking for solutions to the same or similar problem. Contact via my Twitter (Karen Payne) or Facebook (Karen Payne) via my MSDN profile but will not answer coding question on either.

    NuGet BaseConnectionLibrary for database connections.

    StackOverFlow
    profile for Karen Payne on Stack Exchange

    Thursday, April 23, 2020 1:57 PM
  • Unto itself I don't see any serious violation as all the members are focused on this employee which means you only open the class to add functionality to employee. To be honest finding any class that is SOLID and also follows OOP is hard. Academics have to cede to real world technologies. Even the posted samples of "SOLID" types tend to either be unrealistically simple or aren't actually completely SOLID.

    However from a maintainability point of view the fact that it requires access to a DB to get things like a manager's name is just wrong. I don't believe this type is properly encapsulated and that is where the refactoring should occur. But it depends where you're at in the layers. SOLID is a good principal in business types but isn't nearly as useful in UI or DTO types. DTOs are just that so they don't really need SOLID. UI types tend to have framework requirements which makes it hard.

    In your case I'd argue that the DB stuff should be removed altogether. This type seems to be having a clash in personality. For name it is assuming the value won't change for the life of the object but for manager and address it is assuming that it could change every time it is called so it keeps requiring the DB. Types should either be cached copies (most common) or completely dynamic (clients). So in this case I'd say the manager and address methods should be replaced with regular properties that are set when the object is created. 

    One of things to consider is how DDD you are going to get. One of the concepts in DDD is always valid. So you should never be able to set an employee to an invalid state. Hence why they tend to use methods for everything. Personally I think having an invalid employee is fine (otherwise how can you build progressive UIs) but validation does have to happen at some point.  So your persistence layer needs to validate changes to the manager/address info if needed.

    The only remaining member is SendCodeReview. This is where SOLID and OOP collide in my opinion. OOP says put functionality on the type so the method definitely belongs here. But implementing that method requires DB access so now you're mixing concerns again. I don't think there is a good answer here. DDD folks would mention using domain events to solve this problem. Other folks would recommend a service layer but then you're getting into anemic models which isn't good either.

    To me it is a matter of choice. In order to create an employee I need a DB object even if I never intend to send a code review. That seems wrong to me. I would potentially argue that this method should accept the DB/service that implements the functionality as a parameter and then it does any validation and calling itself. But another approach would be to move that into a service class or even an extension method. It depends on how "important" it is for the Employee type to be involved in its data.


    Michael Taylor http://www.michaeltaylorp3.net

    • Marked as answer by Pratap09 Friday, April 24, 2020 9:17 AM
    Thursday, April 23, 2020 2:15 PM
  • I think we probably would want to have a pure Employee class with just its common properties and EmployeeData class that will return the Employee with all the actual data using database access layer.

    For every expert, there is an equal and opposite expert. - Becker's Law


    My blog


    My TechNet articles

    • Marked as answer by Pratap09 Friday, April 24, 2020 9:17 AM
    Thursday, April 23, 2020 5:18 PM