-
Notifications
You must be signed in to change notification settings - Fork 299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Code Metrics #3522
Code Metrics #3522
Changes from 21 commits
030345e
ea2ef2a
6d50282
8207ab7
8f87314
dfb61b0
2542c6b
c5da161
d52ddac
12b49bc
76f3914
489038d
b6ac898
9d42028
43fab4e
7fcdd7e
8794aab
db3e117
ec3061a
01fb5aa
0f1f57b
5269111
022c377
de596b4
44dfb16
fcf9d42
7aac086
9a46e6e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,194 @@ | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using Rubberduck.Parsing.VBA; | ||
using Antlr4.Runtime.Tree; | ||
using Rubberduck.Parsing.Grammar; | ||
using Rubberduck.VBEditor; | ||
using Antlr4.Runtime.Misc; | ||
using Rubberduck.Parsing.Symbols; | ||
using Rubberduck.SmartIndenter; | ||
|
||
namespace Rubberduck.Navigation.CodeMetrics | ||
{ | ||
public class CodeMetricsAnalyst : ICodeMetricsAnalyst | ||
{ | ||
private readonly IIndenterSettings indenterSettings; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The usual convention in RD would be |
||
|
||
public CodeMetricsAnalyst(IIndenterSettings indenterSettings) | ||
{ | ||
this.indenterSettings = indenterSettings; | ||
} | ||
|
||
public IEnumerable<ModuleMetricsResult> ModuleMetrics(RubberduckParserState state) | ||
{ | ||
if (state == null || !state.AllUserDeclarations.Any()) | ||
{ | ||
// must not return Enumerable.Empty | ||
yield break; | ||
} | ||
|
||
var trees = state.ParseTrees; | ||
var results = new List<CodeMetricsResult>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does not seem to be used anywhere. |
||
|
||
foreach (var moduleTree in trees) | ||
{ | ||
yield return GetModuleResult(moduleTree.Key, moduleTree.Value, state.DeclarationFinder); | ||
}; | ||
} | ||
|
||
public ModuleMetricsResult GetModuleResult(RubberduckParserState state, QualifiedModuleName qmn) | ||
{ | ||
return GetModuleResult(qmn, state.GetParseTree(qmn), state.DeclarationFinder); | ||
} | ||
|
||
private ModuleMetricsResult GetModuleResult(QualifiedModuleName qmn, IParseTree moduleTree, DeclarationFinder declarationFinder) | ||
{ | ||
// Consider rewrite as visitor? That should make subtrees easier and allow us to expand metrics | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that a visitor can be cancelled whether a walker can't be as easily cancelled. That may have some UI ramifications if there can be a delay when interacting with the UI. If it executes quickly and UI is responsive, then it might be just be fine as a walker. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure can be. Just check a CancellationToken when entering any rule. Bit of a pain though, so this usually isn't done... |
||
var cmListener = new CodeMetricsListener(declarationFinder, indenterSettings); | ||
ParseTreeWalker.Default.Walk(cmListener, moduleTree); | ||
return cmListener.GetMetricsResult(qmn); | ||
} | ||
|
||
|
||
private class CodeMetricsListener : VBAParserBaseListener | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is there only one listener for all three types of code metrics? Wouldn't it be better to have one for each type and then just combine them for the walk over the tree? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same reasoning as above. In principle the idea is correct, it seemed like too much of a hassle for something we'd probably not need. |
||
{ | ||
private readonly DeclarationFinder _finder; | ||
private readonly IIndenterSettings _indenterSettings; | ||
|
||
private Declaration currentMember; | ||
private List<CodeMetricsResult> results = new List<CodeMetricsResult>(); | ||
private List<CodeMetricsResult> moduleResults = new List<CodeMetricsResult>(); | ||
|
||
private List<MemberMetricsResult> memberResults = new List<MemberMetricsResult>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the variables above are all instance variables, they should start with an underscore. |
||
|
||
public CodeMetricsListener(DeclarationFinder finder, IIndenterSettings indenterSettings) | ||
{ | ||
_finder = finder; | ||
_indenterSettings = indenterSettings; | ||
} | ||
|
||
public override void EnterEndOfLine([NotNull] VBAParser.EndOfLineContext context) | ||
{ | ||
int followingIndentationLevel = 0; | ||
// we have a proper newline | ||
if (context.NEWLINE() != null) | ||
{ | ||
// the last whitespace, which is the one in front of the next line's contents | ||
var followingWhitespace = context.whiteSpace().LastOrDefault(); | ||
followingIndentationLevel = IndentationLevelFromWhitespace(followingWhitespace); | ||
} | ||
(currentMember == null ? moduleResults : results).Add(new CodeMetricsResult(1, 0, followingIndentationLevel)); | ||
} | ||
|
||
public override void EnterIfStmt([NotNull] VBAParser.IfStmtContext context) | ||
{ | ||
results.Add(new CodeMetricsResult(0, 1, 0)); | ||
} | ||
|
||
public override void EnterElseIfBlock([NotNull] VBAParser.ElseIfBlockContext context) | ||
{ | ||
results.Add(new CodeMetricsResult(0, 1, 0)); | ||
} | ||
|
||
// notably: NO additional complexity for an Else-Block | ||
|
||
public override void EnterForEachStmt([NotNull] VBAParser.ForEachStmtContext context) | ||
{ | ||
results.Add(new CodeMetricsResult(0, 1, 0)); | ||
} | ||
|
||
public override void EnterForNextStmt([NotNull] VBAParser.ForNextStmtContext context) | ||
{ | ||
results.Add(new CodeMetricsResult(0, 1, 0)); | ||
} | ||
|
||
public override void EnterCaseClause([NotNull] VBAParser.CaseClauseContext context) | ||
{ | ||
results.Add(new CodeMetricsResult(0, 1, 0)); | ||
} | ||
|
||
public override void EnterSubStmt([NotNull] VBAParser.SubStmtContext context) | ||
{ | ||
results.Add(new CodeMetricsResult(0, 1, 0)); | ||
currentMember = _finder.DeclarationsWithType(DeclarationType.Procedure).Where(d => d.Context == context).First(); | ||
} | ||
|
||
public override void ExitSubStmt([NotNull] VBAParser.SubStmtContext context) | ||
{ | ||
ExitMeasurableMember(); | ||
} | ||
|
||
public override void EnterFunctionStmt([NotNull] VBAParser.FunctionStmtContext context) | ||
{ | ||
results.Add(new CodeMetricsResult(0, 1, 0)); | ||
currentMember = _finder.DeclarationsWithType(DeclarationType.Function).Where(d => d.Context == context).First(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be more appropriate to use |
||
} | ||
|
||
public override void ExitFunctionStmt([NotNull] VBAParser.FunctionStmtContext context) | ||
{ | ||
ExitMeasurableMember(); | ||
} | ||
|
||
public override void EnterPropertyGetStmt([NotNull] VBAParser.PropertyGetStmtContext context) | ||
{ | ||
results.Add(new CodeMetricsResult(0, 1, 0)); | ||
currentMember = _finder.DeclarationsWithType(DeclarationType.PropertyGet).Where(d => d.Context == context).First(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As above |
||
} | ||
|
||
public override void ExitPropertyGetStmt([NotNull] VBAParser.PropertyGetStmtContext context) | ||
{ | ||
ExitMeasurableMember(); | ||
} | ||
|
||
public override void EnterPropertyLetStmt([NotNull] VBAParser.PropertyLetStmtContext context) | ||
{ | ||
results.Add(new CodeMetricsResult(0, 1, 0)); | ||
currentMember = _finder.DeclarationsWithType(DeclarationType.PropertyLet).Where(d => d.Context == context).First(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dito |
||
} | ||
|
||
public override void ExitPropertyLetStmt([NotNull] VBAParser.PropertyLetStmtContext context) | ||
{ | ||
ExitMeasurableMember(); | ||
} | ||
|
||
public override void EnterPropertySetStmt([NotNull] VBAParser.PropertySetStmtContext context) | ||
{ | ||
results.Add(new CodeMetricsResult(0, 1, 0)); | ||
currentMember = _finder.DeclarationsWithType(DeclarationType.PropertySet).Where(d => d.Context == context).First(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dito |
||
} | ||
|
||
public override void ExitPropertySetStmt([NotNull] VBAParser.PropertySetStmtContext context) | ||
{ | ||
ExitMeasurableMember(); | ||
} | ||
|
||
public override void EnterBlockStmt([NotNull] VBAParser.BlockStmtContext context) | ||
{ | ||
// there is a whitespace context here after the option of a statementLabel. | ||
// we need to account for that | ||
results.Add(new CodeMetricsResult(0, 0, IndentationLevelFromWhitespace(context.whiteSpace()))); | ||
} | ||
|
||
private int IndentationLevelFromWhitespace(VBAParser.WhiteSpaceContext wsContext) | ||
{ | ||
if (wsContext == null) return 0; | ||
// the only thing that contains underscores is the line-continuation at this point | ||
var lineContinuation = wsContext.children.LastOrDefault((tree) => tree.GetText().Contains("_")); | ||
var index = lineContinuation != null ? wsContext.children.IndexOf(lineContinuation) : 0; | ||
return (wsContext?.ChildCount ?? 0 - index) / _indenterSettings.IndentSpaces; | ||
} | ||
|
||
private void ExitMeasurableMember() | ||
{ | ||
memberResults.Add(new MemberMetricsResult(currentMember, results)); | ||
results = new List<CodeMetricsResult>(); // reinitialize to drop results | ||
currentMember = null; | ||
} | ||
|
||
internal ModuleMetricsResult GetMetricsResult(QualifiedModuleName qmn) | ||
{ | ||
return new ModuleMetricsResult(qmn, memberResults, moduleResults); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
using Rubberduck.Parsing.Symbols; | ||
using Rubberduck.VBEditor; | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
|
||
namespace Rubberduck.Navigation.CodeMetrics | ||
{ | ||
public struct CodeMetricsResult | ||
{ | ||
public CodeMetricsResult(int lines, int cyclomaticComplexity, int nesting) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we hard-coding the types of code metrics we support? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. because aggregation may follow different rules depending on the metric. Since that's the case, we'd need to accomodate an aggregation in all kinds of metrics. Last but not least this seemed like the least effort. This PR is "only" a first iteration of the feature, and future iterations may well change this. But I don't think we can reasonably expect user-contributed code-metrics. It's just not something I think we're going to need. |
||
: this(lines, cyclomaticComplexity, nesting, Enumerable.Empty<CodeMetricsResult>()) | ||
{ | ||
} | ||
|
||
public CodeMetricsResult(int lines, int cyclomaticComplexity, int nesting, IEnumerable<CodeMetricsResult> childScopeResults) | ||
{ | ||
var childScopeMetric = | ||
childScopeResults.Aggregate(new CodeMetricsResult(), (r1, r2) => new CodeMetricsResult(r1.Lines + r2.Lines, r1.CyclomaticComplexity + r2.CyclomaticComplexity, Math.Max(r1.MaximumNesting, r2.MaximumNesting))); | ||
Lines = lines + childScopeMetric.Lines; | ||
CyclomaticComplexity = cyclomaticComplexity + childScopeMetric.CyclomaticComplexity; | ||
MaximumNesting = Math.Max(nesting, childScopeMetric.MaximumNesting); | ||
} | ||
|
||
public int Lines { get; private set; } | ||
public int CyclomaticComplexity { get; private set; } | ||
public int MaximumNesting { get; private set; } | ||
|
||
} | ||
|
||
public struct MemberMetricsResult | ||
{ | ||
public Declaration Member { get; private set; } | ||
public CodeMetricsResult Result { get; private set; } | ||
|
||
public MemberMetricsResult(Declaration member, IEnumerable<CodeMetricsResult> contextResults) | ||
{ | ||
Member = member; | ||
Result = new CodeMetricsResult(0, 0, 0, contextResults); | ||
} | ||
} | ||
|
||
public struct ModuleMetricsResult | ||
{ | ||
public QualifiedModuleName ModuleName { get; private set; } | ||
public CodeMetricsResult Result { get; private set; } | ||
public IReadOnlyDictionary<Declaration, CodeMetricsResult> MemberResults { get; private set; } | ||
|
||
public ModuleMetricsResult(QualifiedModuleName moduleName, IEnumerable<MemberMetricsResult> memberMetricsResult, IEnumerable<CodeMetricsResult> nonMemberResults) | ||
{ | ||
ModuleName = moduleName; | ||
MemberResults = memberMetricsResult.ToDictionary(mmr => mmr.Member, mmr => mmr.Result); | ||
Result = new CodeMetricsResult(0, 0, 0, nonMemberResults.Concat(MemberResults.Values)); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
using NLog; | ||
using Rubberduck.Parsing.VBA; | ||
using Rubberduck.UI; | ||
using Rubberduck.UI.Command; | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
|
||
namespace Rubberduck.Navigation.CodeMetrics | ||
{ | ||
public class CodeMetricsViewModel : ViewModelBase | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The class should implement |
||
{ | ||
private readonly RubberduckParserState _state; | ||
|
||
public CodeMetricsViewModel(RubberduckParserState state, ICodeMetricsAnalyst analyst) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure what this is doing :P |
||
{ | ||
_state = state; | ||
// FIXME deregister event on destruction | ||
_state.StateChanged += (_, change) => | ||
{ | ||
if (change.State == ParserState.Ready) | ||
{ | ||
ModuleMetrics = analyst.ModuleMetrics(_state); | ||
} | ||
}; | ||
} | ||
|
||
public void FilterByName(object projects, string text) | ||
{ | ||
throw new NotImplementedException(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should it really be here? |
||
} | ||
|
||
private IEnumerable<ModuleMetricsResult> _moduleMetrics; | ||
public IEnumerable<ModuleMetricsResult> ModuleMetrics { | ||
get => _moduleMetrics; | ||
private set | ||
{ | ||
_moduleMetrics = value; | ||
OnPropertyChanged(); | ||
} | ||
} | ||
|
||
|
||
//public CommandBase RefreshCommand { get; set; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Commented code shouldn't be here, either. |
||
|
||
|
||
private bool _canSearch; | ||
public bool CanSearch | ||
{ | ||
get => _canSearch; | ||
set | ||
{ | ||
_canSearch = value; | ||
OnPropertyChanged(); | ||
} | ||
} | ||
|
||
private bool _isBusy; | ||
public bool IsBusy | ||
{ | ||
get => _isBusy; | ||
set | ||
{ | ||
_isBusy = value; | ||
OnPropertyChanged(); | ||
// If the window is "busy" then hide the Refresh message | ||
OnPropertyChanged("EmptyUIRefreshMessageVisibility"); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
using Rubberduck.Parsing.VBA; | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Text; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
|
||
namespace Rubberduck.Navigation.CodeMetrics | ||
{ | ||
public interface ICodeMetricsAnalyst | ||
{ | ||
IEnumerable<ModuleMetricsResult> ModuleMetrics(RubberduckParserState state); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, I'd remove the comment altogether. Or put in the "start a WW III" comment instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the throw discussed in #3563