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

[WIP] Applying best practice #871

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

K4PS3
Copy link

@K4PS3 K4PS3 commented Oct 7, 2023

This pull request mainly focused on organizing the core project.
You will find descriptive message in each commit.

Here is the main points:

  • Moved files in Caliburn.Micro.Core project to sub-folders.
  • The folders named by features instead of technical terms.
  • Each .cs file contains single type and named by that type.
  • Default code refactoring used to clean the code.
  • Arrow expression used whenever possible to reduce code line and make it more readable.
  • Long lines was splitted to be more clear for functionality and readability.
  • Return Early Pattern and Fail Fast Principle (martinfowler).
  • Code Analysis used to enable well-known rules for coding styles and best practice.
  • StyleCop used to enable extra rules.

In general the changes made here not critical for the code itself, most of the changes to benefit from the compiler capability, in Roslyn words most of the changes applied to the Trivias.

The missing part that found in <NoWarn>...</NoWarn> in .csproj of the core project, These errors needs to be fixed one-by-one with extensive testing and good consideration for the effects on others parts of the framework.

I tried to do small changes for each commit to make it easier to review. I also worked on the platform projects, but they need a lot in attention, especially with Conditional compilation.

If this pull request is acceptable, or any part of it can provide any benfit, i will be glade.

I am not native English speaker, so forgive any typos.

The error list

CA1711 // Identifiers should not have incorrect suffix

- AsyncEventHandler.cs
-INotifyPropertyChangedEx.cs

Cause: EventHandler and Ex suffix.

CA1003 // Use generic event handler instances

  • SimpleContainer.cs -> event Action<object> Activated
  • IActivate.cs -> event AsyncEventHandler<ActivationEventArgs> Activated
  • IDeactivate.cs -> event AsyncEventHandler<DeactivationEventArgs> Deactivated

Cause: Action instead of EventHandler, custome delegate AsyncEventHandler for EventHandler.
Solution: use EventHandler delegate for Action,

CA1070 // Do not declare event fields as virtual

  • Screen.cs -> public virtual event AsyncEventHandler<ActivationEventArgs> Activated
  • Screen.cs -> public virtual event EventHandler<DeactivationEventArgs> AttemptingDeactivation
  • Screen.cs -> public virtual event AsyncEventHandler<DeactivationEventArgs> Deactivated
  • ConductorBase.cs -> public virtual event EventHandler<ActivationProcessedEventArgs> ActivationProcessed
  • PropertyChangedBase.cs -> public virtual event PropertyChangedEventHandler PropertyChanged

Cause: virtual keyword, events should not be overrding.
Solution: Remove virtual keyword.

CA1052 // Static holder types should be Static or NotInheritable

  • ConductorWithCollectionAllActive.cs -> Collection::AllActive
  • ConductorWithCollectionOneActive.cs -> Collection::OneActive

CA1034 // Nested types should not be visible

  • ConductorWithCollectionAllActive.cs -> Collection::AllActive
  • ConductorWithCollectionOneActive.cs -> Collection::OneActive

Cause: Nesting public class.
Solution: move class to new namespace

namespace Caliburn.Micro.Conductor.Collection {
  public class AllActive<T> : ConductorBase<T> {
  }
}
namespace Caliburn.Micro.Conductor.Collection {
  public class OneActive<T> : ConductorBase<T> {
  }
}

CA1062 // Validate arguments of public methods

Cause: arguments used without validation.
Solution: validate arguments of methods, when failed, return, return null or throw.

throw new ArgumentNullException(nameof(argumentName))
throw new ArgumentException(message, nameof(argumentName))

CA2007 // Consider calling ConfigureAwait on the awaited task

This is critical issue.

read here and decide when to use ConfigureAwait(false) or ConfigureAwait(true)

CA1033 // Interface methods should be callable by child types

  • ViewAware.cs -> void IViewAware.AttachView(object view, object context)
  • Screen.cs -> async Task IActivate.ActivateAsync(CancellationToken cancellationToken)
  • Screen.cs -> async Task IDeactivate.DeactivateAsync(bool close, CancellationToken cancellationToken)

Cause: Explicit implementation of the interface methods.
Solution:

  • Add public method with same signature (Will not cause breaking change).
  • Make class sealed (Breaking change).
  • Make the implementation public instead of Explicit.

CA1031 // Do not catch general exception types

This is critical issue.

Cause: catch (Exception ex).
Solution: Catch Specific exception type, or rethrow the exception.

CA1716 // Identifiers should not match keywords

  • ILog.cs -> void Error(...)
  • PropertyChangedBase.cs -> public virtual bool Set<T>(...)

Cause: using of reserved keyword (Error and Set).
Solution: Change the name if possible, otherwise suppress this error.

CA2008 // Do not create tasks without passing a TaskScheduler

This is critical issue.

  • DefaultPlatformProvider.cs ->
   public virtual Task OnUIThreadAsync(Func<Task> action)
     => Task.Factory.StartNew(action);

Cause: new task without specifying TaskScheduler.
Solution: read the docs and decide.

CA1849 // Call async methods when in an async method

  • ResultExtensions.cs -> result.Execute(context ?? new CoroutineExecutionContext())

Cause: There is method named 'ExecuteAsync'.
Solution: ignore this error for this case, because calling ExecuteAsync will cause endless loop.

CA1822 // Mark members as static
CA1812 // Avoid uninstantiated internal classes

  • SimpleContainer.cs -> FactoryFactory<T>.Create

Cause: Pure Function not accessing any instance data.
Solution: Can't change this since it is used by GetInstance method with reflection, or you have to change the logic of calling this method by reflection.

The hierarchy of root folders as following:

* Framework main types:
-----------------------
- Screen
- ViewAware
- Conductor
- Coroutine
- Result
- EventAggregator

* Framework services:
---------------------
- PlatformProvider
- IoC
- Logging

* Extended System Types
- Types
Types shared across The parts of the framework.

- Extensions
Extension Methods for system types.

Inside each folder there maybe one or more of the following folders:
- Contracts: The interfaces.
- Base: abstract base implementation of the module.
- DefaultImpl: Default implementation that can be overriding by Platform.
- Impl: Multiple implementation of the contracts that can choosing from.
- Models: POCO used as argument for methods.
- Extensions: Extension methods for Contracts and main module file.
- ExtensionPoints: Service as Static holder that can be opt-in.
- EventArgs: POCO for events
- EventHandlers: Event Handlers for modules.
- Moved contents of TaskExtensions.cs to ResultExtensions.cs, both dealing with IResult.
- Deleted TaskExtensions.cs.

* Note
Nested types still exist in:
- WeakValueDictionary contains ValueCollection as private type (OK).
- LogManager conatins NullLog as private type [Null Pattern] (OK).
- SimpleContainer contains (ContainerEntry and FactoryFactory<T>) as private type (OK).
- EventAggregator contains Handler as private type (OK).
- Conductor partial class contains Collection as partial class which in turn contains (OneActive and AllActive) (Code Smell), We Should use namespaces.

if we were enabling Code Analyzers right now with SDK 7.0.401, we will get CA1052 error among other errors.
change global.json SDK version to 7.0.401 and add these properties to the project:
<PropertyGroup>
  <WarningLevel>7</WarningLevel>
  <TreatWarningsAsErrors>True</TreatWarningsAsErrors>
  <EnforceCodeStyleInBuild>True</EnforceCodeStyleInBuild>
  <EnableNETAnalyzers>True</EnableNETAnalyzers>
  <AnalysisLevel>latest-all</AnalysisLevel>
</PropertyGroup>
- Code Refactoring applied for :

* Use expression body for constructor.
* Use expression body for methods.
* Use expression body for property.
* Use local function.
* Use pattern matching
* Inline variable declaration.
* Inline temporary variable.
* Fixing IDE1006 Naming rule violation: Missing prefix:'_'.
* Fixing IDE0008 Use explicit type instead of 'var'.
* Fixing IDE0059 Unnecessary assignment of value, Use discard
* Fixing IDE0003 Name can be simplified, Remove 'this' qualification.
* Fixing IDE0016 Null check can be simplified, Use 'throw' expression.
* Fixing IDE0034 'default' expression can be simplified.
* Fixing IDE0090 'new' expression can be simplified.
* Fixing IDE0074 Use compound assignment.
* Fixing IDE0044 Make field readonly.
- Splitted long lines for easy read.

- Use expression body for lambda expression.
- Applied 'Fail Fast Principle' as much as possible.

- Minor formatting.

The Benefit is the code less nested, The 'Happy path' can hold more code without unnecessary nested complex method, because the 'Return Early Pattern' and 'Fail Fast Principle' paths has deterministic result for known conditions.
You can activate Code Analysis by setting 'SetupCodeAnalysis' to true in the project file '.csproj' and change SDK version in global.json to 7.0.400, build and you will get punch of errors, you suppress them by setting 'SuppressCodeAnalysisWarnings' to true.

The list of errors in the .csproj file need to be fixed next, one by one, we remove item from NoWarn, fix the code, build, test and refactor.
…ill there and need attention, some of theme is critical.

CA1711 // Identifiers should not have incorrect suffix
- AsyncEventHandler.cs
- INotifyPropertyChangedEx.cs
Cause: EventHandler and Ex suffix.

CA1003 // Use generic event handler instances
- SimpleContainer.cs -> event Action<object> Activated
- IActivate.cs -> event AsyncEventHandler<ActivationEventArgs> Activated
- IDeactivate.cs -> event AsyncEventHandler<DeactivationEventArgs> Deactivated
Cause: Action instead of EventHandler, custome delegate AsyncEventHandler for EventHandler.
Solution: use EventHandler delegate for Action<object>,

CA1070 // Do not declare event fields as virtual
- Screen.cs -> public virtual event AsyncEventHandler<ActivationEventArgs> Activated
- Screen.cs -> public virtual event EventHandler<DeactivationEventArgs> AttemptingDeactivation
- Screen.cs -> public virtual event AsyncEventHandler<DeactivationEventArgs> Deactivated
- ConductorBase.cs -> public virtual event EventHandler<ActivationProcessedEventArgs> ActivationProcessed
- PropertyChangedBase.cs -> public virtual event PropertyChangedEventHandler PropertyChanged
Cause: virtual keyword, events should not be overrding.
Solution: Remove virtual keyword.

CA1052 // Static holder types should be Static or NotInheritable
- ConductorWithCollectionAllActive.cs -> Collection::AllActive
- ConductorWithCollectionOneActive.cs -> Collection::OneActive
CA1034 // Nested types should not be visible
- ConductorWithCollectionAllActive.cs -> Collection::AllActive
- ConductorWithCollectionOneActive.cs -> Collection::OneActive
Cause: Nesting public class.
Solution: move class to new namespace
namespace Caliburn.Micro.Conductor.Collection {
  public class AllActive<T> : ConductorBase<T> {
  }
}
namespace Caliburn.Micro.Conductor.Collection {
  public class OneActive<T> : ConductorBase<T> {
  }
}

CA1062 // Validate arguments of public methods
Cause: arguments used without validation.
Solution: validate arguments of methods, when failed, return, return null or throw.
- throw new ArgumentNullException(nameof(argumentName))
- throw new ArgumentException(message, nameof(argumentName))

CA2007 // Consider calling ConfigureAwait on the awaited task
This is critical issue.
read here and decide when to use ConfigureAwait(false) or ConfigureAwait(true)
https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2007

CA1033 // Interface methods should be callable by child types
- ViewAware.cs -> void IViewAware.AttachView(object view, object context)
- Screen.cs -> async Task IActivate.ActivateAsync(CancellationToken cancellationToken)
- Screen.cs -> async Task IDeactivate.DeactivateAsync(bool close, CancellationToken cancellationToken)
Cause: Explicit implementation of the interface methods.
Solution:
- Add public method with same signature (Will not cause breaking change).
- Make class sealed (Breaking change).
- Make the implementation public instead of Explicit.

CA1031 // Do not catch general exception types
This is critical issue.
Cause: catch (Exception ex).
Solution: Catch Specific exception type, or rethrow the exception.

CA1716 // Identifiers should not match keywords
- ILog.cs -> void Error(...)
- PropertyChangedBase.cs ->  public virtual bool Set<T>(...)
Cause: using of reserved keyword (Error and Set).
Solution: Change the name if possible, otherwise suppress this error.

CA2008 // Do not create tasks without passing a TaskScheduler
This is critical issue.
- DefaultPlatformProvider.cs ->
    public virtual Task OnUIThreadAsync(Func<Task> action)
      => Task.Factory.StartNew(action);
Cause: new task without specifying TaskScheduler.
Solution: read the docs and decid.

CA1849 // Call async methods when in an async method
- ResultExtensions.cs -> result.Execute(context ?? new CoroutineExecutionContext())
Cause: There is method named 'ExecuteAsync'.
Solution: ignore this error for this case, because calling ExecuteAsync will cause endless loop.

CA1822 // Mark members as static
CA1812 // Avoid uninstantiated internal classes
- SimpleContainer.cs -> FactoryFactory<T>.Create
Cause: Pure Function not accessing any instance data.
Solution: Can't change this since it is used by GetInstance method with reflection, or you have to change the logic of calling this method by reflection.
- Added StyleCop package.
- Added StyleCop configuration file 'stylecop.json'.
- Applied StyleCop rules, Methods, Properties, Events and Indexers ordered according to these rules.
- Removed empty xml documentation.
- Added missing curly braces.
- .editorconfig changed to enforce specific coding style, these changes can be rolled-back and StyleCop will enforce the new changes.

By all these changes you should now have well structured project, organized by features, each feature folder contains all necessary files.

Coding style enforced by dotnet analyzers and StyleCop, The rules is configurable via .editorconfig and stylecop.json files.

But there still a lot of improvements that can be applied to the code itself by using new capabilities of the compiler, for example using record and struct record.

The important final note, the warnings in (<NoWarn>...</NoWarn>) have to be resolved, they are critical and change the framework behaviors.
@vb2ae
Copy link
Member

vb2ae commented Oct 7, 2023

Thank you for your pr.

@vb2ae
Copy link
Member

vb2ae commented Oct 7, 2023

If you merge in the latest from main it should fix the expired cert issue error when building the features and setup project

@vb2ae
Copy link
Member

vb2ae commented Oct 8, 2023

The code looks good. I will do a full review when you fix the issue with the build.

Thank you for adding the missing xml comments.
I do like the new folder structure for the code it does make it easier to find the classes.
Thank you for splitting each class into its own file.

Adding code analysis to the solution is great. I hope it catches most the issues that CodeQl found.

Question when we create the Caliburn.Micro nuget packages do the 2 code analysis packages get added as a dependency?

- Added standard text for missing xml comments.
- The code Refactored according to the configured analyzers.
- Added comments to '.editorconfig' to explain the changed diagnostic from silent or suggestion to warning.
- All the projects now build with all analyzers enabled including StyleCop.
- The tests passing.
- There is still issues needs to be resolved, for now they suppressed in 'Directory.Build.props', You will find note there with all errors.
- Test files in Caliburn.Micro.Core.Tests are moved to sub folder and extracted new files, so files contains single type.

There still a lot places that can be improved.
@K4PS3
Copy link
Author

K4PS3 commented Oct 9, 2023

First of all, sorry for the late response,
You are welcome and I hope that my contribution will be useful.

I just pushed commit that i am Confident is building locally with MSBuild without any issues, so The CI should now success.

About your Question, the answer is NO, StyleCop package is Meta package referencing, which in turn referencing stylecop.analyzers.unstable, this package does not contain any runtime files in LIB folder, so there will not be any referencing to it at runtime, the package only contains analyzers.

I glad that you liked changes i made.

By the way, I have been and still do using your framework successfully in many projects, with conjunction with SimpleInjector.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

- Changed the wrong format of the string 'NameFormat' in the test prohject.
- Merged nested 'if' statements.
- Used LINQ to avoid temporary capturing and mutating 'for' variable.
- Removed unnecessary casting.
- Changed the equality comparison from '==' to 'Equals'.

Of course 'Generic catch clause' errors needs to be fixed carefully and take note that the fix will cause Breaking changes for the framework.
@K4PS3
Copy link
Author

K4PS3 commented Oct 9, 2023

@vb2ae I just pushed another commit and hope it will fix CodeQL issues.
The issues are already exist and not introduced by this pr.
The 'Generic catch clause' issues still exist and needs manual fix by the framework team.

You can review the code, and give me feedback, so i can fix any issues you find this pr.

alerts

@K4PS3
Copy link
Author

K4PS3 commented Oct 9, 2023

Sorry, I forgot to update NameFormat in ViewModelLocatorTests.cs, the issue should be now fixed.
CodeQL should be now green with 17 notes about 'Generic catch clause'.

alert2

@vb2ae
Copy link
Member

vb2ae commented Oct 9, 2023

thanks for fixing the codeql errors

Copy link
Member

@vb2ae vb2ae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good.

@K4PS3
Copy link
Author

K4PS3 commented Oct 11, 2023

@vb2ae If you don't mind, I attached ZIP file Caliburn.Micro.git.zip with proposed changes for '.csproj' to make them more organized.

You can test them locally with 'master' branch of Caliburn.

As you see in the image, the active target framework will be shown in the root of the project, that will make it easier to recognize it.
The shared XAML files will shown in its own folder.
The shared code of maui shown in Maui folder in the Caliburn.Micro.Maui project, the active target shown in its own folder.

With this, as you can see, the root of the projects less distraction, even with project's nodes expanded.

These changes could be merged with this pr by another commit, and I can move the files from Caliburn.Micro.Platform to the other projects by another commit, so it will be easier to review the changes.

The power of MSBuild can make '.csproj' content smaller, I learned some tricks from the giants of MSBuild, and used here to clean '.csproj' files.

I hope that i am not mixing the things out of the order, and not breaking any rules or guides.

Looking forward for your review and comments.

folders

@vb2ae
Copy link
Member

vb2ae commented Oct 11, 2023

@K4PS3 thanks

… fixed.

- Tested locally and working correctly.

The code now more readable and clear in its purpose.
@KasperSK
Copy link
Member

I am intrigued about the .csproj changes looks like they could simplify maintenance of the platform projects by a lot.

…eException, handler is null).

I reviewed all the changes related to (someVariable.Equals(someValue)), all of them should be ok.
@vb2ae
Copy link
Member

vb2ae commented Oct 21, 2023

@KasperSK are you ok with merging this?

@KasperSK
Copy link
Member

Let me look it over I am not sure that I like the rearranging of the files. I have some time Monday before a flight so I will do it then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants