3
\$\begingroup\$

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 this).

\$\endgroup\$
3
  • 3
    \$\begingroup\$ 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. \$\endgroup\$ May 5 '17 at 14:27
  • \$\begingroup\$ Hope I have provided the necessary details \$\endgroup\$
    – gvk
    May 5 '17 at 14:41
  • 4
    \$\begingroup\$ 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. \$\endgroup\$ May 5 '17 at 14:47
4
\$\begingroup\$
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))
  {
      ...
  }
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.