Analyzing naming conventions

Question 1

I just created a naming convention analyzer + code fix based on the Roslyn platform. The implemented naming conventions are the following:

enter image description here

If you're interested in seeing all the test scenarios that have been covered, you can take a look here.

Some notes about the implementation:

Analyzers and code fixes cannot share data between each other, which is a major pain. That's why I have to go find out which convention to use in my analyzer and do that again in my code fix. Though now that I think about it: maybe I can extract this common behaviour to a helper method. It's still double work but at least I won't have to write it double. Feel free to provide a proposal implementation.
Verbatim identifiers (@class) and Unicode-littered identifiers (cl\u0061ss) are not applicable for a renaming.
If the identifier contains special characters, I don't touch it either.
I just realized I didn't account for members without explicit modifiers. You can ignore that aspect and pretend I don't do them.

You can view the PR related to this post here.

Analyzer

using System.Collections.Immutable;
using System.Linq;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using VSDiagnostics.Utilities;
namespace VSDiagnostics.Diagnostics.General.NamingConventions
{
 [DiagnosticAnalyzer(LanguageNames.CSharp)]
 public class NamingConventionsAnalyzer : DiagnosticAnalyzer
 {
 public const string DiagnosticId = nameof(NamingConventionsAnalyzer);
 internal const string Title = "A member does not follow naming conventions.";
 internal const string Message = "The {0} {1} does not follow naming conventions. Should be {2}.";
 internal const string Category = "General";
 internal const DiagnosticSeverity Severity = DiagnosticSeverity.Warning;
 internal static DiagnosticDescriptor Rule = new DiagnosticDescriptor(DiagnosticId, Title, Message, Category, Severity, true);
 public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);
 public override void Initialize(AnalysisContext context)
 {
 context.RegisterSyntaxNodeAction(AnalyzeSymbol,
 SyntaxKind.FieldDeclaration,
 SyntaxKind.PropertyDeclaration,
 SyntaxKind.MethodDeclaration,
 SyntaxKind.ClassDeclaration,
 SyntaxKind.InterfaceDeclaration,
 SyntaxKind.LocalDeclarationStatement,
 SyntaxKind.Parameter);
 }
 private void AnalyzeSymbol(SyntaxNodeAnalysisContext context)
 {
 var nodeAsField = context.Node as FieldDeclarationSyntax;
 if (nodeAsField != null)
 {
 if (nodeAsField.Declaration == null)
 {
 return;
 }
 foreach (var variable in nodeAsField.Declaration.Variables)
 {
 SyntaxToken conventionedIdentifier;
 if (nodeAsField.Modifiers.Any(x => new[] { "internal", "protected", "public" }.Contains(x.Text)))
 {
 conventionedIdentifier = variable.Identifier.WithConvention(NamingConvention.UpperCamelCase);
 }
 else if (nodeAsField.Modifiers.Any(x => x.Text == "private"))
 {
 conventionedIdentifier = variable.Identifier.WithConvention(NamingConvention.UnderscoreLowerCamelCase);
 }
 else
 {
 return; // Code is in incomplete state
 }
 if (conventionedIdentifier.Text != variable.Identifier.Text)
 {
 context.ReportDiagnostic(Diagnostic.Create(Rule, variable.Identifier.GetLocation(), "field", variable.Identifier.Text, conventionedIdentifier.Text));
 }
 }
 return;
 }
 var nodeAsProperty = context.Node as PropertyDeclarationSyntax;
 if (nodeAsProperty != null)
 {
 var conventionedIdentifier = nodeAsProperty.Identifier.WithConvention(NamingConvention.UpperCamelCase);
 if (conventionedIdentifier.Text != nodeAsProperty.Identifier.Text)
 {
 context.ReportDiagnostic(Diagnostic.Create(Rule, nodeAsProperty.Identifier.GetLocation(), "property", nodeAsProperty.Identifier.Text, conventionedIdentifier.Text));
 }
 return;
 }
 var nodeAsMethod = context.Node as MethodDeclarationSyntax;
 if (nodeAsMethod != null)
 {
 var conventionedIdentifier = nodeAsMethod.Identifier.WithConvention(NamingConvention.UpperCamelCase);
 if (conventionedIdentifier.Text != nodeAsMethod.Identifier.Text)
 {
 context.ReportDiagnostic(Diagnostic.Create(Rule, nodeAsMethod.Identifier.GetLocation(), "method", nodeAsMethod.Identifier.Text, conventionedIdentifier.Text));
 }
 return;
 }
 var nodeAsClass = context.Node as ClassDeclarationSyntax;
 if (nodeAsClass != null)
 {
 var conventionedIdentifier = nodeAsClass.Identifier.WithConvention(NamingConvention.UpperCamelCase);
 if (conventionedIdentifier.Text != nodeAsClass.Identifier.Text)
 {
 context.ReportDiagnostic(Diagnostic.Create(Rule, nodeAsClass.Identifier.GetLocation(), "class", nodeAsClass.Identifier.Text, conventionedIdentifier.Text));
 }
 return;
 }
 var nodeAsInterface = context.Node as InterfaceDeclarationSyntax;
 if (nodeAsInterface != null)
 {
 var conventionedIdentifier = nodeAsInterface.Identifier.WithConvention(NamingConvention.InterfacePrefixUpperCamelCase);
 if (conventionedIdentifier.Text != nodeAsInterface.Identifier.Text)
 {
 context.ReportDiagnostic(Diagnostic.Create(Rule, nodeAsInterface.Identifier.GetLocation(), "interface", nodeAsInterface.Identifier.Text, conventionedIdentifier.Text));
 }
 return;
 }
 var nodeAsLocal = context.Node as LocalDeclarationStatementSyntax;
 if (nodeAsLocal != null)
 {
 if (nodeAsLocal.Declaration == null)
 {
 return;
 }
 foreach (var variable in nodeAsLocal.Declaration.Variables)
 {
 var conventionedIdentifier = variable.Identifier.WithConvention(NamingConvention.LowerCamelCase);
 if (conventionedIdentifier.Text != variable.Identifier.Text)
 {
 context.ReportDiagnostic(Diagnostic.Create(Rule, variable.Identifier.GetLocation(), "local", variable.Identifier.Text, conventionedIdentifier.Text));
 }
 }
 return;
 }
 var nodeAsParameter = context.Node as ParameterSyntax;
 if (nodeAsParameter != null)
 {
 var conventionedIdentifier = nodeAsParameter.Identifier.WithConvention(NamingConvention.LowerCamelCase);
 if (conventionedIdentifier.Text != nodeAsParameter.Identifier.Text)
 {
 context.ReportDiagnostic(Diagnostic.Create(Rule, nodeAsParameter.Identifier.GetLocation(), "parameter", nodeAsParameter.Identifier.Text, conventionedIdentifier.Text));
 }
 }
 }
 }
}

Code Fix

using System.Collections.Immutable;
using System.Composition;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using VSDiagnostics.Utilities;
namespace VSDiagnostics.Diagnostics.General.NamingConventions
{
 [ExportCodeFixProvider("NamingConventions", LanguageNames.CSharp), Shared]
 public class NamingConventionsCodeFix : CodeFixProvider
 {
 public override ImmutableArray<string> FixableDiagnosticIds => ImmutableArray.Create(NamingConventionsAnalyzer.DiagnosticId);
 public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;
 public override async Task RegisterCodeFixesAsync(CodeFixContext context)
 {
 var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
 var diagnostic = context.Diagnostics.First();
 var diagnosticSpan = diagnostic.Location.SourceSpan;
 var identifier = root.FindToken(diagnosticSpan.Start);
 context.RegisterCodeFix(CodeAction.Create("Rename", x => RenameAsync(context.Document, root, identifier)), diagnostic);
 }
 private Task<Solution> RenameAsync(Document document, SyntaxNode root, SyntaxToken identifier)
 {
 var identifierParent = identifier.Parent;
 var newIdentifier = default(SyntaxToken);
 do
 {
 var parentAsField = identifierParent as FieldDeclarationSyntax;
 if (parentAsField != null)
 {
 if (parentAsField.Modifiers.Any(x => new[] { "internal", "protected", "public" }.Contains(x.Text)))
 {
 newIdentifier = identifier.WithConvention(NamingConvention.UpperCamelCase);
 }
 else
 {
 newIdentifier = identifier.WithConvention(NamingConvention.UnderscoreLowerCamelCase);
 }
 break;
 }
 if (identifierParent is PropertyDeclarationSyntax || identifierParent is MethodDeclarationSyntax || identifierParent is ClassDeclarationSyntax)
 {
 newIdentifier = identifier.WithConvention(NamingConvention.UpperCamelCase);
 break;
 }
 if (identifierParent is LocalDeclarationStatementSyntax || identifierParent is ParameterSyntax)
 {
 newIdentifier = identifier.WithConvention(NamingConvention.LowerCamelCase);
 break;
 }
 if (identifierParent is InterfaceDeclarationSyntax)
 {
 newIdentifier = identifier.WithConvention(NamingConvention.InterfacePrefixUpperCamelCase);
 break;
 }
 identifierParent = identifierParent.Parent;
 } while (identifierParent != null);
 var newParent = identifierParent.ReplaceToken(identifier, newIdentifier);
 var newRoot = root.ReplaceNode(identifierParent, newParent);
 return Task.FromResult(document.WithSyntaxRoot(newRoot).Project.Solution);
 }
 }
}

Extensions

using System;
using System.Linq;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
namespace VSDiagnostics.Utilities
{
 public static class Extensions
 {
 public static SyntaxToken WithConvention(this SyntaxToken identifier, NamingConvention namingConvention)
 {
 // int @class = 5;
 if (identifier.IsVerbatimIdentifier())
 {
 return identifier;
 }
 // int cl\u0061ss = 5;
 if (identifier.Text.Contains("\\"))
 {
 return identifier;
 }
 var originalValue = identifier.ValueText;
 string newValue;
 switch (namingConvention)
 {
 case NamingConvention.LowerCamelCase:
 newValue = GetLowerCamelCaseIdentifier(originalValue);
 break;
 case NamingConvention.UpperCamelCase:
 newValue = GetUpperCamelCaseIdentifier(originalValue);
 break;
 case NamingConvention.UnderscoreLowerCamelCase:
 newValue = GetUnderscoreLowerCamelCaseIdentifier(originalValue);
 break;
 case NamingConvention.InterfacePrefixUpperCamelCase:
 newValue = GetInterfacePrefixUpperCamelCaseIdentifier(originalValue);
 break;
 default:
 throw new ArgumentException(nameof(namingConvention));
 }
 return SyntaxFactory.Identifier(identifier.LeadingTrivia, newValue, identifier.TrailingTrivia);
 }
 // lowerCamelCase
 private static string GetLowerCamelCaseIdentifier(string identifier)
 {
 if (ContainsSpecialCharacters(identifier))
 {
 return identifier;
 }
 var normalizedString = GetNormalizedString(identifier);
 if (normalizedString.Length >= 1)
 {
 return char.ToLower(normalizedString[0]) + normalizedString.Substring(1);
 }
 return identifier;
 }
 // UpperCamelCase
 private static string GetUpperCamelCaseIdentifier(string identifier)
 {
 if (ContainsSpecialCharacters(identifier))
 {
 return identifier;
 }
 var normalizedString = GetNormalizedString(identifier);
 if (normalizedString.Length == 0)
 {
 return identifier;
 }
 return char.ToUpper(normalizedString[0]) + normalizedString.Substring(1);
 }
 // _lowerCamelCase
 private static string GetUnderscoreLowerCamelCaseIdentifier(string identifier)
 {
 if (ContainsSpecialCharacters(identifier, '_'))
 {
 return identifier;
 }
 var normalizedString = GetNormalizedString(identifier);
 if (normalizedString.Length == 0)
 {
 return identifier;
 }
 if (normalizedString.Length == 1)
 {
 return "_" + char.ToLower(normalizedString[0]);
 }
 // _Var
 if (normalizedString[0] == '_' && char.IsUpper(normalizedString[1]))
 {
 return "_" + char.ToLower(normalizedString[1]) + normalizedString.Substring(2);
 }
 // Var
 if (char.IsUpper(normalizedString[0]))
 {
 return "_" + char.ToLower(normalizedString[0]) + normalizedString.Substring(1);
 }
 // var
 if (char.IsLower(normalizedString[0]))
 {
 return "_" + normalizedString;
 }
 return normalizedString;
 }
 // IInterface
 private static string GetInterfacePrefixUpperCamelCaseIdentifier(string identifier)
 {
 if (ContainsSpecialCharacters(identifier))
 {
 return identifier;
 }
 var normalizedString = GetNormalizedString(identifier);
 if (normalizedString.Length <= 1)
 {
 return identifier;
 }
 // iSomething
 if (normalizedString[0] == 'i' && char.IsUpper(normalizedString[1]))
 {
 return "I" + normalizedString.Substring(1);
 }
 // isomething
 if (char.IsLower(normalizedString[0]) && char.IsLower(normalizedString[1]))
 {
 return "I" + char.ToUpper(normalizedString[0]) + normalizedString.Substring(1);
 }
 // Isomething
 if (normalizedString[0] == 'I' && char.IsLower(normalizedString[1]))
 {
 return "I" + char.ToUpper(normalizedString[1]) + normalizedString.Substring(2);
 }
 return normalizedString;
 }
 private static string GetNormalizedString(string input)
 {
 return new string(input.ToCharArray().Where(x => char.IsLetter(x) || char.IsNumber(x)).ToArray());
 }
 private static bool ContainsSpecialCharacters(string input, params char[] allowedCharacters)
 {
 return !input.ToCharArray().All(x => char.IsLetter(x) || char.IsNumber(x) || allowedCharacters.Contains(x));
 }
 }
}

NamingConvention

namespace VSDiagnostics.Utilities
{
 public enum NamingConvention
 {
 UpperCamelCase,
 LowerCamelCase,
 UnderscoreLowerCamelCase,
 InterfacePrefixUpperCamelCase
 }
}

Question 2

if (nodeAsField.Modifiers.Any(x => new[] { "internal", "protected", "public" }.Contains(x.Text)))
{
 // ...
}
else if (nodeAsField.Modifiers.Any(x => x.Text == "private"))
{
 // ...
}

I find the following alternative a little easier to read, and less prone to typos:

var modifiers = nodeAsField.Modifiers;
if (modifiers.Any(SyntaxKind.InternalKeyword) ||
 modifiers.Any(SyntaxKind.ProtectedKeyword) ||
 modifiers.Any(SyntaxKind.PublicKeyword))
{
 // ...
}
else if (modifiers.Any(SyntaxKind.PrivateKeyword))
{
 // ...
}

It might also be more performant since we avoid string comparisons and array allocations in the lambda (though of course you should measure this yourself).

Heap Allocations Viewer also warns that in the original code nodeAsField.Modifiers is boxed.

I noticed that structs are missing from both the naming conventions table and the code.

Question 3

You're accessing the Identifier more than often enough here to use a variable, in my humble opinion.

var conventionedIdentifier = nodeAsMethod.Identifier.WithConvention(NamingConvention.UpperCamelCase);
 if (conventionedIdentifier.Text != nodeAsMethod.Identifier.Text)
 {
 context.ReportDiagnostic(Diagnostic.Create(Rule, nodeAsMethod.Identifier.GetLocation(), "method", nodeAsMethod.Identifier.Text, conventionedIdentifier.Text));
 }

Doing so would dramatically reduce the horizontal scroll.

This logic gets repeated an awful lot too. It only differs in a few places. It should be possible to extract a method something like this.

private Diagnostic CheckNode(Node node, SyntaxKind kind)
{
 Diagnostic diagnostic = null;
 // some code to dynamically cast??
 if (node != null)
 {
 var conventionedIdentifier = node.Identifier.WithConvention(GetNamingConvention(kind));
 if (conventionedIdentifier.Text != node.Identifier.Text)
 {
 diagnostic = Diagnostic.Create(Rule, node.Identifier.GetLocation(), GetFriendlySyntaxKindText(kind), node.Identifier.Text, conventionedIdentifier.Text);
 }
 }
 return diagnostic;
}

Untested and the types might not be right, but I'm sure you get the gist. There are also a couple of unimplemented methods in there. They'll let you abstract away the decisions you made for each different SyntaxKind into single methods in the source code.

The calling code will end up looking significantly different.

 diagnostic = CheckNode(context.Node, SyntaxKind.PropertyDeclaration);
 if (diagnostic != null) 
 {
 context.ReportDiagnostic(diagnostic);
 return;
 }
 diagnostic = CheckNode(context.Node, SyntaxKind.MethodDeclaration);
 if (diagnostic != null) 
 {
 context.ReportDiagnostic(diagnostic);
 return;
 }

Which is going to turn into a second round of refactoring where you determine the kind dynamically somehow. (So don't know how though.)

Question 4

I updated the code with 3 changes: implicit access modifiers for private fields are now accounted for, I drastically improved readability of the analyzer but couldn't get it as far as I wanted to due to Roslyn's inheritance structure and I removed some pointless code for fixing the naming of _lowerCamelCase types. All changes can be found in this commit: github.com/Vannevelj/VSDiagnostics/commit/…

Question 5

There are a couple of unexpected results returned by the code fix provider:

GetInterfacePrefixUpperCamelCaseIdentifier("Foo") => Foo, expected IFoo
GetLowerCamelCaseIdentifier("_foo") => _foo, expected foo
GetUpperCamelCaseIdentifier("_foo") => _foo, expected Foo
GetInterfacePrefixUpperCamelCaseIdentifier("_foo") => _foo, expected IFoo

Question 6

I think I've now covered these and a few more: github.com/Vannevelj/VSDiagnostics/commit/…

mjolka mjolka 16.3k2 gold badges30 silver badges73 bronze badges · Answer 1 · 2015-06-03 03:08:57Z

if (nodeAsField.Modifiers.Any(x => new[] { "internal", "protected", "public" }.Contains(x.Text)))
{
 // ...
}
else if (nodeAsField.Modifiers.Any(x => x.Text == "private"))
{
 // ...
}

I find the following alternative a little easier to read, and less prone to typos:

var modifiers = nodeAsField.Modifiers;
if (modifiers.Any(SyntaxKind.InternalKeyword) ||
 modifiers.Any(SyntaxKind.ProtectedKeyword) ||
 modifiers.Any(SyntaxKind.PublicKeyword))
{
 // ...
}
else if (modifiers.Any(SyntaxKind.PrivateKeyword))
{
 // ...
}

It might also be more performant since we avoid string comparisons and array allocations in the lambda (though of course you should measure this yourself).

Heap Allocations Viewer also warns that in the original code nodeAsField.Modifiers is boxed.

I noticed that structs are missing from both the naming conventions table and the code.

RubberDuck RubberDuck 31.1k6 gold badges73 silver badges176 bronze badges · Answer 2 · 2015-06-02 23:18:35Z

You're accessing the Identifier more than often enough here to use a variable, in my humble opinion.

var conventionedIdentifier = nodeAsMethod.Identifier.WithConvention(NamingConvention.UpperCamelCase);
 if (conventionedIdentifier.Text != nodeAsMethod.Identifier.Text)
 {
 context.ReportDiagnostic(Diagnostic.Create(Rule, nodeAsMethod.Identifier.GetLocation(), "method", nodeAsMethod.Identifier.Text, conventionedIdentifier.Text));
 }

Doing so would dramatically reduce the horizontal scroll.

This logic gets repeated an awful lot too. It only differs in a few places. It should be possible to extract a method something like this.

private Diagnostic CheckNode(Node node, SyntaxKind kind)
{
 Diagnostic diagnostic = null;
 // some code to dynamically cast??
 if (node != null)
 {
 var conventionedIdentifier = node.Identifier.WithConvention(GetNamingConvention(kind));
 if (conventionedIdentifier.Text != node.Identifier.Text)
 {
 diagnostic = Diagnostic.Create(Rule, node.Identifier.GetLocation(), GetFriendlySyntaxKindText(kind), node.Identifier.Text, conventionedIdentifier.Text);
 }
 }
 return diagnostic;
}

Untested and the types might not be right, but I'm sure you get the gist. There are also a couple of unimplemented methods in there. They'll let you abstract away the decisions you made for each different SyntaxKind into single methods in the source code.

The calling code will end up looking significantly different.

 diagnostic = CheckNode(context.Node, SyntaxKind.PropertyDeclaration);
 if (diagnostic != null) 
 {
 context.ReportDiagnostic(diagnostic);
 return;
 }
 diagnostic = CheckNode(context.Node, SyntaxKind.MethodDeclaration);
 if (diagnostic != null) 
 {
 context.ReportDiagnostic(diagnostic);
 return;
 }

Which is going to turn into a second round of refactoring where you determine the kind dynamically somehow. (So don't know how though.)

I updated the code with 3 changes: implicit access modifiers for private fields are now accounted for, I drastically improved readability of the analyzer but couldn't get it as far as I wanted to due to Roslyn's inheritance structure and I removed some pointless code for fixing the naming of _lowerCamelCase types. All changes can be found in this commit: github.com/Vannevelj/VSDiagnostics/commit/…

mjolka mjolka 16.3k2 gold badges30 silver badges73 bronze badges · Answer 3 · 2015-06-03 13:24:33Z

There are a couple of unexpected results returned by the code fix provider:

GetInterfacePrefixUpperCamelCaseIdentifier("Foo") => Foo, expected IFoo
GetLowerCamelCaseIdentifier("_foo") => _foo, expected foo
GetUpperCamelCaseIdentifier("_foo") => _foo, expected Foo
GetInterfacePrefixUpperCamelCaseIdentifier("_foo") => _foo, expected IFoo

I think I've now covered these and a few more: github.com/Vannevelj/VSDiagnostics/commit/…

Stack Exchange Network

Analyzing naming conventions

Analyzer

Code Fix

Extensions

NamingConvention

3 Answers 3

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Linked

Hot Network Questions

Analyzing naming conventions

Analyzer

Code Fix

Extensions

NamingConvention

3 Answers 3

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Linked

Related

Hot Network Questions