OSLoader QA - Githubissues

BoettcherDasOriginal commented 4 months ago

Code Base

Entrypoint.cs ✔ Loader.cs ✔ LoaderConfig.cs ✔ Logger.cs ✔ -> ⚠Type 1 ModReference.cs ⚠ -> ⚠ Type 2 Mod.cs ✔ ModInfo.cs ✔ ModSettingAttribute.cs ✔ ModSetting.cs ✔ LoaderUI.cs ✔ -> ⚠Type 1 + (maybe move the ')' in line 59 under scrollbarValue like you do it with '{}')

⚠ Types

private variables which don't follow c# naming guidelines -> private var _name // Good private var name // Bad (You dont have to change this, but it adds readability)
absolute c# no-go , using var instead of default variable (string in this case) (You dont have to change this, but yeah)

💡 Ideas

Logger.cs -> maybe add a Log.Warning() function?
Mod.cs & ModInfo.cs -> maybe add a load function for modders to use?

Result

I would say u did really good work mate😀 Except from these minor things mentioned above you have a really good clean code base and i am currently happy with the feature set🥇

Actual Usage

Notes

The logger for a mod is pretty good when using it in a mod class but outside of it you need to setup a reference to it and i personaly would just use Debug.Log() myself instead

Could be improved by adding static logger functions like this:

public static void Log(string name, string message)
{
string log = $"[{name}] {message}";
if (logToLoaderLog)
{
    File.AppendAllText(loaderFileFilepath, log + "\n");
}

if (Loader.Instance.ModloaderInitialized)
{
    Debug.Log(log);
}
}

It is pretty nasty to create a JSON by hand and have manually copy the attributes to it
- maybe provide example JSON files in the documentation later on or/and add them to the TestMod repo

QA on going...

BoettcherDasOriginal commented 4 months ago

Better ) placement

// LoaderUI.cs
// Line 47-64

private void MainMenuWindowCallback(int id)
{
    int height = (int)mainMenuRect.height;
    int width = (int)mainMenuRect.width;

    int modsCount = Loader.Instance.mods.Count;
    int maximumModsToDisplay = (height - titleSpacing) / modEntryHeight;

    if (modsCount > maximumModsToDisplay + 1)
    {
        scrollbarValue = (int)GUI.VerticalScrollbar(
            new Rect(edgeWidth + (int)mainMenuRect.width - scrollBarDistance, edgeWidth + titleSpacing, 15, height - titleSpacing - 20),
            scrollbarValue, maximumModsToDisplay, 0, modsCount
        ); // Better 
    }
    else
    {
        scrollbarValue = 0;
    }

    // . . .
}

Ierdna100 commented 2 months ago

I'm going to reopen this to remind myself to check all of these, and do a final pass where I try to follow the naming conventions that I was previously unaware of

Ierdna100 / OSLoader