Constructive criticism on this class

Question

Hi, I've just reviewed some code that looked like this before

public class ProductChecker
{
     // some std stuff
     public ProductChecker(int AccountNumber)
     {
         var account = new AccountPersonalDetails(AccountNumber);
         //Get some info from account and populate class fields
     } 
     public bool ProductACriteriaPassed()
     {
          //return some criteria based on stuff in account class 
          //but now accessible in private fields
     }

}

There has now been some extra criteria added which needs data not in the AccountPersonalDetails class

the new code looks like this

public class ProductChecker
{
     // some std stuff
     public ProductChecker(int AccountNumber)
     {
         var account = new AccountPersonalDetails(AccountNumber);
         var otherinfo = getOtherInfo(AccountNumber)
         //Get some info from account and populate class fields
     } 
     public bool ProductACriteriaPassed()
     {
          //return some criteria based on stuff in account class  
          // but now accessible in private fields and other info
     }

     public otherinfo getOtherInfo(int AccountNumber)
     {
        //DIRECT CALL TO DB TO GET OTHERINFO 
     }

}

I'm bothered by the db part but can people spell out to me why this is wrong? Or is it?

Answer 1

It seems like the extra data grabbed in getOtherInfo should be encapsulated as part of the AccountPersonalDetails class, and thus already part of your account variable in the constructor when you create a new AccountPersonalDetails object. You pass in AccountNumber to both, so why not make AccountPersonalDetails gather all the info you need? Then you won't have to tack on extra stuff externally, as you're doing now.

Answer 2

It definitely looks like there might be something going haywire with the design of the class...but it's hard to tell without knowing the complete architecture of the application.

First of all, if the OtherInfo object pertains to the Account rather than the Product you're checking on...it's introducing responsibilities to your class that shouldn't be there.

Second of all, if you have a Data Access layer...then the ProductChecker class should be using the Data Access layer to retrieve data from the database rather than making direct calls in to retrieve the data it needs.

Third of all, I'm not sure that the GetOtherInfo method needs to be public. It looks like something that should only be used internally to your class (if, in fact, it actually belongs there to begin with). In that case, you also shouldn't need to pass around the accountId (you class should hold that somewhere already).

But...if OtherInfo pertains to the Product you're checking on AND you have no real Data Access layer then I can see how this might be a valid design.

Still, I'm on your side. I don't like it.

Answer 3

considering that an accountNumber was passed into the constructor you shouldn't have to pass it to another method like that.

Answer 4

In a layered view of your system, it looks like ProductChecker belongs to the business rules / business logic layer(s), so it shouldn't be "contaminated" with either user interaction functionality (that belongs in the layer(s) above) or -- and that's germane to your case -- storage functionality (that belongs in the layer(s) below).

The "other info" should be encapsulated in its own class for the storage layers, and that class should be the one handling persist/retrieve functionality (just like I imagine AccountPersonalDetails is doing for its own stuff). Whether the "personal details" and "other info" are best kept as separate classes or joined into one I can't tell from the info presented, but the option should be critically considered and carefully weighed.

The rule of thumb of keeping layers separate may feel rigid at times, and it's often tempting to shortcut it to add a feature by miscegenation of the layers -- but to keep your system maintainable and clean as it grows, I do almost invariably argue for layer separation whenever such a design issue arises. In OOP terms, it speaks to "strong cohesion but weak coupling"; but in a sense it's more fundamental than OOP since it also applies to other programming paradigms, and mixes thereof!-)

Answer 5

A few points

• The parameter names are pascal case, instead of camel (this maybe a mistake)
• getOtherInfo() looks like it's a responsibility of AccountPersonalDetails and so should be in that class
• You may want to use a Façade class or Repository pattern to retrieve your AccountPersonalDetails instead of using a constructor
• getOtherInfo() may also be relevant for this refactor, so the database logic isn't embedded inside the domain object, but in a service class (the Façade/Repository)
• ProductACriteriaPassed() is in the right place

Answer 6

I would recommend this:

public class AccountPersonalDetails
{
   public OtherInfo OtherInfo { get; private set; }
}

public class ProductChecker
{
     public ProductChecker(AccountPersonalDetails) {}
}

// and here's the important piece
public class EitherServiceOrRepository
{
   public static AccountPersonalDetails GetAccountDetailsByNumber(int accountNumber)
   {
       // access db here
   }

   // you may also feel like a bit more convinience via helpers
   // this may be inside ProductCheckerService, though
   public static ProductChecker GetProductChecker(int accountNumber)
   {
      return new ProductChecker(GetAccountDetailsByNumber(accountNumber));
   }
}

I'm not expert in Domain-Driven Design but I believe this is what DDD is about. You keep your logic clean of DB concerns, moving this to external services/repositories. Will be glad if somebody correct me if I'm wrong.

Answer 7

Whats good. It looks like you have a productChecker with a nice clear purpose. Check products. You'd refactor or alter this because your have a need to. If you don't need to, you wouldn't. Here's what I would probably do.

It "feels" clunky to create a new instance of the class for each account number. A constructor argument should be something required for the class to behave correctly. Its a parameter of the class, not a dependency. It leads to the tempation to do a lot of work in the constructor. Usage of the class should look like this:

result = new ProductChecker().ProductACriteriaPassed(accountNumber)

Which I'd quickly rename to indicate it does work.

result = new ProductChecker().PassesProductACriteria(accountNumber)

A few others have mentioned that you may want to split out the database logic. You'd want to do this if you want unit tests that are fast. Most programs want unit tests (unless you are just playing around), and they are nicer if they are fast. They are fast when you can get the database out of the way.

Let's make a dummy object representing results of the database, and pass it to a method that determines whether the product passes. If not for testibility, this would be a private. Testability wins. Suppose I want to verify a rule such as "the product must be green if the account number is prime." This approach to unit testing works great without fancy infrastructure.

// Maybe this is just a number of items. 
DataRequiredToEvaluateProduct data =  // Fill in data
// Yes, the next method call could be static. 
result = new ProductChecker().CheckCriteria(accountNumber, data)
// Assert result

Now we need to connect the database. The database is a dependency, its required for the class to behave correctly. It should be provided in the constructor.

public class ProductRepository {} // Define data access here. 

// Use the ProductChecker as follows. 
result = new ProductChecker(new ProductRepository()).CheckCriteria(accountNumber)

If the constructor gets annoyingly lengthy (it probably has to read a config file to find the database), create a factory to sort it out for you.

result = ProductCheckerFactory().GimmeProductChecker().CheckCriteria(accountNumber)

So far, I haven't used any infrastructure code. Typically, we'd make the above easier and prettier with mocks and dependency injection (I use rhinomocks and autofac). I won't go into that. That is only easier if you already have it in place.