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

Some suggestions on the code #3

Open
snixtho opened this issue Feb 22, 2023 · 0 comments
Open

Some suggestions on the code #3

snixtho opened this issue Feb 22, 2023 · 0 comments

Comments

@snixtho
Copy link
Member

snixtho commented Feb 22, 2023

Looking over the code, here is what I think should be done in terms of cleaning up the code.

Refer to these docs for framework design guidelines, this is the common practice for all .NET libraries: https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/?redirectedfrom=MSDN

  • Improve the naming. For example, don't use generic names for public-facing classes. For example "Component" is way too generic and it doesn't describe well what it is and can cause conflicts. A better name would be "ManialinkComponent" or "MlComponent" etc. Pretty much all classes under Lib/ has this issue.
  • Add doc comments
  • Improve namespace naming. Instead of just putting everything under Lib/ categorize them better, like Components/, Utils/, Templates/ etc.
  • Instead of using constructors, use property initializers with the new required keyword.
    For example: https://github.com/EvoTM/ManiaTemplates/blob/init/Lib/ComponentProperty.cs#L9
    a better and cleaner way to write this class would be:
public class ComponentProperty
{
    public required string Type { get; }
    public required string Name { get; }
    public string? Default { get; }
}
  • The Helper.cs file seems to contain things that are not needed for the lib itself, but I guess its just for testing anyways?
  • Tests (use XUnit) should be added
  • Put interfaces in it's own namespace, it allows for separation of the implementation.
  • In general, you want to avoid optional parameters in .NET when creating libraries, and instead make overloads for the methods. It increases compatibility between API changes in the future. This is only really needed for public methods though.
  • I would probably move the regexes out of the methods and make them a static variable in the class so that they are compiled once instead of every time you call a method.
  • Use StringBuilder when concatenating strings. I see some places where this should be used. For example this: https://github.com/EvoTM/ManiaTemplates/blob/init/Lib/ComponentMarkdownGenerator.cs#L14 StringBuilder is very efficient and should almost always be used for generating string output of some sort.
  • Surely this can be made better than hard-coding a bunch of Replace() calls: https://github.com/EvoTM/ManiaTemplates/blob/init/Lib/ManiaLink.cs#L44
  • You can make this cleaner with .ToString(): https://github.com/EvoTM/ManiaTemplates/blob/init/Lib/Snippet.cs#L55
    For example:
private string Indentation(int additionalIndentation) => 
((_currentIndentation + additionalIndentation) * IndentationMultiplier).ToString()
internal static bool UsesManialinkComponents(this XmlNode node, ComponentList components)
{
    foreach (XmlNode child in node.ChildNodes)
    {
        return UsesComponents(child, components);
    }

    return components.ContainsKey(node.Name);
}

Then you would just need to call it like node.UsesManialinkComponents(components)

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

No branches or pull requests

1 participant