Discussion: Establishing MonoGame.Extended Coding Conventions

[AristurtleDev]

Background

After taking over lead of the MonoGame.Extended project, I've been working on updates in focused areas. During this time, I've discovered inconsistencies in the codebase, particularly around code duplication and naming conventions.

For example

• HslColor.FromRgb and ColorExtensions.ToHsl perform idential funtionality
• Extension methods for HslColor exist in ColorExtensions rather than a dedicated HslColorExtensions clss.
• There are multiple approaches to conversions between types (From method, To extension methods, and helper classes).

These inconsistencies make the codebase harder to maintain and contribute to. Moving forward, I believe we need to establish clear coding conventions that will guide future development.

Proposed Approach

I have drafted an initial set of coding guidelines to use. Some of these are based on general .NET best practices for libraries, others are more targeted since this is a library meant to be used specifically with MonoGame. These guidelines cover:

1. Naming conventions for types and members
2. Extension method organization
3. Standardized approaches to conversion methods (From vs To)
4. Helper class usage and organization.
5. Code structure and documentation requirements.

and much more.

Questions for discussion

1. Namespace Organization: I have proposed that we should mirror MonoGame's namespace structure (e.g. graphics classes in MonoGame.Extended.Graphics corresponding to MonoGame's Microsoft.Xna.Framework.Graphics). Does this make sense for all components, or are there exceptions we should consider.

2. Conversion Methods: Should we standardize on Type.From() static methods, x.ToType() extension methods, or both? What naming patterns make the most sense and are more intuitive?

3. Extension Methods: Should we enforce strictly one extension class per extended type. For example, ColorExtensions should only extend the Color struct type and other color spaces, like the HslColor struct type, it should be in a HslColorExtensions class?

4. Helper Classes: What criteria should determine when to use a helper class vs extension methods vs instance method?

5. Additional Areas: What other areas of inconsistency should be addressed?

Next Steps

1. Review the guidelines in PR #987
2. Provide feedback and suggestions.
3. Once we have a consensus, I'll finalize the guidelines document.

[lithiumtoast]

Should we standardize on Type.From() static methods, x.ToType() extension methods, or both?

Please take a look at Microsoft's extended Rosyln analyzers. Specifically CA2225 for From and To when using implicit or explicit cast operators. It should give you some ideas to chew on. https://learn.microsoft.com/en-ca/dotnet/fundamentals/code-analysis/quality-rules/ca2225.

[lithiumtoast]

Extension methods are a tool to be used but often used incorrectly IMO. Having a bunch of extension classes can make the code harder to follow when reading directly the source code. If we have control over the source code directly (i.e. it's not from MonoGame directly but part of MonoGame.Extended source code) it makes more sense to move the logic directly into the type (e.g. TypeA.StaticMethodX or typeAInstance.MethodX) or into a class that operates on the type (e.g. a Renderer or Converter class).

[AristurtleDev]

Extension methods are a tool to be used but often used incorrectly IMO. Having a bunch of extension classes can make the code harder to follow when reading directly the source code. If we have control over the source code directly (i.e. it's not from MonoGame directly but part of MonoGame.Extended source code) it makes more sense to move the logic directly into the type (e.g. TypeA.StaticMethodX or typeAInstance.MethodX) or into a class that operates on the type (e.g. a Renderer or Converter class).

I can get behind this. I would rather the logic be contained within the struct/class itself where we have control of the source.

If I'm reading it correctly your suggesting to either to static methods like:

public struct HslColor
{
    public static Color ToRgb(HslColor hsl)
    {
        // implementation
    }
}

or instance methods like

public struct HslColor
{
    public Color ToRgb()
    {
        // implementation
    }
}

Would there be a downside to having both, where the static method is just a forward call to the instance method?

[lithiumtoast]

Static methods are the norm I seen. Calling a method on a struct can have the possibility (depends on the context of how it is written, declared, and used) to create a defensive copy of the struct. See https://www.developmentsimplyput.com/post/defensive-copy-in-net-c

[AristurtleDev]

Static methods are the norm I seen. Calling a method on a struct can have the possibility (depends on the context of how it is written, declared, and used) to create a defensive copy of the struct. See https://www.developmentsimplyput.com/post/defensive-copy-in-net-c

Thanks for the source read. The implementation of HSLColor that's coming does mark the struct and all respective methods a readonly (with the exception of a copyref method using UnSafe.As but that's a discussion for that PR).

I like the static method approach as it is more explicit when reading the code as to what's happening, though I know these days everyone wants everything as minimal characters typed as possible.

[lithiumtoast]

I would also mention that in the context of Brute and NativeAOT, static methods are the norm in MonoGame for Vector2 and so on. I would not be too worried about the name being too short or too long for code as everyone should be running a C# LSP (Language Server Protocol) tool anyways right? It should come with autocomplete.

[AristurtleDev]

Updated the guidelines on preferring static methods within the class/struct in eb1d02d