SubMain - CodeIt.Right The First Time!

/Community

Support Community for SubMain Products
 Home Products Services Download Purchase Support
in Search
 
Home Forums Blogs Tutorials/CIR Tutorials/GD Downloads
Welcome to SubMain Community Sign in | Join | Help

SubMain Blog

Browse by Tags

All Tags » RulesExplained   (RSS)

  • CodeIt.Right Rules Explained, Part 5

    Today, I'll do another installment of the CodeIt.Right Rules, Explained series.  This is post number five in the series.  And, as always, I'll start off by citing my two personal rules about static analysis guidance, along with the explanation for them.

    • Never implement a suggested fix without knowing what makes it a fix.
    • Never ignore a suggested fix without understanding what makes it a fix.

    It may seem as though I'm playing rhetorical games here.  After all, I could simply say, "learn the reasoning behind all suggested fixes."  But I want to underscore the decision you face when confronted with static analysis feedback.  In all cases, you must actively choose to ignore the feedback or address it.  And for both options, you need to understand the logic behind the suggestion.

    In that spirit, I'm going to offer up explanations for three more CodeIt.Right rules today.

    Mark ISerializable Types with "Serializable" Attribute

    If you run across this rule, you might do so while writing an exception class.  For example, the following small bit of code in a project of mine triggers it.

    public class GithubQueryingException
    : Exception { public GithubQueryingException(string message,
    Exception ex) : base(message,
    ex) { } }

    It seems pretty innocuous, right?  Well, let's take a look at what went wrong.

    The rule actually describes its own solution pretty well.  Slap a serializable attribute on this exception class and make the tool happy.  But who cares?  Why does it matter if you don't mark the exception as serializable?

    To understand the issue, you need awareness of a concept called "application domains" within the .NET framework.  Going into much detail about this would take us beyond the scope of the post.  But suffice it to say, "application domains provide an isolation boundary for security, reliability, and versioning, and for unloading assemblies."  Think two separate processes running and collaborating.

    If some external process will call your code, it won't access and deal with your objects the same way that your own code will.  Instead, it needs to communicate by serializing the object and passing it along as if over some remote service call.  In the case of the exception above, it lacks the attribute marking it explicitly as serializable, in spite of implementing that interface.  So bad things will happen at runtime.  And this warning exists to give you the heads up.

    If you'll only ever handle this exception within the same app domain, it won't cause you any heartburn.  But, then again, neither will adding an attribute to your class.

    Do Not Handle Non-CLS-Compliant Exceptions

    Have you ever written code that looks something like this?

    try {
    DoSomething(); return true;
    } catch { return false;
    }

    In essence, you want to take a stab at doing something and return true if it goes well and false if anything goes wrong.  So you write code that looks something like the above.

    If you you have, you'll run afoul of the CodeIt.Right rule, "do not handle non-cls-compliant exceptions."  You might find this confusing at first blush, particularly if you code exclusively in C# or Visual Basic.  This would confuse you because you cannot throw exceptions not compliant with the common language specification (CLS).  All exceptions you throw inherit from the Exception class and thus conform.

    However, in the case of native code written in, say, C++, you can actually throw non-CLS-compliant exceptions.  And this code will catch them because you've said "catch anything that comes my way."  This earns you a warning.

    The CodeIt.Right warning here resembles one telling you not to catch the general exception type.  You want to be intentional about what exceptions you trap, rather than casting an overly wide net.  You can fix this easily enough by specifying the actual exception you anticipate might occur.

    Async Methods Should Return Task or Task<T>

    As of .NET Framework 4.5, you can use the async keyword to allow invocation of an asynchronous operation.  For example, imagine that you had a desktop GUI app and you wanted to populate a form with data.  But imagine that acquiring said data involved doing an expensive and time consuming call over a network.

    With synchronous programming, the call out to the network would block, meaning that everything else would grind to a halt to wait on the network call... including the GUI's responsiveness.  That makes for a terrible user experience.  Of course, we solved this problem long before the existence of the async keyword.  But we used laborious threading solutions to do that, whereas the async keyword makes this more intuitive.

    Roughly speaking, designating a method as "async" indicates that you can dispatch it to conduct its business while you move on to do other things.  To accomplish this, the method synchronously returns something called a Task, which acts as a placeholder and a promise of sorts.  The calling method keeps a reference to the Task and can use it to get at the result of the method, once the asynchronous operation completes.

    But that only works if you return a Task or Task<T>.  If, instead, you create a void method and label it asynchronous, you have no means to get at it later and no means to explicitly wait on it.  There's a good chance this isn't what you want to do, and CodeIt.Right lets you know that.  In the case of an event handler, you might actually want to do this, but better safe than sorry.  You can fix the violation by returning a non-parameterized Task rather than declaring the method void.

    Until Next Time

    This post covered some interesting language and framework features.  We looked at the effect of crossing app domain boundaries and what that does to the objects whose structure you can easily take for granted.  Then we went off the beaten path a little by looking at something unexpected that can happen at the intersection of managed and native code.  And, finally, we delved into asynchronous programming a bit.

    As we wander through some of these relatively far-reaching concerns, it's nice to see that CodeIt.Right helps us keep track.  A good analysis tool not only helps you catch mistakes, but it also helps you expand your understanding of the language and framework.

    Learn more how CodeIt.Right can help you automate code reviews and improve your code quality.

    About the Author

    Erik Dietrich

    I'm a passionate software developer and active blogger. Read about me at my site. View all posts by Erik Dietrich

  • CodeIt.Right Rules Explained, Part 4

    Today, I'll do another installment of the CodeIt.Right Rules, Explained series.  I have now made four such posts in this series.  And, as always, I'll start off by citing my two personal rules about static analysis guidance.

    • Never implement a suggested fix without knowing what makes it a fix.
    • Never ignore a suggested fix without understanding what makes it a fix.

    It may seem as though I'm playing rhetorical games here.  After all, I could simply say, "learn the reasoning behind all suggested fixes."  But I want to underscore the decision you face when confronted with static analysis feedback.  In all cases, you must actively choose to ignore the feedback or address it.  And for both options, you need to understand the logic behind the suggestion.

    In that spirit, I'm going to offer up explanations for three more CodeIt.Right rules today.

    Type that contains only static members should be sealed

    Let's start here with a quick example.  I think this picture will suffice for some number of words, if not necessarily one thousand.

    blog-codeitright-rules-part4-1

    Here, I've laid a tiny seed for a Swiss Army Knife, "utils" class.  Presumably, I will continue to dump any method I think might help me with Linq into this class.  But for now, it contains only a single method to make things easy to understand.  (As an aside, I discourage "utils" classes as a practice.  I'm using this example because everyone reading has most assuredly seen one of these things at some point.)

    When you run CodeIt.Right analysis on this code, you will find yourself confronted with a design issue.  Specifically, "types that contain only static members should be sealed."

    You probably won't have a hard time discerning how to remedy the situation.  Adding the "sealed" modifier to the class will do the trick.  But why does CodeIt.Right object?

    The Microsoft guidelines contain a bit more information.  They briefly explain that static analyzers make an inference about your design intent, and that you can better communicate that intent by using the "sealed" keyword.  But let's unpack that a bit.

    When you write a class that has nothing but static members, such as a static utils class, you create something with no instantiation logic and no state.  In other words, you could instantiate "a LinqUtils," but you couldn't do anything with it.  Presumably, you do not intend that people use the class in that way.

    But what about other ways of interacting with the class, such as via inheritance?  Again, you could create a LinqUtilsChild that inherited from LinqUtils, but to what end?  Polymorphism requires instance members, and non exist here.  The inheriting class would inherit absolutely nothing from its parent, making the inheritance awkward at best.

    Thus the intent of the rule.  You can think of it telling you the following.  "You're obviously not planning to let people use inheritance with you, so don't even leave that door open for them to possibly make a mistake."

    So when you find yourself confronted with this warning, you have a simple bit of consideration.  Do you intend to have instance behavior?  If so, add that behavior and the warning goes away.  If not, simply mark the class sealed.

    Async methods should have async suffix

    Next up, let's consider a rule in the naming category.  Specifically, when you name an async method with suffixing "async" on its name, you see the warning.  Microsoft declares this succinctly in their guidelines.

    By convention, you append "Async" to the names of methods that have an async modifier.

    So, CodeIt.Right simply tells us that we've run afoul of this convention.  But, again, let's dive into the reasoning behind this rule.

    When Microsoft introduced this programming paradigm, they did so in a non-breaking release.  This caused something of a conundrum for them because of a perfectly understandable language rule stating that method overloads cannot vary only by a return type.  To take advantage of the new language feature, users would need to offer the new, async methods, and also backward compatibility with existing method calls.  This put them in the position of needing to give the new, async methods different names.  And so Microsoft offered guidance on a convention for doing so.

    I'd like to make a call-out here with regard to my two rules at the top of each post.  This convention came about because of expediency and now sticks around for convention's sake.  But it may bother you that you're asked to bake a keyword into the name of a method.  This might trouble you in the same way that a method called "GetCustomerNumberString()" might bother you.  In other words, while I don't advise you go against convention, I will say that not all warnings are created equally.

    Always define a global error handler

    With this particular advice, we dive into warnings specific to ASP.  When you see this warning, it concerns the Global.asax file.  To understand a bit more about that, you can read this Stack Overflow question.  In short, Global.asax allows you to define responses to "system level" in a single place.

    CodeIt.Right is telling you to define just such an event -- specifically one in response to the "Application_Error" event.  This event occurs whenever an exception bubbles all the way up without being trapped anywhere by your code somewhere.  And, that's a perfectly reasonable state of affairs -- your code won't trap every possible exception.

    CodeIt.Right wants you to define a default behavior on application errors.  This could mean something as simple as redirecting to a page that says, "oops, sorry about that."  Or, it could entail all sorts of robust, diagnostic information.  The important thing is that you define it and that it be consistent.  You certainly don't want to learn from your users what your own application does in response to an error.

    So spent a bit of time defining your global error handling behavior.  By all means, trap and handle exceptions as close to the source as you can.  But always make sure to have a backup plan.

    Until Next Time

    In this post, I ran the gamut across concerns.  I touched on an object-oriented design concern.  Then, I went into a naming consideration involving async, and, finally, I talked specifically about ASP programming considerations.

    I don't have a particular algorithm for the order in which I cover these subjects.  But, I like the way this shook out.  It goes to show you that CodeIt.Right covers a lot of ground, across a lot of different landscapes of the .NET world.

    Learn more how CodeIt.Right can help you automate code reviews and improve your code quality.

    About the Author

    Erik Dietrich

    I'm a passionate software developer and active blogger. Read about me at my site. View all posts by Erik Dietrich

  • CodeIt.Right Rules Explained, Part 3

    In what has become a series of posts, I have been explaining some CodeIt.Right rules in depth.  As with the last post in the series, I'll start off by citing two rules that I, personally, follow when it comes to static code analysis.

    • Never implement a suggested fix without knowing what makes it a fix.
    • Never ignore a suggested fix without understanding what makes it a fix.

    It may seem as though I'm playing rhetorical games here.  After all, I could simply say, "learn the reasoning behind all suggested fixes."  But I want to underscore the decision you face when confronted with static analysis feedback.  In all cases, you must actively choose to ignore the feedback or address it.  And for both options, you need to understand the logic behind the suggestion.

    In that spirit, I'm going to offer up explanations for three more CodeIt.Right rules today.

    Use Constants Where Appropriate

    First up, let's consider the admonition to "use constants where appropriate."  Consider this code that I lifted from a Github project I worked on once.

    blog-codeitright-rules-part3-1

    I received this warning on the first two lines of code for this class.  Specifically, CodeIt.Right objects to my usage of static readonly string. If I let CodeIt.Right fix the issue for me, I wind up with the following code.

    blog-codeitright-rules-part3-2

    Now, CodeIt.Right seems happy.  So, what gives?  Why does this matter?

    I'll offer you the release notes of the version where CodeIt.Right introduced this rule.  If you look at the parenthetical next to the rule, you will see "performance."  This preference has something to do with code performance.  So, let's get specific.

    When you declare a variable using const or static readonly, think in terms of magic values and their elimination.  For instance, imagine my UserAgentKey value.  Why do you think I declare that the way I did?  I did it to name that string, rather than using it inline as a "magic" string. 

    As a maintenance programmer, how frustrating do you find stumbling across lines of code like, "if(x == 299)"?  "What is 299, and why do we care?!"

    So you introduce a variable (or, preferably, a constant) to document your intent.  In the made-up hypothetical, you might then have "if(x == MaximumCountBeforeRetry)".  Now you can easily understand what the value means.

    Either way of declaring this (constant or static, readonly field) serves the replacement purpose.  In both cases, I replace a magic value with a more readable, named one.  But in the case of static readonly, I replace it with a variable, and in the case of const, I replace it with, well, a const.

    From a performance perspective, this matters.  You can think of a declaration of const as simply hard-coding a value, but without the magic.  So, when I switch to const, in my declaration, the compiler replaces every version of UserAgentKey with the string literal "user-agent".  After compilation, you can't tell whether I used a const or just hard-coded it everywhere.

    But with a static readonly declaration, it remains a variable, even when you use it like a constant.  It thus incurs the relative overhead penalty of performing a variable lookup at runtime.  For this reason, CodeIt.Right steers you toward considering making this a constant.

    Parameter Names Should Match Base Declaration

    For the next rule, let's return to the Github scraper project from the last example.  I'll show you two snippets of code.  The first comes from an interface definition and the second from a class implementing that interface.  Pay specific attention to the method, GetRepoSearchResults.

    blog-codeitright-rules-part3-3

    blog-codeitright-rules-part3-4

    If you take a look at the parameter names, it probably won't surprise you to see that they do not match.  Therein lies the problem that CodeIt.Right has with my code.  It wants the implementing class to match the interface definition (i.e. the "base").  But why?

    In this case, we have a fairly simple answer.  Having different names for the conceptually same method creates confusion. 

    Specifically, maintainers will struggle to understand whether you meant to override or overload the method.  In our mind's eyes, identical method signatures signals polymorphic approaches, while same name, different parameters signals overload.  In a sense, changing the name of a variable fakes maintenance programmers out.

    Do Not Declare Externally Visible Instance Fields

    I don't believe we need a screenshot for this one.  Consider the following trivial code snippet.

    public class SomeClass
    { public string _someVariable;
    }

    This warning says, "don't do that."  More specifically, don't declare an instance field with external (to the type) visibility.  The question is, "why not?"

    If you check out the Microsoft guidance on the subject, they explain that, the "use of a field should be as an implementation detail."  In other words, they contend that you violate encapsulation by exposing fields.  Instead, they say, you should expose this via a property (which simply offers syntactic sugar over a method).

    Instead of continuing with abstract concepts, I'll offer a concrete example.  Imagine that you want to model a family and you declare an integer field called _numberOfChildren. That works fine initially, but eventually you encounter the conceptually weird edge case where someone tries to define a family with -1 children.  With an integer field, you can technically do this, but you want to prevent that from happening.

    With clients of your class directly accessing and setting this field, you wind up having to go install this guard logic literally everywhere your clients interact with the field.  But had you hidden the field behind a property, you could simply add logic to the property setter wherein you throw an exception on an attempt to set a negative value.

    This rule attempts to help you future-proof your code and follow good OO practice.

    Until Next Time

    Somewhat by coincidence, this post focused heavily on the C# flavor of object-oriented programming.  We looked at constants versus field access, but then focused on polymorphism and encapsulation.

    I mention this because I find it interesting to see where static analyzers take you.  Follow along for the rest of the series and, hopefully, you'll learn various useful nuggets about the language you use.

    Learn more how CodeIt.Right can help you automate code reviews and improve your code quality.

    About the Author

    Erik Dietrich

    I'm a passionate software developer and active blogger. Read about me at my site. View all posts by Erik Dietrich

    

This Blog

Syndication

 
     
 
Home |  Products |  Services |  Download |  Purchase |  Support |  Community |  About Us |