Code quality review and other small things

[Draco18s]

https://github.com/silverua/slay-the-spire-map-in-unity/blob/f184b8ccf54ee051a1d2778ade7951a7a533419c/Assets/Scripts/MapGenerator.cs#L77

Why bother with both the node type and blueprint name, isn't a NodeBlueprint sufficient?

https://github.com/silverua/slay-the-spire-map-in-unity/blob/f184b8ccf54ee051a1d2778ade7951a7a533419c/Assets/Scripts/Point.cs#L5

Vector2Int exists. If it didn't when you made this, that's understandable, but Unity added integer vectors and it makes things so much nicer not having to rebuild such a basic struct.

https://github.com/silverua/slay-the-spire-map-in-unity/blob/f184b8ccf54ee051a1d2778ade7951a7a533419c/Assets/Scripts/MapGenerator.cs#L47 https://github.com/silverua/slay-the-spire-map-in-unity/blob/f184b8ccf54ee051a1d2778ade7951a7a533419c/Assets/Scripts/MapGenerator.cs#L76 https://github.com/silverua/slay-the-spire-map-in-unity/blob/f184b8ccf54ee051a1d2778ade7951a7a533419c/Assets/Scripts/ShufflingExtension.cs#L27

Re-type your extension to take in an IEnumerator<> instead of a list and then you don't need to allocate memory for a new list that is immediately discarded. If you're going to take advantage of Linq, then make your Linq extensions compatible with Linq.

https://github.com/silverua/slay-the-spire-map-in-unity/blob/f184b8ccf54ee051a1d2778ade7951a7a533419c/Assets/Scripts/ShufflingExtension.cs#L32

Ditto. Linq already has an IEnumerator.Last<>

https://github.com/silverua/slay-the-spire-map-in-unity/blob/f184b8ccf54ee051a1d2778ade7951a7a533419c/Assets/Scripts/MapView.cs#L341-L345 https://github.com/silverua/slay-the-spire-map-in-unity/blob/f184b8ccf54ee051a1d2778ade7951a7a533419c/Assets/Scripts/MapViewUI.cs#L114

Then you don't need this method either, as its only caller is the child class trying to get back the NodeBlueprint reference you already had and didn't pass to the Node constructor. I know you did this to make the class easy to serialize, but because you have the ability to lookup the NodeBlueprint reference by name, you can write a custom JsonConverter to do that during de/serialization; its pretty easy, example, albeit I need to rename it to Converter probably; lingering cruft from various refactors. And the bit that gets added to the JsonSettings, I'm using an attribute for my own classes, but the Unity classes I have to specify individually (and this might be why you made your own Point class (cough, should've been a struct and implemented the == operator), now that I think about it).

https://github.com/silverua/slay-the-spire-map-in-unity/blob/f184b8ccf54ee051a1d2778ade7951a7a533419c/Assets/Scripts/Node.cs#L15 The NodeType isn't even referenced by anything anyway.

https://github.com/silverua/slay-the-spire-map-in-unity/blob/f184b8ccf54ee051a1d2778ade7951a7a533419c/Assets/Scripts/MapConfig.cs#L21-L27

List<> is sufficient here. Unity will already make the inspector wrap this in a ReorderableList that allows it to be reordered.

https://github.com/silverua/slay-the-spire-map-in-unity/blob/f184b8ccf54ee051a1d2778ade7951a7a533419c/Assets/Scripts/MapGenerator.cs#L11

IEnumerable<MapNodeType> RandomNodes = Enum.GetValues(typeof(MapNodeType)).Cast<MapNodeType>().Where(v => v != MapNodeType.Boss); Bam. Make the code do that array for you. And not even bother making it a list, leave it as IEnumerable because of how you're using it:

https://github.com/silverua/slay-the-spire-map-in-unity/blob/f184b8ccf54ee051a1d2778ade7951a7a533419c/Assets/Scripts/MapGenerator.cs#L274-L277

Replace with RandomNodes.Random()

https://github.com/silverua/slay-the-spire-map-in-unity/blob/f184b8ccf54ee051a1d2778ade7951a7a533419c/Assets/Scripts/MapGenerator.cs#L187-L196

return config.GridWidth % 2 == 1 || Random.Range(0, 2) == 0
    ? new Vector2Int(config.GridWidth / 2, y)
    : new Vector2Int(config.GridWidth / 2 - 1, y);

Couple other methods could be shortened similarly (like GetNode above it)

https://github.com/silverua/slay-the-spire-map-in-unity/blob/f184b8ccf54ee051a1d2778ade7951a7a533419c/Assets/Scripts/MapGenerator.cs#L51-L56

This method can be written to cache the summed value in addition to the between value as such:

private static List<(float dist, float sum)> layerDistances = new List<(float, float)>();
//...
    private static void GenerateLayerDistances(MapConfig config)
    {
        float totalDistance = 0;
        layerDistances = new List<(float, float)>();
        foreach (MapLayer layer in config.layers)
        {
            float nextDist = layer.distanceFromPreviousLayer.GetValue();
            totalDistance += nextDist;
            layerDistances.Add((nextDist, totalDistance));
        }
    }

//usage:
// layerDistances[layerIndex].dist - what layerDistances[layerIndex] previously held
// layerDistances[layerIndex].sum - what GetDistanceToLayer() returned

Rendering GetDistanceToLayer() irrelevant and avoiding a repeated computation (layer x+1 will always be the total distance to x plus the spacing of x+1, so why compute x1, x2, x3, ..., x every time?). Anonymous tuples are fantastic.

https://github.com/silverua/slay-the-spire-map-in-unity/blob/f184b8ccf54ee051a1d2778ade7951a7a533419c/Assets/Scripts/MapGenerator.cs#L117-L118

Per Microsoft's documentation on the usage of var you should not be using var here.

Don't use var when the type isn't apparent from the right side of the assignment. Don't assume the type is clear from a method name. A variable type is considered clear if it's a new operator, an explicit cast or assignment to a literal value.

I've actually seen that mistake result in a bug where the return type was a Dictionary<,> of some kind and not a List<CustomObject> (which was being returned by a rest API call) and the developer did a foreach over the contents and was comparing a searchKey with the foreach_var.Key value because the CustomObject also had a Key and Value property. In a twist of irony I was fired for telling this developer (who was also the CEO) that I don't like var because people don't use it correctly during casual conversation. He then linked the above documentation and I had to remind him of the bug he'd created because he'd used var incorrectly. He didn't like admitting he was wrong about anything.

This one might be a bit on the pedantic side, but the official coding convention exists, and I'd like people to actually follow it if they're going to use var. I only ever use var to avoid typing out a Type name but always do a replace-with-explicit refactor afterwards ([v][a][r][ctrl][.] is fewer keystrokes than a lot of Type names).

https://github.com/silverua/slay-the-spire-map-in-unity/blob/f184b8ccf54ee051a1d2778ade7951a7a533419c/Assets/Scripts/MapGenerator.cs#L17

Paths is only used by GeneratePaths (as a return value) and SetUpConnections (as a consumer). We can remove this static object allocation that lives forever and inline everything down to... foreach (List<Vector2Int> path in GeneratePaths(config))

We can also make the config an object that's passed as a parameter to each function rather than storing a forever-reference to it as well as nodes. Which makes GetNode a little funky, but we can turn that into an extension method super easily.

https://github.com/silverua/slay-the-spire-map-in-unity/blob/f184b8ccf54ee051a1d2778ade7951a7a533419c/Assets/Scripts/Map.cs#L10 https://github.com/silverua/slay-the-spire-map-in-unity/blob/f184b8ccf54ee051a1d2778ade7951a7a533419c/Assets/Scripts/MapGenerator.cs#L48

Is there a reason to pass this new empty list to the constructor rather than creating inside the constructor? It's used to hold the player's path through the map, so it doesn't seem like it should be created externally (or be both public and not readonly).

Aaaand that gets to a full refactor (albeit with a few renames, fekking hate it when Visual Studio imports random shit because of class names like Node are used by dozens of packages).

[silverua]

Hi @Draco18s ! Thanks for such a detailed write-up! Some great points there. I will apply them to the repo in the coming weeks.

[silverua]

Is there a reason to pass this new empty list to the constructor rather than creating inside the constructor? It's used to hold the player's path through the map, so it doesn't seem like it should be created externally (or be both public and not readonly).

I think, I did it because of deserialization. Newtonsoft.Json would use the same constructor and I need it to deserialize paths correctly.

[Draco18s]

I think, I did it because of deserialization. Newtonsoft.Json would use the same constructor and I need it to deserialize paths correctly.

Makes sense. Weird, but makes sense. Give the custom converters a bash and see if it helps in that regard too.

[Draco18s]

One more https://github.com/silverua/slay-the-spire-map-in-unity/blob/f184b8ccf54ee051a1d2778ade7951a7a533419c/Assets/Scripts/MapGenerator.cs#L260-L261 This is capable of generating an Array Out Of Bounds Exception if all three checks from 121 to 139 fail and don't add anything to the list. Not sure if this helps, but I've gotten very consistent exceptions with this config.

[Draco18s]

https://github.com/silverua/slay-the-spire-map-in-unity/blob/f184b8ccf54ee051a1d2778ade7951a7a533419c/Assets/Scripts/MapGenerator.cs#L100-L104

Random.Range(-0.5f, 0.5f) and you can dispense with all of the /2s.

[Draco18s]

I put together a MapConfig inspector. It uses a custom list element drawer so that instead of "element 1, element 2," etc. it can display the NodeType so it's easy to glance at a collapsed list and see what you've got. As well as a button to conveniently just "find all my node type assets and fill in the blueprints array." Code I originally wrote to find all of the cards for my game, because they were going to be organized by folder and have a BUNCH of them. https://github.com/Draco18s/Bullethell-Refactor/blob/master/Assets/Editor/bulletboss/map/MapConfigInspector.cs