Skip to content
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

Merged
merged 28 commits into from Nov 28, 2017
Merged

Code Metrics #3522

merged 28 commits into from Nov 28, 2017

Conversation

@Vogel612
Copy link
Member

@Vogel612 Vogel612 commented Oct 28, 2017

See #403

Current Status of the UI:

Code Metrics dockable presenter

The columns resize together.

@Vogel612 Vogel612 force-pushed the Vogel612:code-metrics branch 2 times, most recently from 18e856e to 47824c8 Nov 3, 2017
@Vogel612 Vogel612 force-pushed the Vogel612:code-metrics branch from 65bce47 to 7fcdd7e Nov 21, 2017
Vogel612 added 3 commits Nov 21, 2017
…ke CodeMetricsAnalyst internal"

This reverts commit 43fab4e.
Apparently internal implementations for interfaces are not discovered by
Castle Windsor
@Vogel612 Vogel612 changed the title Code Metrics (WIP - Backend finished) Code Metrics Nov 24, 2017
Copy link
Contributor

@bclothier bclothier left a comment

Small suggestions, notable are throw which could crash the runtime; LTGM otherwise.

@@ -221,7 +221,9 @@ private void Startup()
catch (Exception e)
{
_logger.Log(LogLevel.Fatal, e, "Startup sequence threw an unexpected exception.");
//throw; // <<~ uncomment to crash the process
#if DEBUG
throw; // <<~ uncomment to crash the process

This comment has been minimized.

@bclothier

bclothier Nov 24, 2017
Contributor

IMO, I'd remove the comment altogether. Or put in the "start a WW III" comment instead.

This comment has been minimized.

@Vogel612

Vogel612 Nov 24, 2017
Author Member

That's the throw discussed in #3563


private ModuleMetricsResult GetModuleResult(QualifiedModuleName qmn, IParseTree moduleTree, DeclarationFinder declarationFinder)
{
// Consider rewrite as visitor? That should make subtrees easier and allow us to expand metrics

This comment has been minimized.

@bclothier

bclothier Nov 24, 2017
Contributor

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.

This comment has been minimized.

@Vogel612

Vogel612 Nov 24, 2017
Author Member

sure can be. Just check a CancellationToken when entering any rule. Bit of a pain though, so this usually isn't done...


namespace Rubberduck.Navigation.CodeMetrics
{
public class CodeMetricsViewModel : ViewModelBase

This comment has been minimized.

@bclothier

bclothier Nov 24, 2017
Contributor

The class should implement IDisposable and the disposing pattern. That way you can deregister events from state cleanly.


public void FilterByName(object projects, string text)
{
throw new NotImplementedException();

This comment has been minimized.

@bclothier

bclothier Nov 24, 2017
Contributor

Should it really be here?

}


//public CommandBase RefreshCommand { get; set; }

This comment has been minimized.

@bclothier

bclothier Nov 24, 2017
Contributor

Commented code shouldn't be here, either.

_vbe = vbe;
_declarationFinderFactory = declarationFinderFactory;
_vbe = vbe ?? throw new ArgumentNullException(nameof(vbe));
_declarationFinderFactory = declarationFinderFactory ?? throw new ArgumentNullException(nameof(declarationFinderFactory));

This comment has been minimized.

@bclothier

bclothier Nov 24, 2017
Contributor

Should we be crashing runtime like that?

This comment has been minimized.

@Vogel612

Vogel612 Nov 24, 2017
Author Member

this is just c#7-ification of existing behaviour...

This comment has been minimized.

@MDoerner

MDoerner Nov 24, 2017
Contributor

I really do not like the change here. This way, it is rather hard to see the guard clauses; it no longer screams: "I want non-null arguments!"

This comment has been minimized.

@Vogel612

Vogel612 Nov 25, 2017
Author Member

@MDoerner to me that seems to just be a matter of getting used to it. I can see benfits of both approaches. The one is significantly more explicit, the other conserves screen real-estate...

{
public class CodeMetricsAnalyst : ICodeMetricsAnalyst
{
private readonly IIndenterSettings indenterSettings;

This comment has been minimized.

@MDoerner

MDoerner Nov 24, 2017
Contributor

The usual convention in RD would be _indenterSettings.

}

var trees = state.ParseTrees;
var results = new List<CodeMetricsResult>();

This comment has been minimized.

@MDoerner

MDoerner Nov 24, 2017
Contributor

This does not seem to be used anywhere.


namespace Rubberduck.UI.Converters
{
public class SubractionConverter : IValueConverter

This comment has been minimized.

@MDoerner

MDoerner Nov 24, 2017
Contributor

Is this a typo?

This comment has been minimized.

@Vogel612

Vogel612 Nov 24, 2017
Author Member

yes, it is. keen eye

This comment has been minimized.

@MDoerner

MDoerner Nov 25, 2017
Contributor

The typo is still present in the filename.

{
public struct CodeMetricsResult
{
public CodeMetricsResult(int lines, int cyclomaticComplexity, int nesting)

This comment has been minimized.

@MDoerner

MDoerner Nov 25, 2017
Contributor

Why are we hard-coding the types of code metrics we support?

This comment has been minimized.

@Vogel612

Vogel612 Nov 25, 2017
Author Member

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.

}


private class CodeMetricsListener : VBAParserBaseListener

This comment has been minimized.

@MDoerner

MDoerner Nov 25, 2017
Contributor

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?

This comment has been minimized.

@Vogel612

Vogel612 Nov 25, 2017
Author Member

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 List<CodeMetricsResult> results = new List<CodeMetricsResult>();
private List<CodeMetricsResult> moduleResults = new List<CodeMetricsResult>();

private List<MemberMetricsResult> memberResults = new List<MemberMetricsResult>();

This comment has been minimized.

@MDoerner

MDoerner Nov 25, 2017
Contributor

Since the variables above are all instance variables, they should start with an underscore.

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();

This comment has been minimized.

@MDoerner

MDoerner Nov 25, 2017
Contributor

I think it would be more appropriate to use UserDeclarations here. DeclarationsWithType is the concatenation of BuiltInDeclarations with UserDeclarations.

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();

This comment has been minimized.

@MDoerner

MDoerner Nov 25, 2017
Contributor

As above

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();

This comment has been minimized.

@MDoerner

MDoerner Nov 25, 2017
Contributor

dito

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();

This comment has been minimized.

@MDoerner

MDoerner Nov 25, 2017
Contributor

Dito

private readonly ICodeMetricsAnalyst _analyst;

public CodeMetricsViewModel(RubberduckParserState state, ICodeMetricsAnalyst analyst)

This comment has been minimized.

@Hosch250

Hosch250 Nov 26, 2017
Member

Not sure what this is doing :P

_state.StateChanged -= OnStateChanged;
}


This comment has been minimized.

@Hosch250

Hosch250 Nov 26, 2017
Member

More extra spaces.

: base(command)
{
}
public override bool EvaluateCanExecute(RubberduckParserState state) => true;

This comment has been minimized.

@Hosch250

Hosch250 Nov 26, 2017
Member

Pretty sure true is the default state.

This comment has been minimized.

@Vogel612

Vogel612 Nov 26, 2017
Author Member

it isn't. The default state is state != null && (_command?.CanExecute(state) ?? false);. Since that's all irrelevant for showing a docked window it's overriden here

public object Convert(object value, Type targetType, object parameter, CultureInfo culture)
{
var typedValue = (double)value;
if (!Double.TryParse((string)parameter, out var typedParam))

This comment has been minimized.

@Hosch250

Hosch250 Nov 26, 2017
Member

Could this possibly be simplified to:

if (parameter is double typedParam)
{
    return typedValue - typedParam;
}

return typedValue;

This comment has been minimized.

@Vogel612

Vogel612 Nov 26, 2017
Author Member

no, since parameter is a string, accordingly we need to try-parse

This comment has been minimized.

@retailcoder retailcoder merged commit a4d6173 into rubberduck-vba:next Nov 28, 2017
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@Vogel612 Vogel612 deleted the Vogel612:code-metrics branch Dec 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.