Rules to Better Code - 74 Rules

Refactoring is all about making code easier to understand and cheaper to modify without changing its behavior.

As a rule of thumb, no method should be greater than 50 lines of code. Long-winded methods are the bane of any developer and should be avoided at all costs. Instead, a method of 50 lines or more should be broken down into smaller functions.

You should generally be looking for ways to simplify your code (e.g. removing heavily-nested case statements). As a minimum, look for the most complicated method you have and check whether it needs simplifying.

In Visual Studio, there is built-in support for Cyclomatic Complexity analysis.

1. Go to Analyze | Calculate Code Metrics | For Solution

2. Look at the function with the largest Cyclomatic Complexity number and consider refactoring to make it simpler.

Too often, developers are writing a line of code, and they forget that little bit of syntax they need to do what they want. In that case, they usually end up googling it to find the documentation and then copy and paste it into their code. That process is a pain because it wastes valuable dev time.

Not to worry, AI pair programming is here to save the day!

Video: See awesome examples of GitHub Copilot in action with Jesse Skinner

New tools like GitHub Copilot provide devs with potentially complete solutions as they type. It might sound like it's too good to be true, but in reality you can do so much with these tools.

"It’s hard to believe that GitHub Copilot is actually an AI and not a Mechanical Turk. The quality of the code is at the very least comparable to my own (and in fairness that's me bragging), and it's staggering to see how accurate it is in determining your needs, even in the most obscure scenarios."
- Matt Goldman

What can it do?

There is a lot to love with AI pair programming ❤️, here is just a taste of what it can do:

• Populate a form
• Do complex maths
• Create DTOs
• Hydrate data
• Query APIs
• Do unit tests
  and more

Why is it awesome?

AI pair programming has so much to offer, here are the 3 key benefits:

1. Accessibility - Quick suggestions in heaps of languages
2. C#
3. JavaScript
4. SQL
   and many more
5. Efficiency - Less time doing gruntwork like repetitive tasks and making boilerplate
6. Confidence - Higher confidence and less wasted time when working in unfamiliar languages or areas

Code duplication is a big "code smell" that harms maintainability. You should keep an eye out for repeated code and make sure you refactor it into a single place.

For example, have a look at these two Action methods in an MVC 4 controller.

//
// GET: /Person/
[Authorize]
public ActionResult Index()
{
    // get company this user can view
    Company company = null;
    var currentUser = Session["CurrentUser"] as User;
    if (currentUser != null)
    {
        company = currentUser.Company;
    }

    // show people in that company
    if (company != null)
    {
        var people = db.People.Where(p => p.Company == company);
        return View(people);
    }
    else
    {
        return View(new List());
    }
}

//
// GET: /Person/Details/5
[Authorize]
public ActionResult Details(int id = 0)
{
    // get company this user can view
    Company company = null;
    var currentUser = Session["CurrentUser"] as User;
    if (currentUser != null)
    {
        company = currentUser.Company;
    }

    // get matching person
    Person person = db.People.Find(id);
    if (person == null || person.Company == company)
    {
        return HttpNotFound();
    }
    return View(person);
}

We can refactor this code to make sure the repeated lines are only in one place.

private Company GetCurrentUserCompany()
{
    // get company this user can view
    Company company = null;
    var currentUser = Session["CurrentUser"] as User;
    if (currentUser != null)
    {
        company = currentUser.Company;
    }
    return company;
}

//
// GET: /Person/
[Authorize]
public ActionResult Index()
{
    // get company this user can view
    Company company = GetCurrentUserCompany();

    // show people in that company
    if (company != null)
    {
        var people = db.People.Where(p => p.Company == company);
        return View(people);
    }
    else
    {
        return View(new List());
    }
}


// GET: /Person/Details/5
[Authorize]
public ActionResult Details(int id = 0)
{
    // get company this user can view
    Company company = GetCurrentUserCompany();

    // get matching person
    Person person = db.People.Find(id);
    if (person == null || person.Company == company)
    {
        return HttpNotFound();
    }
    return View(person);
}

Tip: The Refactor menu in Visual Studio 11 can do this refactoring for you.

One of the major issues people had back in the day with ASP (before ASP.NET) was the prevalence of "Spaghetti Code". This mixed Reponse.Write() with actual code.

Ideally, you should keep design and code separate - otherwise, it will be difficult to maintain your application. Try to move all data access and business logic code into separate modules.

Bob Martin explains this best:

It's the most obvious - but naming conventions are so crucial to simpler code, it's crazy that people are so loose with them...

For Javascript / Typescript 

Google publishes a JavaScript style guide. For more guides, please refer to this link: Google JavaScript Style Guide

Here are some key points:

• Use const or let – Not var
• Use semicolons
• Use arrow functions
• Use template strings
• Use uppercase constants
• Use single quotes

See 13 Noteworthy Points from Google’s JavaScript Style Guide

For C# Java

See chapter 2: Meaningful Names Clean Code: A Handbook of Agile Software Craftsmanship

For SQL (see Rules to Better SQL Databases)

• SQL Server Object Naming Standard
• SQL Stored Procedure Naming Standard - Avoid System Prefixes
• SQL Server Indexes Naming Standard
• SQL Server Relationship Naming Standard

When moving through the different stages of testing i.e. from internal testing, through to UAT, you should suffix the application name with the appropriate stage:

Stage	Testing Description	Naming Convention
Alpha	Developer testing with project team	Northwind_v2-3_alpha.exe
Beta	Internal “Test Please" testing with non-project working colleagues	Northwind_v2-3_beta.exe
Production e.g.	When moving onto production, this naming convention is dropped	Northwind_v2-3.exe

It is not a good idea to have spaces in a folder or file name as they don't translate to URLs very well and can even cause technical problems.

Instead of using spaces, we recommend:

• kebab-case - using dashes between words

Other not recommended options include:

• CamelCase - using the first letter of each word in uppercase and the rest of the word in lowercase
• snake_case - using underscores between words

For further information, read Do you know how to name documents?

This rule should apply to any file or folder that is on the web. This includes Azure DevOps Team Project names and SharePoint Pages.

Try to avoid problems in if-statements without curly brackets and just one statement which is written one line below the if-statement. Use just one line for such if-statements. If you want to add more statements later on and you could forget to add the curly brackets which may cause problems later on.

if (ProductName == null) ProductName = string.Empty; if (ProductVersion == null)
 ProductVersion = string.Empty; if (StackTrace == null) StackTrace = string.Empty;

if (ProductName == null) 
{ 
 ProductName = string.Empty; 
} 
if (ProductVersion == null)
{ 
 ProductVersion = string.Empty; 
} 
if (StackTrace == null) 
{ 
 StackTrace = string.Empty;
}

Try to avoid Double-Negative Conditionals in if-statements. Double negative conditionals are difficult to read because developers have to evaluate which is the positive state of two negatives. So always try to make a single positive when you write if-statement.

if (!IsValid)
{
        // handle error
}
else
{
       // handle success
}

if (IsValid)
{
       // handle success
}
else
{
       // handle error
}

if (!IsValid)
{
       // handle error
}

Use pattern matching for boolean evaluations to make your code even more readable!

if (IsValid is false)
{
       // handle error
}

Do you know String should be @-quoted instead of using escape character for "\"?The @ symbol specifies that escape characters and line breaks should be ignored when the string is created.

As per: Strings

string p2 = "\\My Documents\\My Files\\";

string p2 = @"\My Documents\My Files\";

Raw String Literals

In C#11 and later, we also have the option to use raw string literals. These are great for embedding blocks of code from another language into C# (e.g. SQL, HTML, XML, etc.). They are also useful for embedding strings that contain a lot of escape characters (e.g. regular expressions).

Another advantage of Raw String Literals is that the redundant whitespace is trimmed from the start and end of each line, so you can indent the string to match the surrounding code without affecting the string itself.

var bad = "<html>" +
           "<body>" +
           "<p class=\"para\">Hello, World!</p>" +
           "</body>" +
           "</html>";

var good = """
           <html>
           <body>
           <p class="para">Hello, World!</p>
           </body>
           </html>
           """;

For more information on Raw String literals see learn.microsoft.com/en-us/dotnet/csharp/language-reference/tokens/raw-string

You should always add the application name to the connection string so that SQL Server will know which application is connecting, and which database is used by that application. This will also allow SQL Profiler to trace individual applications which helps you monitor performance or resolve conflicts.

<add key="Connection" value="Integrated Security=SSPI;Persist Security Info=False;Initial Catalog=Biotrack01;Data Source=sheep;"/>

<add key="Connection" value="Integrated Security=SSPI;Persist Security 
 Info=False;Initial Catalog=Biotrack01;Data Source=sheep; 
 Application Name=Biotracker"/> <!-- Good Code - Application Name is added in the connection string. -->

There are 2 type of connection strings. The first contains only address type information without authorization secrets. These can use all of the simpler methods of storing configuration as none of this data is secret.

Option 1 - Using Azure Managed Identities (Recommended)

When deploying an Azure hosted application we can use Azure Managed Identities to avoid having to include a password or key inside our connection string. This means we really just need to keep the address or url to the service in our application configuration. Because our application has a Managed Identity, this can be treated in the same way as a user's Azure AD identity and specific roles can be assigned to grant the application access to required services.

This is the preferred method wherever possible, because it eliminates the need for any secrets to be stored. The other advantage is that for many services the level of access control available using Managed Identities is much more granular making it much easier to follow the Principle of Least Privilege.

Option 2 - Connection Strings with passwords or keys

If you have to use some sort of secret or key to login to the service being referenced, then some thought needs to be given to how those secrets can be secured.Take a look at Do you store your secrets securely to learn how to keep your secrets secure.

Example - Integrating Azure Key Vault into your ASP.NET Core application

In .NET 5 we can use Azure Key Vault to securely store our connection strings away from prying eyes.

Azure Key Vault is great for keeping your secrets secret because you can control access to the vault via Access Policies. The access policies allows you to add Users and Applications with customized permissions. Make sure you enable the System assigned identity for your App Service, this is required for adding it to Key Vault via Access Policies.

You can integrate Key Vault directly into your ASP.NET Core application configuration. This allows you to access Key Vault secrets via IConfiguration.

public static IHostBuilder CreateHostBuilder(string[] args) =>
 Host.CreateDefaultBuilder(args)
  .ConfigureWebHostDefaults(webBuilder =>
  {
   webBuilder
    .UseStartup<Startup>()
    .ConfigureAppConfiguration((context, config) =>
    {
     // To run the "Production" app locally, modify your launchSettings.json file
     // -> set ASPNETCORE_ENVIRONMENT value as "Production"
     if (context.HostingEnvironment.IsProduction())
     {
      IConfigurationRoot builtConfig = config.Build();

      // ATTENTION:
      //
      // If running the app from your local dev machine (not in Azure AppService),
      // -> use the AzureCliCredential provider.
      // -> This means you have to log in locally via `az login` before running the app on your local machine.
      //
      // If running the app from Azure AppService
      // -> use the DefaultAzureCredential provider
      //
      TokenCredential cred = context.HostingEnvironment.IsAzureAppService() ?
       new DefaultAzureCredential(false) : new AzureCliCredential();

      var keyvaultUri = new Uri($"https://{builtConfig["KeyVaultName"]}.vault.azure.net/");
      var secretClient = new SecretClient(keyvaultUri, cred);
      config.AddAzureKeyVault(secretClient, new KeyVaultSecretManager());
     }
    });
  });

public static class IWebHostEnvironmentExtensions
{
 public static bool IsAzureAppService(this IWebHostEnvironment env)
 {
  var websiteName = Environment.GetEnvironmentVariable("WEBSITE_SITE_NAME");
  return string.IsNullOrEmpty(websiteName) is not true;
 }
}

Setting up your Key Vault correctly

In order to access the secrets in Key Vault, you (as User) or an Application must have been granted permission via a Key Vault Access Policy.

Applications require at least the LIST and GET permissions, otherwise the Key Vault integration will fail to retrieve secrets.

Azure Key Vault and App Services can easily trust each other by making use of System assigned Managed Identities. Azure takes care of all the complicated logic behind the scenes for these two services to communicate with each other - reducing the complexity for application developers.

So, make sure that your Azure App Service has the System assigned identity enabled.

Once enabled, you can create a Key Vault Access policy to give your App Service permission to retrieve secrets from the Key Vault.

Adding secrets into Key Vault is easy.

1. Create a new secret by clicking on the Generate/Import button
2. Provide the name
3. Provide the secret value
4. Click Create

Note: The ApplicationSecrets section is indicated by "ApplicationSecrets--" instead of "ApplicationSecrets:".

As a result of storing secrets in Key Vault, your Azure App Service configuration (app settings) will be nice and clean. You should not see any fields that contain passwords or keys. Only basic configuration values.

Video: Watch SSW's William Liebenberg explain Connection Strings and Key Vault in more detail (8 min)

History of Connection Strings

In .NET 1.1 we used to store our connection string in a configuration file like this:

<configuration>
     <appSettings>
          <add key="ConnectionString" value ="integrated security=true;
           data source=(local);initial catalog=Northwind"/>
     </appSettings>
</configuration>

...and access this connection string in code like this:

SqlConnection sqlConn = 
new SqlConnection(System.Configuration.ConfigurationSettings.
AppSettings["ConnectionString"]);

In .NET 2.0 we used strongly typed settings classes:

Step 1: Setup your settings in your common project. E.g. Northwind.Common

Step 2: Open up the generated App.config under your common project. E.g. Northwind.Common/App.config

Step 3: Copy the content into your entry applications app.config. E.g. Northwind.WindowsUI/App.config The new setting has been updated to app.config automatically in .NET 2.0

<configuration>
      <connectionStrings>
         <add name="Common.Properties.Settings.NorthwindConnectionString"
              connectionString="Data Source=(local);Initial Catalog=Northwind;
              Integrated Security=True"
              providerName="System.Data.SqlClient" />
        </connectionStrings>
 </configuration>

...then you can access the connection string like this in C#:

SqlConnection sqlConn =
 new SqlConnection(Common.Properties.Settings.Default.NorthwindConnectionString);

Most systems will have variables that need to be stored securely; OpenId shared secret keys, connection strings, and API tokens to name a few.

These secrets must not be stored in source control. It is insecure and means they are sitting out in the open, wherever code has been downloaded, for anyone to see.

There are many options for managing secrets in a secure way:

Bad Practices

Good Practices

Resources

The following resources show some concrete examples on how to apply the principles described:

• https://github.com/brydeno/bicepsofsteel
• https://docs.microsoft.com/en-us/azure/key-vault/general/best-practices
• https://docs.microsoft.com/en-us/azure/key-vault/general/security-features
• https://docs.microsoft.com/en-us/aspnet/core/security/app-secrets?view=aspnetcore-5.0&tabs=windows
• https://docs.microsoft.com/en-us/sql/connect/ado-net/connection-strings-and-configuration-files?view=sql-server-ver15
• https://docs.microsoft.com/en-us/azure/azure-app-configuration/howto-integrate-azure-managed-service-identity?tabs=core5x
• https://www.youtube.com/watch?v=F9H0txgz0ns

Most systems will have variables that need to be stored securely; OpenId shared secret keys, connection strings, and API tokens to name a few.

These secrets must not be stored in source control. It is not secure and means they are sitting out in the open, wherever code has been downloaded, for anyone to see.

Do you store your secrets securely? shows different ways to store your secrets securely. When you use .NET User Secrets, you can store your secrets in a JSON file on your local machine. This is great for development, but how do you share those secrets securely with other developers in your organization?

You may be asking what's a secret for a development environment? A developer secret is any value that would be considered sensitive.

An encryption key or sql connection string to a developer's local machine/container is a good example of something that will not always be sensitive for in a development environment, whereas a GitHub PAT token or Azure Storage SAS token would be considered sensitive as it allows access to company-owned resources outside of the local development machine.

Bad Examples

Good Practices

For development purposes once you are using .NET User Secrets you will still need to share them with other developers on the project.

As a way of giving a heads up to other developers on the project, you can add a step in your _docs\Instructions-Compile.md file (Do you make awesome documentation?) to inform developers to get a copy of the user secrets. You can also add a placeholder to the appsettings.Development.json file to remind developers to add the secrets.

Clear text email addresses in web pages are very dangerous because it gives spam sender a chance to pick up your email address, which produces a lot of spam/traffic to your mail server, this will cost you money and time to fix.

Never put clear text email addresses on web pages.

<!--SSW Code Auditor - Ignore next line(HTML)--> 
<a href="mailto:test@ssw.com.au">Contact Us</a>

<a href="javascript:sendEmail('74657374407373772e636f6d2e6175')" onmouseover="javascript:displayStatus('74657374407373772e636f6d2e6175');return true;" onmouseout="javascript:clearStatus(); return true;">Contact Us</a>

Tip: To decode and encode a string you can use this page. If you use Wordpress, use the Email Encoder Bundle plugin to help you encode email addresses easily.

We have a program called SSW CodeAuditor to check for this rule. We have a program called SSW LinkAuditor to check for this rule.

One of our goals is to make the job of the developer as easy as possible. If you have to write a lot of code for something that you think you should not have to do, you should make a suggestion and add it to the relevant page.

If you have to add a suggestion, make sure that you put the link to that suggestion into the comments of your code.

/// <summary>
/// base class for command implementations
/// This is a work around as standard MVVM commands
/// are not provided by default. 
/// </summary>
public class Command : ICommand
{
 // code
}

/// <summary>
/// base class for command implementations
/// This is a work around as standard MVVM commands
/// are not provided by default. 
/// </summary>
///
/// <remarks>
///  Issue Logged here: https://github.com/SSWConsulting/SSW.Rules/issues/3
///</remarks>
public class Command : ICommand
{
 // code
}

Use casts only if:a. You know 100% that you get that type backb. You want to perform a user-defined conversion

private void AMControlMouseLeftButtonUp(object sender, MouseButtonEventArgs e)
{
 var auc = (AMUserControl)sender; 
   
 var aucSessionId = auc.myUserControl.Tag;
 // snip snip snip
}

private void AMControlMouseLeftButtonUp(object sender, MouseButtonEventArgs e)
{
 var auc = sender as AMUserControl; 
   
 if (auc != null)
 {
 var aucSessionId = auc.myUserControl.Tag;
 // snip snip snip
 } 
   
}

More info here: http://blog.gfader.com/2010/08/avoid-type-casts-use-operator-and-check.html

Empty Visual C# .NET methods consume program resources unnecessarily. Put a comment in code block if its stub for future application.Don’t add empty C# methods to your code. If you are adding one as a placeholder for future development, add a comment with a TODO.

Also, to avoid unnecessary resource consumption, you should keep the entire method commented until it has been implemented.

If the class implements an inherited interface method, ensure the method throws NotImplementedException.

public class Example
 {
       public double salary()
       { 
   
       }
 }

public class Sample
 {
       public double salary()
       {
               return 2500.00;
        }
 }

public interface IDemo
 {
       void DoSomethingUseful();
       void SomethingThatCanBeIgnored();
 }
public class Demo : IDemo
 {
       public void DoSomethingUseful()
       {
              // no audit issues
             Console.WriteLine("Useful");
       }
       // audit issues 
      public void SomethingThatCanBeIgnored()
      { 
      } 
 }

public interface IDemo
 {
       void DoSomethingUseful();
       void SomethingThatCanBeIgnored();
 }
public class Demo : IDemo
 {
       public void DoSomethingUseful()
       {
              // no audit issues
              Console.WriteLine("Useful");
       }
       // No audit issues 
       public void SomethingThatCanBeIgnored() 
       {
              // stub for IDemo interface
       } 
 }

We see a lot of programmers doing this, they have two conditions - true and false - and they do not consider other possibilities - e.g. an empty string. Take a look at this example. We have an If statement that checks what backend database is being used.

In the example the only expected values are "Development" and "Production".

void Load(string environment)
{
  if (environment == "Development")
  {
    // set Dev environment variables
  }
  else
  {
    // set Production environment variables	
  }
}

Consider later that extra environments may be added: e.g. "Staging"

By using the above code, the wrong code will run because the above code assumes two possible situations. To avoid this problem, change the code to be defensive .g. Use an Else If statement (like below).

Now the code will throw an exception if an unexpected value is provided.

void Load(string environment)
{
  if (environment == "Development")
  {
    // set Dev environment variables
  }
  else if (environment == "Production")
  {
    // set Production environment variables	
  }
  else
  {
    throw new InvalidArgumentException(environment); 
  }
}

Be sure you are aware of what is business logic and what isn't. Typically, looping code will be placed in the business layer. This ensures that no redundant code is written and other projects can reference this logic as well.

private void btnOK_Click(object sender, EventArgs e)
{
  rtbParaText.Clear();
  var query =
    from p in dc.GetTable()
    select p.ParaID;
  foreach (var result in query)
  {
    var query2 =
      from t in dc.GetTable()
      where t.ParaID == result
      select t.ParaText;
    rtbParaText.AppendText(query2.First() + "\r\n");
  }
}

private void btnOK_Click(object sender, EventArgs e)
{
  string paraText = Business.GetParaText();
  rtbParaText.Clear();
  rtbParaText.Add(paraText);
}

No "UI" in event names, the event raiser should be unaware of the UI in MVVM and user controlsThe handler of the event should then do something on the UI.

private void RaiseUIUpdateBidButtonsRed() { 
  if (UIUpdateBidButtonsRed != null) {
    UIUpdateBidButtonsRed();
  }
}

private void RaiseSelectedLotUpdated() {
  if (SelectedLotUpdated != null) {
    SelectedLotUpdated();
  }
}

The .NET framework and the C# language provide two methods for conditional handling where multiple distinct values can be selected from. The switch statement is less flexible than the if-else-if tree but is generally considered to be more efficient.

The .NET compiler generates a jump list for switch blocks, resulting in far better performance than if/else for evaluating conditions. The performance gains are negligible when the number of conditions is trivial (i.e. fewer than 5), so if the code is clearer and more maintainable using if/else blocks, then you can use your discretion. But be prepared to refactor to a switch block if the number of conditions exceeds 5.

int DepartmentId = GetDepartmentId()
if(DepartmentId == 1)
{
// do something
}
else if(DepartmentId == 2)
{
// do something #2
}
else if(DepartmentId == 3)
{
// do something #3
}
else if(DepartmentId == 4)
{
// do something #4
}
else if(DepartmentId == 5)
{
// do something #5
}
else 
{
// do something #6
}

int DepartmentId = GetDepartmentId()
switch(DepartmentId)
{
case 1:
// do something
break;
case 2:
// do something # 2
break;
case 3:
// do something # 3
break;
case 4:
// do something # 4
break;
case 5:
// do something # 5
break;
case 6:
// do something # 6
break;
default:
//Do something here
break;
}

In situation where your inputs have a very skewed distribution, if-else-if could outperform switch statement by offering a fast path. Ordering your if statement with the most frequent condition first will give priority to tests upfront, whereas switch statement will test all cases with equal priority.

Further Reading:

• Speed Test: Switch vs If-Else-If
• C# If Versus Switch Performance

Validating an XML document against a schema is expensive, and should not be done where it is not absolutely necessary. Combined with weight the XML document object, validation can cause a significant performance hit:

• Read with XmlValidatingReader: 172203 nodes - 812 ms
• Read with XmlTextReader: 172203 nodes - 320 ms
• Parse using XmlDocument no validation - length 1619608 - 1052 ms
• Parse using XmlDocument with XmlValidatingReader: length 1619608 - 1862 ms

You can disable validation when using the XmlDocument object by passing an XmlTextReader instead of the XmlValidatingTextReader:

XmlDocument report = new XmlDocument();
 XmlTextReader tr = new XmlTextReader(Configuration.LastReportPath);
 report.Load(tr);

To perform validation:

XmlDocument report = new XmlDocument();
 XmlTextReader tr = new XmlTextReader(Configuration.LastReportPath);
 XmlValidatingReader reader = new XmlValidatingReader(tr);
 report.Load(reader);

The XSD should be distributed in the same directory as the XML file and a relative path should be used:

<Report> <Report xmlns="LinkAuditorReport.xsd">
 ... </Report>

By default, the connection timeout is 15 seconds. When it comes to testing if a connection is valid or not, 15 seconds is a long time for the user to wait. You should change the connection timeout inside your connection strings to 5 seconds.

"Integrated Security=SSPI;Initial Catalog=SallyKnoxMedical;Data Source=TUNA"

"Integrated Security=SSPI;Initial Catalog=SallyKnoxMedical;Data Source=TUNA;Connect Timeout=5"

Not explicitly specifying the access type for members of a structure or class can be misleading for other developers. The default member accessibility level for classes and structs in Visual C# .NET is always private. In Visual Basic .NET, the default for classes is private, but for structs is public.

Match MatchExpression(string input, string pattern)

private Match MatchExpression(string input, string pattern)

We have a program called SSW Code Auditor to check for this rule.

The return statement can be very useful when used for validation filtering.

Instead of a deep nested If, use Return to provide a short execution path for conditions which are invalid.

private void AssignRightToLeft()
{
  // Validate Right 
  if (paraRight.SelectedIndex >= 0)
  { 
    // Validate Left 
    if (paraLeft.SelectedIndex >= 0)
    {
       string paraId = paraRight.SelectedValue.ToString();
       Paragraph para = new Paragraph();
       para.MoveRight(paraId);
       RefreshData();
    }
  }
}

private void AssignRightToLeft()
{
  // Validate Right 
  if (paraRight.SelectedIndex < 0)
  {
    return; 
  }
  
  // Validate Left 
  if (paraLeft.SelectedIndex < 0)
  {
    return;
  }

  string paraId = paraRight.SelectedValue.ToString();
  Paragraph para = new Paragraph();
  para.MoveRight(paraId);
  RefreshData();
}

You should expose events as events.

public Action
< connectioninformation > ConnectionProblem;

public event Action
< connectioninformation > ConnectionProblem;

This rule is inspired by a piece from Robert C. Martin (Uncle Bob) where he identifies an age old boys scouts rule could be used by software developers to constantly improve a codebase.

Uncle Bob proposed the original rule...

Always leave the campground cleaner than you found it.

...be changed to

Always check a module in cleaner than when you checked it out.

The reasoning being that no matter how good of a software developer we are, over time, smells creep into code. Be it from tight deadlines, old code that has been changed or appended to in insolation 100's of times over years or just or just newer & better ways of doing things become available.

So each time you touch some code, leave it just a little cleaner than the way you found it.

Here are some simple examples of how you can leave your campsite code cleaner:

1. Remove a compiler warning
2. Remove unused code
3. Improve variable/method naming to make it clearer
4. DRY out some code
5. Restructure a code block to make it more readable
6. Add a test for a missing use case

Boolean Properties must be prefixed by a verb. Verbs like "Supports", "Allow", "Accept", "Use" should be valid. Also properties like "Visible", "Available" should be accepted (maybe not). Here is how we name Boolean columns in SQL databases.

public bool Enable { get; set; }
public bool Invoice { get; set; }

public bool Enabled { get; set; }
public bool IsInvoiceSent { get; set; }

We have a program called SSW Code Auditor to check for this rule.

You should format "Environment.NewLine" at the end of a line.

string message = "The database is not valid." + Environment.NewLine + "Do you want to upgrade it? ";

string message = "The database is not valid." + Environment.NewLine;
message += "Do you want to upgrade it? ";

return string.Join(Environment.NewLine, paragraphs);

This feature is Particularly important if the user runs a semi-long task (e.g.30 seconds) once a day. Only at the end of the long process can they know the particular amount of time, if the time taken dialog is shown after the finish. If the status bar contains the time taken and the progress bar contains the progress percentage, they can evaluate how long it will take according to the time taken and percentage. Then they can switch to other work or go get a cup of coffee.

Also a developer, you can use it to know if a piece of code you have modified has increased the performance of the task or hindered it.

You should import namespaces and shorten the references.

System.Text.StringBuilder myStringBuilder = new System.Text.StringBuilder();

using System.Text;
...
...
StringBuilder myStringBuilder = new StringBuilder();

If you have ReSharper installed, you can let ReSharper take care of this for you:

You should initialize variables outside of the try block.

Cursor cur;

try {
  // ...
  cur = Cursor.Current; //Bad Code - initializing the variable inside the try block
  Cursor.Current = Cursors.WaitCursor;
  // ...
} finally {
  Cursor.Current = cur;
}

Cursor cur = Cursor.Current; //Good Code - initializing the variable outside the try block

try {
  // ...
  Cursor.Current = Cursors.WaitCursor;
  // ...
} finally { 
  Cursor.Current = cur;
}

This is against the .NET Object Naming Conventions and inconsistent with the framework.

Public Enum ProjectLanguageEnum CSharp VisualBasic End Enum

Public Enum ProjectLanguage CSharp VisualBasic End Enum

We have a program called SSW Code Auditor to check for this rule.

If you have to use a workaround you should always comment your code.

In your code add comments with:

1. The pain - In the code add a URL to the existing resource you are following e.g. a blog post
2. The potential solution - Search for a suggestion on the product website. If there isn't one, create a suggestion to the product team that points to the resource. e.g. on https://uservoice.com/ or https://bettersoftwaresuggestions.com/

Figure: Always add a URL to the suggestion that you are compensating for

Exercise: Understand commenting

You have just added a grid that auto updates, but you need to disable all the timers when you click the edit button. You have found an article on Code Project (http://www.codeproject.com/Articles/39194/Disable-a-timer-at-every-level-of-your-ASP-NET-con.aspx) and you have added the work around.

Now what do you do?

protected override void OnPreLoad(EventArgs e)
{
     //Fix for pages that allow edit in grids
     this.Controls.ForEach(c =>
     {   
          if (c is System.Web.UI.Timer)
          {
              c.Enabled = false;
          }
     });
     base.OnPreLoad(e);
}

Figure: Work around code in the Page Render looks good. The code is done, something is missing

Named parameters have always been there for VB developers and in C# 4.0, C# developers finally get this feature.

You should use named parameters under these scenarios:

• When there are 4 or more parameters
• When you make use of optional parameters
• If it makes more sense to order the parameters a certain way

Although many have differing opinions on this matter, Windows applications have standard storage locations for their files, whether they're settings or user data. Some will disagree with those standards, but it's safe to say that following it regardless will give users a more consistent and straightforward computing experience.

The following grid shows where application files should be placed:

Further Information

• The System.Environment class provides the most general way of retrieving those paths
• The Application class lives in the System.Windows.Form namespace, which indicates it should only be used for WinForm applications. Other types of applications such as Console and WebForm applications use their corresponding utility classes

Microsoft's write-up on this subject can be found at Microsoft API and reference catalog.

Events should end in "ing" or "ed".

public event Action
< connectioninformation > ConnectionProblem;

public event Action
< connectioninformation > ConnectionProblemDetected;

TimeSpan.Parse() constructs a Timespan from a time indicated by a specified string. The acceptable parameters for this function are in the format "d.hh:mm" where "d" is the number of days (it is optional), "hh" is hours and is between 0 and 23 and "mm" is minutes and is between 0 and 59. If you try to pass, as a parameter, as a string such as "45:30" (meaning 45 hours and 30 minutes), TimeSpan.Parse() function will crash. (The exact exception received is: "System.OverflowException: TimeSpan overflowed because duration is too long".) Therefore it is recommended that you should always pre-parse the time string before passing it to the "TimeSpan.Parse()" function.

This pre-parsing is done by the FormatTimeSpanString( ) function. This function will format the input string correctly. Therefore, a time string of value "45:30" will be converted to "1.21:30" (meaning 1 day, 21 hours and 30 minutes). This format is perfectly acceptable for TimeSpan.Parse() function and it will not crash.

ts = TimeSpan.Parse(cboMyComboBox.Text)

ts = TimeSpan.Parse(FormatTimeSpanString(cboMyComboBox.Text))

We have a program called SSW Code Auditor to check for this rule.

Do not put "Exit Sub" statements before the "End Sub". The function will end on "End Sub". "Exit Sub" is serving no real purpose here.

Private Sub SomeSubroutine()
'Your code here....
Exit Sub ' Bad code - Writing Exit Sub before End Sub.
End Sub

Private Sub SomeOtherSubroutine()
'Your code here....
End Sub

We have a program called SSW Code Auditor to check for this rule.

Optional parameters should be placed at the end of the method signature as optional ones tend to be less important. You should put the important parameters first.

public void SaveUserProfile(
  [Optional] string username,
  [Optional] string password,
  string firstName,
  string lastName, 
  [Optional] DateTime? birthDate
) {}

public void SaveUserProfile(
  string firstName,
  string lastName, 
  [Optional] string username,
  [Optional] string password,
  [Optional] DateTime? birthDate
) {}

Note: When using optional parameters, please be sure to use named para meters

When programming in form based environments one thing to remember is not to refer to form controls directly. The correct way is to pass the controls values that you need through parameters.

There are a number of benefits for doing this:

1. Debugging is simpler because all your parameters are in one place
2. If for some reason you need to change the control's name then you only have to change it in one place
3. The fact that nothing in your function is dependant on outside controls means you could very easily reuse your code in other areas without too many problems re-connecting the parameters being passed in

It's a correct method of programming.

Private Sub Command0_Click()
 CreateSchedule
End Sub
Sub CreateSchedule()
 Dim dteDateStart As Date
 Dim dteDateEnd As Date
 dteDateStart = Format(Me.ctlDateStart,"dd/mm/yyyy") 'Bad Code - refering the form control directly
 dteDateEnd = Format(Me.ctlDateEnd, "dd/mm/yyyy")
 .....processing code
End Sub

Private Sub Command0_Click()
 CreateSchedule(ctlDateStart, ctlDateEnd)
End Sub
Sub CreateSchedule(pdteDateStart As Date, pdteDateEnd As Date)
 Dim dteDateStart As Date
 Dim dteDateEnd As Date
 dteDateStart = Format(pdteDateStart, "dd/mm/yyyy") 'Good Code - refering the parameter directly
 dteDateEnd = Format(pdteDateEnd, "dd/mm/yyyy")
 &....processing code
End Sub

You should always write each parameter of MessageBox in a separate line. So it will be more clear to read in the code. Format your message text in code as you want to see on the screen.

Private Sub ShowMyMessage()
 MessageBox.Show("Are
 you sure you want to delete the team project """ + strProjectName
 + """?" + Environment.NewLine + Environment.NewLine + "Warning:
 Deleting a team project cannot be undone.", strProductName + "
 " + strVersion(), MessageBoxButtons.YesNo, MessageBoxIcon.Warning, MessageBoxDefaultButton.Button2)

Private Sub ShowMyMessage()
 MessageBox.Show( _ 
 "Are you sure you want to delete the team project """ + strProjectName + """?"
 _ + Environment.NewLine _ +
 Environment.NewLine _ +
 "Warning: Deleting a team project cannot be undone.", _
 strProductName + " " + strVersion(), _
 MessageBoxButtons.YesNo, _
 MessageBoxIcon.Warning, _
 MessageBoxDefaultButton.Button2)
End Sub

If you end up using someone else's code, or even idea, that you found online, make sure you add a reference to this in your source code. There is a good chance that you or your team will revisit the website. And of course, it never hurts to tip your hat, to thank other coders.

private void HideToSystemTray()
{
       // Hide the windows form in the system tray
       if (FormWindowState.Minimized == WindowState)
       { 
              Hide();
       } 
}

private void HideToSystemTray()
{
       // Hide the windows form in the system tray
       // I found this solution at http://www.developer.com/net/csharp/article.php/3336751
       if (FormWindowState.Minimized == WindowState)
       { 
              Hide();
       } 
}

For database applications, it is best to keep application-level values (apart from connection strings) from this in the database rather than in the web.config.  There are some merits as following:

1. It can be updated easily with normal SQL e.g. Rolling over the current setting to a new value.
2. It can be used in joins and in other queries easily without the need to pass in parameters.
3. It can be used to update settings and affect the other applications based on the same database.

Unit test classes should be suffixed with the word "Tests" for better coding readability.

[TestFixture] public class SqlValidatorReportTest { }

[TestFixture] public class HtmlDocumentTests { }

We have a program called SSW Code Auditor to check for this rule.

Enter Intro Text

Instead of:

private void RaiseUpdateOnExistingLotReceived() {
  if (ExistingLotUpdated != null) {
    ExistingLotUpdated();
  }
}

...use this event extension method:

public static void Raise<t>(
  this EventHandler<t> @event,
  object sender, 
  T args
) where T : EventArgs {
  var temp = @event;
  
  if (temp != null) {
    temp(sender, args);
  }
}

public static void Raise(this Action @event) {
  var temp = @event;

  if (temp != null) {
    temp();
  }
}

That means that instead of calling:

RaiseExistingLotUpdated();

...you can do:

ExistingLotUpdated.Raise();

Less code = less code to maintain = less code to be blamed for ;)

A regex is the best way to verify an email address.

public bool IsValidEmail(string email)
{
 // Return true if it is in valid email format.
 if (email.IndexOf("@") <= 0) return false; 
 if (email.EndWith("@")) return false; 
 if (email.IndexOf(".") <= 0) return false; 
 if ( ... 
}

public bool IsValidEmail(string email) 
{ 
 // Return true if it is in valid email format.
 return System.Text.RegularExpressions.Regex.IsMatch( email, 
 @"^([\w-\.]+)@(([[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.)|(([\w-]+\.)+))([a-zA-Z]{2,4}|[0-9]{1,3})(\]?)$";
}

A regex is the best way to verify an URI.

public bool IsValidUri(string uri)
{
try 
{ 
Uri testUri = new Uri(uri); 
return true; 
} 
catch (UriFormatException ex)
{ 
return false; 
} 
}

public bool IsValidUri(string uri) 
{ 
// Return true if it is in valid Uri format.
return System.Text.RegularExpressions.Regex.IsMatch( uri,@"^(http|ftp|https)://([^\/][\w-/:]+\.?)+([\w- ./?/:/;/\%&=]+)?(/[\w- ./?/:/;/\%&=]*)?"); 
}

You should have unit tests for it, see our Rules to Better Unit Tests for more information.

Use Enums instead of hard-coded strings, it makes your code lot cleaner and is really easy to manage .

When you need to create a new line in your string, make sure you use Environment.NewLine, and then literally begin typing your code on a new line for readability purposes.

string strExample = "This is a very long string that is \r\n not properly implementing a new line.";

string strExample = "This is a very long string that is " + Environment.NewLine +
		 " properly implementing a new line.";

var example = new StringBuilder();

example.AppendLine("This is a very long string that is ");

example.Append(" properly implementing a new line.");

Supporting old operating systems and old versions means you have more (and often messy) code, with lots of if or switch statements. This might be OK for you because you wrote the code, but down the track when someone else is maintaining it, then there is more time/expense needed.

When you realize there is a better way to do something, then you will change it, clean code should be the goal, however, because this affects old users, and changing interfaces at every whim also means an expense for all the apps that break, the decision isn't so easy to make.

Our views on backward compatibility start with asking these questions:

• Question 1: How many apps are we going to break externally?
• Question 2: How many apps are we going to break internally?
• Question 3: What is the cost of providing backward compatibility and repairing (and test) all the broken apps?

Let's look at an example:

If we change the URL of this public Web Service, we'd have to answer the questions as follows:

• Answer 1: Externally - Don't know, we have some leads: We can look at web stats and get an idea.  If an IP address enters our website at this point, it tells us that possibly an application is using it and the user isn't just following the links.
• Answer 2: Website samples + Adams code demo
• Answer 3: Can add a redirect or change the page to output a warning Old URL. Please see www.ssw.com.au/ PostCodeWebService for new URL

Because we know that not many external clients use this example, we decide to remove the old web service after some time.

Just to be friendly, we would send an email for the first month, and then another email in the second month.  After that, just emit "This is deprecated (old)."  We'll also need to update the UDDI so people don't keep coming to our old address.

We probably all prefer working on new features, rather than supporting old code, but it’s still a core part of the job. If your answer to question 3 scares you, it might be time to consider a backward compatibility warning.

Public/Protected properties have a number of advantages over public/protected fields:

• Data validation
  Data validation can be performed in the get/set accessors of a public property. This is especially important when working with the Visual Studio .NET Designer.
• Increased flexibility
  Properties conceal the data storage mechanism from the user, resulting in less broken code when the class is upgraded. Properties are a recommended object-oriented practice for this reason.
• Compatibility with data binding
  You can only bind to a public property, not a field.
• Minimal performance overhead
  The performance overhead for public properties is trivial. In some situations, public fields can actually have inferior performance to public properties.

public int Count;

public int Count
{
 get
 {
 return _count;
 }
 set
 {
 _count = value; 
 }
}

We agree that the syntax is tedious and think Microsoft should improve this.

Storing all the messages and global strings in one place will make it easy to manage them and to keep the applications in the same style.

Catch(SqlNullValueException sqlex)
{
Response.Write("The value cannot be null.");
}

Catch(SqlNullValueException sqlex)
{
Response.Write(GetGlobalResourceObject("Messages", "SqlValueNotNull"));
}

Catch(SqlNullValueException sqlex)
{
Response.Write(Resources.Messages.SqlValueNotNull); 'Good Code - storing message in resource file. 
}

All messages are stored in one central place so it's easy to reuse. Furthermore, it is strongly typed - easy to type with IntelliSense in Visual Studio.

Module Startup Dim HelloWorld As String = "Hello World!" Sub Main() Console.Write(HelloWorld)Console.Read() End Sub End Module

Module Startup Sub Main() Console.Write(My.Resources.Messages.Constant_HelloWorld) Console.Read() End Sub End Module

Since .NET 5+, the choice between using String.Empty and "" is a stylistic decision for the team. In .NET Framework, "" is less efficient than String.Empty from a memory perspective which can result in better performance due to faster garbage collection.

From the team that worked on performance in .NET: String.Empty vs "" in modern .NET language

public string myString 
   
{
 get
 {
 return ;
 } 
   
}

public string myString
{ 
   
 get 
   
 { 
   
 return string.Empty; 
   
 } 
   
}

We have a program called SSW Code Auditor to check for this rule.

Don't explicitly use "dispose" to close objects and dispose of them, the "using" statement will do all of them for you. It is another awesome tool that helps reduce coding effort and possible issues.

static int WriteLinesToFile(IEnumerable<string> lines)
{
  // We must declare the variable outside of the using block
  // so that it is in scope to be returned.
  int skippedLines = 0;
  var file = new System.IO.StreamWriter("WriteLines2.txt")
  foreach (string line in lines)
  {
    if (!line.Contains("Second"))
    {
      file.WriteLine(line);
    }
    else
    {
      skippedLines++;
    }
  }
  file.Dispose();
  return skippedLines;
}

static int WriteLinesToFile(IEnumerable<string> lines)
{
  // We must declare the variable outside of the using block
  // so that it is in scope to be returned.
  int skippedLines = 0;
  using (var file = new System.IO.StreamWriter("WriteLines2.txt"))
  {
    foreach (string line in lines)
    {
      if (!line.Contains("Second"))
      {
        file.WriteLine(line);
      }
      else
      {
        skippedLines++;
      }
    }
  } // file is disposed here
   return skippedLines;
}

static int WriteLinesToFile(IEnumerable<string> lines)
{
  using var file = new System.IO.StreamWriter("WriteLines2.txt");
  // Notice how we declare skippedLines after the using statement.
  int skippedLines = 0;
  foreach (string line in lines)
  {
    if (!line.Contains("Second"))
    {
      file.WriteLine(line);
    }
    else
    {
      skippedLines++;
     }
    }
    // Notice how skippedLines is in scope here.
    return skippedLines;
   // file is disposed here
}

When your application is about to start a long process (more than 30 seconds) it should first show a warning message to let the user know approximately how long it will take.

You will need to have 2 things:

1. A table to record processes containing the following fields:

   • ALogRecord (DateCreated, FunctionName, EmpUpdated, ComputerName, ActiveForm, ActiveControl, SystemsResources, ConventionalMemory, FormsCount, TimeStart, TimeEnd, TimeTaken, RecordsProcessed, Avg, Note, RowGuide, SSWTimeStamp)
2. A function to change the number of seconds lapsed to words - see the "1 minute, 9 seconds" in the above messagebox - this requires a SecondsToWords() function shown

Is your code DRY? Any logic that is used more than once, should be encapsulated in a method, and the method called wherever it is needed.

This will reduce redundancy, decrease maintenance effort, improve efficiency and reusability, and make the code more clear to read.

public class WarningEmail
{
    //...
    public void SendWarningEmail(string pFrom, string pTo, string pCC, string pUser, string pPwd, string pDomain)
    {
        //...
        MailMessage sMessage = new MailMessage();
        sMessage.From = new MailAddress(pFrom);
        sMessage.To.Add(pTo);
        sMessage.CC.Add(pCC);
        sMessage.Subject = "This is a Warning";
        sMessage.Body = GetWarning();
        SmtpClient sSmtpClient = new SmtpClient();
        sSmtpClient.Credentials = new NetworkCredential(pUser, pPwd, pDomain);
        sSmtpClient.Send(sMessage);
        //...
    }
}

public class ErrorEmail
{
    public void SendErrorEmail(string pFrom, string pTo, string pCC, string pUser, string pPwd, string pDomain)
    {
        //...
        MailMessage sMessage = new MailMessage();
        sMessage.From = new MailAddress(pFrom);
        sMessage.To.Add(pTo);
        sMessage.CC.Add(pCC);
        sMessage.Subject = "This is a Error";
        sMessage.Body = GetError();
        SmtpClient sSmtpClient = new SmtpClient();
        sSmtpClient.Credentials = new NetworkCredential(pUser, pPwd, pDomain);
        sSmtpClient.Send(sMessage);
        //...
    }
}

public class WarningEmail
{
    //...
    public void SendWarningEmail(string pFrom, string pTo, string pCC, string pUser, string pPwd, string pDomain)
    {
        //...
        EmailHelper.SendEmail(pFrom, pTo, pCC, "This is a Warning", GetWarning(), pUser, pPwd, pDomain);
        //...
    }
}

public class ErrorEmail
{
    public void SendErrorEmail(string pFrom, string pTo, string pCC, string pUser, string pPwd, string pDomain)
    {
        //...
        EmailHelper.SendEmail(pFrom, pTo, pCC, "This is an Error", GetError(), pUser, pPwd, pDomain);
        //...
    }
}

public class EmailHelper
{ 
    public static void SendEmail(string pFrom, string pTo, string pCC, string pSubject, string pBody, string pUser, string pPwd, string pDomain)
    {
        MailMessage sMessage = new MailMessage();
        sMessage.From = new MailAddress(pFrom);
        sMessage.To.Add(pTo);
        sMessage.CC.Add(pCC);
        sMessage.Subject = pSubject;
        sMessage.Body = pBody;
        SmtpClient sSmtpClient = new SmtpClient();
        sSmtpClient.Credentials = new NetworkCredential(pUser, pPwd, pDomain);
        sSmtpClient.Send(sMessage);
    } 
}