# 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.

public class GroupBillingPayment
{
public void Save(IGroupBillingPayment model)
{
if (model == null || UserInfo.UserID == 0)
{
}
Data.GroupBillingPayment groupBillingPayment = RepositoryManager.GroupBillingPaymentRepository.GetById(model.GroupBillingPaymentID);
Mapper.Map(model, groupBillingPayment);
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 this).

• Welcome to CR! I'd suggest you give us a bit more context, e.g. where's Mapper coming from? Is RepositoryManager a static class? With GroupBillingPaymentRepository being a nested static class exposing a static Update method? Same with ServiceManager.GroupBilling, and what's [Parameters]? Where's UserInfo coming from? The more context you give reviewers, the better we can help you, without making guesses and assumptions. May 5 '17 at 14:27
• Hope I have provided the necessary details
– gvk
May 5 '17 at 14:41
• Thanks for the edit, that's better indeed, but I would have just provided the class' code exactly as you have it in the IDE; it's easier to picture UserInfo being a property when it shows up in the code block as such - and as a bonus if there's an issue with the definition or implementation of that property then reviewers can point it out, too! Note that edits are always better off seamless; people that want to see the edit history can always see it by clicking the "edited x mins ago" timestamp. May 5 '17 at 14:47

if (model == null || UserInfo.UserID == 0)
{
}


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))
{
...
}