Save model to database

I recently asked a question on Software Engineering and I had a comment:


The updated version has some issues which are out of topic here. You may consider posting it at codereview.stackexchange.com to get some suggestions for improvements.

The code I had asked in that site:

public class GroupBillingPayment
{
    public void Save(IGroupBillingPayment model)
    {
        if (model == null || UserInfo.UserID == 0)
        {
            throw new Exception("GroupBillingPayment object or Current User Id is NULL , Please Contact Administrator.");
        }
        Data.GroupBillingPayment groupBillingPayment = RepositoryManager.GroupBillingPaymentRepository.GetById(model.GroupBillingPaymentID);
        Mapper.Map(model, groupBillingPayment);
        ServiceManager.GroupBilling.IsBillAlreadyCancelled(groupBillingPayment.GroupBillingID, THROW_ERROR);
        groupBillingPayment.UpdatedBy = UserInfo.UserID;
        groupBillingPayment.UpdatedOn = DateTime.Now;        
        RepositoryManager.GroupBillingPaymentRepository.Update(groupBillingPayment, false);
        UpdateGroupBilling(groupBillingPayment.GroupBillingPaymentID, groupBillingPayment.GroupBillingID)
    }
}

Could anyone please highlight the issues in this method?

A bit more details about the above method:

ServiceManager and RepositoryManager are private properties, which have been injected into this class via constructor using Autofac.

The Service/Repository managers contain the reference to the classes in that particular layer. Hence ServiceManager can access the service GroupBilling which is in service layer and RepositoryManager can access GroupBillingPaymentRepository.

The classes in service and repository layers are resolved using Autofac and so is UserInfo.

[Parameters] are the necessary parameters required for that method to run. In this case, it is groupBillingPayment.GroupBillingPaymentID and groupBillingPayment.GroupBillingID (have updated the code to reflect

if (model == null || UserInfo.UserID == 0)
{
  throw new Exception("GroupBillingPayment object or Current User Id is NULL , Please Contact Administrator.");
}

You should split this into two ifs and two different exceptions.

The first one should only check the parameter passed and throw the ArgumentNullException because we don't throw the Exception directly but some more specific ones that convey more information.

The other one should be the InvalidOperationException which means that the object is in an invalid state which would be when the UserInfo.UserID is 0.

ServiceManager.GroupBilling.IsBillAlreadyCancelled(
       groupBillingPayment.GroupBillingID,
       THROW_ERROR);

If the second parameter does what I think it does, this is, indicates whether the method should throw an exception or not then this is a very bad idea. If you need a method that does not throw then you should create another one by following the try-something pattern.

if(ServiceManager.GroupBilling.TryGetBillAlreadyCancelled(
       groupBillingPayment.GroupBillingID,
       out bool canceled))
  {
      ...
  }

StackExchange Code Review Q#162608, answer score: 4