-4
\$\begingroup\$

This code gets request status based on request type and some mapping info.

I'm wondering if there is a way to write nice conditional expressions in if statement. Expression should test if request.IsInvalid property equals null or false. It'd be nice that one can see just from looking at the expression that request.IsInvalid is nullable property. I'd appreciate comment on other sutff as well.

public RequestStatus GetAppropriateRequestStatus(PortfolioRequest request, PortfolioMapping mapping)
{
    // "cancel" request
    if (request != null && request.RequestType == 3) 
    {
        status = RequestStatus.Cancelled;
    }
    else if (mapping.Action == "exclude" && mapping.ActiveCurrent == 0 &&
         (request == null || request.RequestType != 3))
    {
        status = RequestStatus.Excluded;
    }
    // "update" request
    else if (request != null && request.RequestType == 2 && request.IsInvalid != true) 
    {
        status = RequestStatus.Modified;
    }
    else if (request != null && request.IsInvalid == true)
    {
        status = RequestStatus.Invalid;
    }
    else
    {
        status = RequestStatus.Error;
    }

    return status;
}

Side note: I don't like operator ! since it is harder to spot than == false and it usually makes all the difference.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I don't like operator ! since it is harder to spot than == false Perhaps, but it's still common usage in many language. Readability is good, but so is conciseness. Don't make it more complicated than it is. \$\endgroup\$ – Mast May 26 '15 at 10:15
2
\$\begingroup\$

If you have to put a comment to specify what a request type is then you should either change Request type to be a enum so it uses words, or if you can't do that then use a constant in your class that would change. I'm going to assume you can't change RequestType easily, so I'll just show using constants

private const int CancelRequest = 3;
private const int UpdateRequest = 2;
//...
if(request != null && request.RequestType == CancelRequest)

now you can get rid of the comment because the code is more clear.

second is that I would not let your logic drop down. In my mind I'm reading and seeing that you are setting status. but you don't return right away so in my mind I think that you have more logic you need to perform, but instaed you just skip through everything and return status. Ok well if that is the case then just return on the spot so I don't keep reading the next bit of code. then you can just either delete the else statement and return RequestStatus.Error, or remove return status.

third bit of logic that can be cleared up is when request is null. You check if it is not null in almost every if statement. Make it the first call, checking your other conditions then return.

fourth is that if a condition takes prescendence over another condition put it first in your else if chain then you don't have to test for it again.

With those 4 tips I would change your code to something like this.

private const int CancelRequest = 3;
private const int UpdateRequest = 2;

public RequestStatus GetAppropriateRequestStatus(PortfolioRequest request, PortfolioMapping mapping)
{
    if(request == null)
    {
        if (MappingActionIsToExclude(mapping))
            return RequestStatus.Excluded;
    }
    else if (request.RequestType == CancelRequest)
    {
        return RequestStatus.Cancelled;
    }
    else if (MappingActionIsToExclude(mapping))
    {
        return RequestStatus.Excluded;
    }
    else if (request.IsInvalid)
    {
        return RequestStatus.Invalid;
    }
    else if (request.RequestType == UpdateRequest)
    {
        return RequestStatus.Modified;
    }

    return RequestStatus.Error;
}

private static bool MappingActionIsToExclude(PortfolioMapping mapping)
{
    return mapping.Action == "exclude" && mapping.ActiveCurrent == 0;
}

Worried about the code not working the same? Look into unit testing. I wrote these tests (using nunit) first before I changed 1 line of code and made sure all 9 tests passed. Then I proceeded to refactor until The code looked like it does above.

using NUnit.Framework;
using System.Collections.Generic;

namespace StackExchange.Tests
{
    [TestFixture]
    public class PortfolioRequestStateTests
    {
        [TestCaseSource("TestCaseProvider")]
        public RequestStatus TestMethod1(PortfolioRequest request, PortfolioMapping mapping)
        {
            return new PortfolioRequestState().GetAppropriateRequestStatus(request, mapping);
        }

        private IEnumerable<ITestCaseData> TestCaseProvider()
        {
            //    if (request != null && request.RequestType == 3) 
            yield return
                new TestCaseData(CancelledRequest, DefaultMapping)
                .SetName("When Request type is Cancelled then Status is Cancelled")
                .Returns(RequestStatus.Cancelled);

            //else if (mapping.Action == "exclude" && mapping.ActiveCurrent == 0 && (request == null || request.RequestType != 3))
            yield return 
                new TestCaseData(NullRequest, new PortfolioMapping { Action = "exclude", ActiveCurrent = 0 })
                .SetName("When request is null and Mapping Action is exclude and current is 0 then Status is Excluded")
                .Returns(RequestStatus.Excluded);
            yield return
                new TestCaseData(DefaultRequest, new PortfolioMapping { Action = "exclude", ActiveCurrent = 0 })
                .SetName("When request is not cancel and Mapping Action is exclude and current is 0 then Status is Excluded")
                .Returns(RequestStatus.Excluded);
            yield return
                new TestCaseData(NullRequest, DefaultMapping)
                .SetName("When request is null and Mapping Action is not exclude and current is 0 then status is Error")
                .Returns(RequestStatus.Error);
            yield return
                new TestCaseData(DefaultRequest, new PortfolioMapping { Action = "exclude", ActiveCurrent = 1 })
                .SetName("When request is not cancel and Mapping Action is exclude but current is not 0 then Status is Error")
                .Returns(RequestStatus.Error);
            yield return
                new TestCaseData(DefaultRequest, DefaultMapping)
                .SetName("When request is not cancel and Mapping current is 0 but Action is not exclude then Status is Error")
                .Returns(RequestStatus.Error);
            yield return
                new TestCaseData(NullRequest, new PortfolioMapping { Action = "exclude", ActiveCurrent = 1 })
                .SetName("When Mapping Action is exclude and current is not 0 Status is Excluded")
                .Returns(RequestStatus.Error);

            //else if (request != null && request.RequestType == 2 && request.IsInvalid != true) 
            yield return
                new TestCaseData(ModifiedRequest, DefaultMapping)
                .SetName("When Request is to update and is valid then Status is Modified")
                .Returns(RequestStatus.Modified);

            //else if (request != null && request.IsInvalid == true)
            yield return
                new TestCaseData(InvalidRequest, DefaultMapping)
                .SetName("When Request is to update but is invalid then Status is Invalid")
                .Returns(RequestStatus.Invalid);

        }

        private readonly static PortfolioRequest NullRequest = null;
        private readonly static PortfolioRequest CancelledRequest = new PortfolioRequest { RequestType = 3 };
        private readonly static PortfolioRequest DefaultRequest = new PortfolioRequest();
        private readonly static PortfolioRequest ModifiedRequest = new PortfolioRequest { RequestType = 2 };
        private readonly static PortfolioRequest InvalidRequest = new PortfolioRequest { IsInvalid = true };

        private readonly static PortfolioMapping DefaultMapping = new PortfolioMapping { Action = "", ActiveCurrent = 0 };
    }
}
\$\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.