1

I have added this method to a C# class:

public bool CanAddLeave(Leave newLeave, out AddLeaveResult result)
{
    result = _repository.CreateSqlQuery("CanSendLeaveRequest ")
            .SetParameter("employeePositionId", newLeave.EmployeePosition.EmployeePositionId)
            .UniqueResult<AddLeaveResult> ();

    return (result == AddLeaveResult.Success);
}

The method returns a bool which the client will use to determine quickly if a leave (holiday/time off etc) request can be sent to the employee's manager by email.

The AddLeaveResult output parameter is an enum with following values Success =0, NoManagersWithEmailAddress =1, NoManagersToDeliverTo = 2

When writing this I had the TryParse pattern in mind, but this code smells. My gut is telling me there's a better way, but I can't think of one. Can you suggest a better, cleaner pattern to use?

9

Return the AddLeaveResult directly and drop the out parameter.

public AddLeaveResult CanAddLeave(Leave newLeave)
{
    AddLeaveResult result = _repository.CreateSqlQuery("CanSendLeaveRequest")
            .SetParameter("employeePositionId", newLeave.EmployeePosition.EmployeePositionId)
            .UniqueResult<AddLeaveResult>();
    return result;
}

This way all the available information is sent to the caller. The previously returned boolean value can be inferred by checking result == AddLeaveResult.Success.

TryParse methods are different, they can't return meaningful value in case of failure, so they need to return bool (success or failure) and to fill the out parameter in case of success.

  • Right, the easiest solution is what I'll go for then. I think this is what's known as analysis paralysis... when I start overthinking. – Scott Jan 18 '17 at 12:00
  • If you wanted to use the "out" convention, name your method "TryAddLeave". Most of M$ "try" methods with an out parameter are set up that way. But @Rumen's answer is cleaner and better for your case. – Jon Raynor Jan 18 '17 at 15:31
3

Return the result:

public AddLeaveResult CanAddLeave(Leave newLeave) { }

What is the real reason why a boolean needs to be returned to the GUI? Another result == AddLeaveResult.Success check in the GUI is enough to determine if an email can be sent. It sounds like the boolean return type is not chosen for a good reason other than that it sounded about right when the person who implemented it thought about it for a second.

Also, I think you are using the word 'pattern' too often as if it comes back in every corner in your code. Not everything is a pattern.

Cleanness and readability are two subjective subjects but general consensus is that:

  • The intent of a function should become clear by reading its signature (return type, name and parameter(s)). In your example I would have to read the functions body to know when / what the boolean represents and when / what the AddLeaveResult represents.
  • A function (or method) should do ONE thing (and thus not have side-effects)
  • You should minimize the amount of parameters a method uses
  • Only use ref and out parameters if you cannot solve it differently without wrecking the rest of your code
  • etcetera
  • The code is a work in progress - by me - and it's not committed to the mainline yet. I was tinkering with this idea and trying to think of whatever advantages it gave me. Only advantage I can think of is no need to check the enum for success, when you get a bool true back. I keep thinking of awful nested code from C days (e.g. HRESULT hr = GetCode(); if (SUCCEEDED(hr)) { } ) etc etc. – Scott Jan 18 '17 at 11:59
  • I decided to apply the KISS principle and went for the simplest. Sometimes in code its hard to see the wood for the trees. Thanks for your advice. – Scott Jan 18 '17 at 12:09

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.