There's been ongoing debates amongst my team members on whether or not to follow the "Single Entry Point - Single Exit Point" practice when returning values from methods or terminating the execution of a method with a void return type using a 'return' statement. And if the SESE pattern should be used, what its implementation in C# entails and how loosely or strictly this implementation should be followed?
Since all the individual's perspective and opinions are somewhat subjective, I wanted to ask this in an open and unbiased forum, hence this post.
I already looked around a little, but my questions are both a little more involved and they are also specific to C#5 using the .NET 4.5 framework. Visual Studio 2013 as IDE/Compiler and the OS being MS Windows 7/MS Windows Server.
I have also provided a piece of slightly altered code to bring some context to my questions and better illustrate the issues I am referring to.
My questions are:
Since the "single point of entry and single point of exist" model should always be followed, does that mean control blocks should not ever be used in C# / .NET or are there any scenarios where using control blocks is beneficial?
Our current implementation of SESE consists of declaring and initializing a local variable at line 1 of each method that holds the return value. The value of this variable is always initialized to 0 for numeric values, null for most reference types (for certain types we use specific values such as String.Empty for string return values, etc).
In the code sample I've provided the 'Foo_WithProperSeSe' method is the standard implementation of the SESE pattern in my team. Notice the 'result' variable declared and initialized at line 1 and returned at the very last line of the method.
The next two methods (namely Foo_UsingTheTerneryOperatorToInitializeResult & Foo_UsingTheTerneryOperatorToDirectlyReturnValue) do the same work as the former, but the second method uses a conditional operator to initialize the result variable instead of if/else statements. And the 3rd method does the same thing, only without storing it to a local variable. The result of the ternary operator in this case is directly returned from the method.
Technically all 3 methods have the same number of entry/exit points, so my question is: do the latter 2 methods also still count as following the the SESE practice?
It's clearly useful to initialize the result variable value to prevent compiler errors from happening. Is there a better way of doing this?
Foo_MultiReturn() does not follow SESE. We do gain a little readability at the expense of loosing the SESE model. In more complex/larger methods, can this approach be better than following SESE?
I understand the concept of SESE was introduced in the late 1950's to discourage the use of GOTO statements in structured languages.
Is using 'return' statements in C# methods as dangerous as using a GOTO statements in the 1950's? I know there's been quite a lot of changes in IT since the 50's, but the concepts are the same, and this practice should remain unaffected, right?
One of the arguments favoring the strict implementation of SESE is providing the ability to place breakpoint on the last line of any given method and inspect its result value before it returns it.
The counter argument from those opposing SESE is that if each method has a well-defined and specific purpose, its expected behavior can be defined as a series of "automated tests" as they explained. I am not entirely clear on the details, but it sounded like they were convinced these "tests" would decrease the need to use the breakpoints and debug the code. Is this true? Can it ever be possible to reduce (let alone eliminate) the dependency on breakpoints while debugging code?
The counter-argument against the "automated / unit testing approach" was that due to our aggressive deadlines and strict time constraints, we can't afford to be wasting time writing unit tests.
I was wondering if there are any types of projects/circumstances in which using these "automated tests" would be justified. Is there a general rule of thumbs? Or is it ever worth creating these tests at all?
And of so, how much extra time is spent on a project when unit tests are created? Is the ROI ever worth it?
In some of our code we have methods exceeding several hundreds of lines in length. This is after we refactored them and rearranged the conditions using a single/long switch statement instead of the long series of nested "if/else/if" statements.
Despite the use of the switch statement, when following SESE and nesting the 600-line switch statement 4 or 5 levels deep, keeping track of which level we are working at can get a little tricky on occasion and sometimes results in some confusion and/or small errors.
So in an attempt to improve the code while retaining the SESE model, we started thinking of various approaches. Some of the suggested solutions that were:
- for each 'case' statement within the current switch statement, create a corresponding class that will perform the same work that is currently being performed inside the case statement block.
and instead of the switch statement choosing the case statement that is activated, we could create essentially a "type picker": a class that searches for all these worker classes (say by looking for anything that is either explicitly registered, or specified in a config file, or decorated with a specific type of attribute, or registered with the IoC container, or derives from a specific base class, or implements a specific interface, or any combination thereof) and from the set of all such "handler" types, each time the method is invoked, create and return an object from the type that would perform the same work as the current case statement block does.
- create a "class factory"
- identify and separate out various aspects of the work being performed in that method, create separate types with specific responsibility, each addressing the needs of a single aspect, and then inject those aspects into any methods anywhere in the application anytime they're needed so they can provide the routinely needed behaviors to any part of the application without being re-implemented in separate places. That could also help us keep various aspects from spreading into our business objects.(this one just sounded really complex).
The latter was voted out very quickly due to its complexity in nature and our time constraints, so we ignored that and only considered the former solutions.
After some discussion the team decided against this refactoring activity. The top 2 reasons were:
i. 'switch' statements are simple and anyone on the team including junior developers can understand, maintain, fix, debug, and add to them. Whereas implementing and maintaining patterns such as a "class factory"
would be far more difficult and less experienced developers would no longer be able to work on those parts of the code.
and
ii. Implementing a solution such as the "class factory" or otherwise breaking down the responsibilities and work that is currently being done in a single simple method, would then become spread over many classes.
"Too many" classes and interfaces would need to be created, which would render the application unmaintainable.
My question is: are there any practical approaches for addressing this situation? Or is keeping switch statement and just adding a few case statements to it each release based on the business requirements the safer and cleaner direction to go?
I realize this is a lot of information to ask for, and I thank you in advance.
Code:
// standard/proper practice following the "single point of entry and single point of exit" to and from this method.
MyReferenceType Foo_WithProperSeSe()
{
MyReferenceType result = null; // initializing the result value fixes the compiler errors that would other manifest as "use of unassigned local variable 'result'"
if (someConditionThatEvaluatesToABoolean)
result = someValue;
else
result = someOtherValue;
// very often we have a few nested levels of code like this and we have a lot of "if/else if" (note the last after the else if)
if (this.Property1==1)
{
result = DoSomethingThatReturnsTheValueWeNeedWhenProperty1IsEqualToOne();
}
else if(this.Property1==2)
{
result = DoSomethingElse();
}
return result;
}
// this method is semantically the equivallent of the previous one, but uses the ternary operator instead of if/else statements to initialize the 'result' variable value.
SomeReturnType Foo_UsingTheTerneryOperatorToInitializeResult()
{
var result = someConditionThatEvaluatesToABoolean ? DoSomethingThatReturnsTheValueWeNeedWhenProperty1IsEqualToOne() : someOtherValue();
return result;
}
// Same as the previous method, but just returning the return value immediately instead of storing it to a local variable first.
SomeReturnType Foo_UsingTheTerneryOperatorToDirectlyReturnValue
{
return someConditionThatEvaluatesToABoolean ? DoSomethingThatReturnsTheValueWeNeedWhenProperty1IsEqualToOne : DoSomethingElse();
}
// another method with slightly more complex logic
string GetCustomerContactAddress(CustomerDetails profile)
{
string result = string.Empty; // !! Note: do not remove this line or change the initial value. And especially do not change it to a null.
//
// in case there is a bug in this method and the value of result is not set later on, the compiler would
// generate an error and not let us proceed unless the result var is initialized.
//
// Also, setting the value to null can cause problems in caller methods in the case of the scenario described above. The difference is,
// the compiler will allow the build to proceed, but at runtime if we have a bug and don't overwrite the null value, the caller methods
// will get NullReferenceException's when they try to invoke methods of the string object or read its value.
//
// To avoid that, do NOT initialize value to null.
if(profile!=null)
{
if(profile.MainUser!=null)
{
if(profile.MainUser.PreferedContactMethod !=null)
{
if(profile.MainUser.PreferedContactMethod.Name == "Email")
{
if(profile.MainUser.PreferedContactMethod.Address != null)
{
if(profile.MainUser.PreferedContactMethod.Address.Trim() != string.Empty)
{
if(this.IsEmailAvailableAtTheMomentOnThisSystem)
result = profile.MainUser.PreferedContactMethod.Address;
}
}
}
else if (profile.MainUser.PreferedContactMethod.Name == "TextMessage")
{
if(profile.MainUser.PreferedContactMethod.Value!=null)
{
string phoneNumberDigits = new string(profile.MainUser.PreferedContactMethod.Value.Where(ch=>ch.IsDigit()).ToArray());
if(phoneNumberDigits.Length==7)
{
phoneNumberDigits = GetDefaultAreaCode() + phoneNumberDigits;
}
else if(phoneNumberDigits.Length==7)
{
// adding logic here to address the bug that killed us last month: when the number begins with the international prefix instead of (not in addition to) the area code.
// the scenario where they are both provided is handled next.
if(phoneNumberDigits.Substring(0,3)=="001")
{
result = GetDefaultAreaCode() + phoneNumberDigits(3);
}
else if (phoneNumberDigits.Length==13)
{
// set result based on the logics here
}
// no need to worry about phoneNumberDigits.Length != 13. if the length doesn't fit, we've already assigned result so this code will still compile.
// I do realize it'd make some sense to separate the concerns, leave the phone number validation to a designated class for that, similarly do the same with the email
// address validation, number correction, etc.
// but I just find the nested layers of "if/else if" more easthetically pleasing and I don't want to hear about propousteroous ideas such as "SOLID, SoC, TDD, etc". Unit tests in particular are a waste of time as we all know.
}
}
}
}
}
}
}
// same as last method, but a little easier to read/maintain. Downside: we have deviated from the SESE model.
string GetContactDetailToUse_NotSese(UserProfile profile)
{
if(profile!=null) return string.Empty;
if (profile.MainUser==null) return string.Empty;
if (profile.MainUser.PreferedContactMethod==null) return string.Empty;
if (profile.MainUser.PreferedContactMethod.Name==null) return string.Empty;
if (string.IsNullOrWhiteSpace(profile.MainUser.PreferedContactMethod.Name)) return string.Empty;
switch(profile.MainUser.PreferedContactMethod.Name)
{
case "Email":
return GetEmailAddress(profile.MainUser.PreferedContactMethod);
case "TextMessage":
return GetPhoneNumber(profile.MainUser.PreferedContactMethod);
default:
// throw new NotSupportedException("Contact method not supported: " + profile.MainUser.PreferedContactMethod.Name);
// commenting out the previous line and just going to return string.Empty instead.
// this will prevent all those exceptions we're getting prod...
return string.Empty;
}
}
Aucun commentaire:
Enregistrer un commentaire