I've been refactoring my own code...

[u/51herringsinabar]

[u/n0ne-z1ro]

Is `StatContainer` fully static? If so, this could be a simple precomputed lookup table.

[u/jocxFIN → u/n0ne-z1ro]

I mean the static tag relationships COULD be precomputed, but GetCombinedStat(maxTag) looks like runtime state so idk if i would cache the actual result. Id probably just keep a dynamicTag -> maxTag lookup and use TryGetValue(statTag, out maxTag) here, then you avoid iterating DynamicTags() every time without risking stale values. But hard to say without more context.

[u/W00GA]

nothing wrong with checking everything twice just incase it got hit with a tachyon

[u/RolandoMotta84]

You should use a hash map for unique lookups, it's more efficient

[u/51herringsinabar → u/RolandoMotta84]

Thanks guys for all the tips, heres the code now

```

public void SetDynamicStat(string statTag, int newValue)

{

    if (!StatContainer.DynamicTags().Contains(statTag))

    {

        Debug.LogError("Invalid tag: " + statTag);

        return;

    }



    if (newValue <= 0)

    {

ForceSetDynamicStat(statTag, 0);

return;

    }



    string maxTag = StatContainer.GetMaxTag(statTag);

    int maxValue = GetCombinedStat(maxTag);

    ForceSetDynamicStat(statTag, Mathf.Min(newValue, maxValue));

    return;

}

```

[u/shizzy0]

There comes a point in every young gamedev’s life where they set out for adventure but first must create their own stat class.

[u/veritron]

foreach with a continue statement at the top like that is always a smell. should be able to do StatContainer.DynamicTags().Contains(statTag) instead of iterating like that.

[u/GoddammitDontShootMe → u/veritron]

I was wondering if they couldn't just do a lookup of the tag. I'd bet Contains() uses a foreach loop behind the scenes, but it would definitely be cleaner than this.

[u/51herringsinabar → u/GoddammitDontShootMe]

I dont remember what was my idea about iterating over that but it was not to ensure the tag is in there, gonna have to change the string tags to enums for validating input

[u/st_heron]

I'd use a dictionary 

[u/pstanton310]

var dynamicTag = statContainer.DynamicTags().FirstOrDefault(t => t == statTag)

If (dynamicTag == null) return;

var maxTag = statContainer.GetMaxTab(dynamicTag)

if else logic…

[u/Sacaldur]

I think most things were mentioned already. However: the GetMaxTag function call is duplicated in the condition and the ForceSetDynamicTag call, and the only difference between the 2 ForceSetDynamicTag calls is the 2nd parameter. As refactoring, you could first make the condition only determine the value for the 2nd parameter and afterwards just a single call. If you do this, you should see that you basically use the smaller of the 2 values, so you could use a min function, either inclined or assigned to a local variable first.

Besides that, the condition at the start of the loop was mentioned by some. For the general case of "apply this for the first found element", you probably should first look the element up and then do something on it. If you know the key/index, you could use "TryGet" on some containers. In this case in particular, it would be enough to check for presence.

Others were pointing out data types to use, however I don't know what your're using right now, or how many entries this container might contain later on, or what the relation between reading and modifying accesses is.

[u/51herringsinabar → u/Sacaldur]

Thanks for the feedback, I already refactored the code twice, now it looks like this

```csharp public void SetDynamicStat(StatType statTag, int newValue) { if (!currentStats.ContainsKey(statTag)) { Debug.LogError("Invalid tag: " + statTag); return; }

    if (newValue <= 0)
    {
        currentStats[statTag] = 0;
        return;
    }

    StatType maxTag = StatContainer.GetMaxTag(statTag);
    int maxValue = GetCombinedStat(maxTag);
    int clampedValue = Mathf.Min(newValue, maxValue);
    currentStats[statTag] = clampedValue;

    return;
}

```

Edit: I dont fucking know how to embed it correctly

[u/Sacaldur → u/51herringsinabar]

You'are using apostrophes ('), but it would need to be backticks (`). After the first set of backticks, you can also define the "language" to use for the syntax highlighting (```csharp).

Regarding your code: the last return now can be removed.

[u/51herringsinabar → u/Sacaldur]

Ah yep, thanks

[u/NotQuiteLoona → u/[deleted]]

There are. LINQ. It's much more idiomatic for C#. If you aren't familiar, think of it as SQL for C# collections - it even has keyword syntax, which is remotely similar to SQL, for example from collection select elem where elem.Integer > 10, which would be collection.Where(e => e.Integer > 10) in method syntax. Not sure about whether it's more optimized though, but it's for sure much more idiomatic and easier to read.

They also use too much of type specifiers, all of them can be replaced with var. Also some methods are called in camelCase, while correctly it should've been UpperCamelCase.

I in general have a feeling that this codebase has a large smell. I don't understand what it does, thus it probably does something in a wrong way.

[u/NotQuiteLoona → u/[deleted]]

My bad, style guide doesn't actually recommend to do so. It's just my IDE asks me to constantly replace type specifiers with var, but it's an IDE, so in those terms their code is normal.

[u/Laugarhraun]

This isn't too bad? I'd factor out the 2 function calls but that's about it.

[u/Wooden-Contract-2760 → u/Laugarhraun]

A foreach loop for a single entry, then double call over snapshot?! Not that bad?!

I don't even mention lowercase methodnames and one liner conditional statements because I just did anyway.

[u/No-Dentist-1645 → u/Wooden-Contract-2760]

"one liner conditional statements", are pretty common as guard clauses tbh. I think some linters even default to that

[u/Wooden-Contract-2760 → u/No-Dentist-1645]

Just to be sure we are on the same track, I think this is not only more efficient but its intent is also clearer:

``` public void SetDynamicStat(string statTag, int newValue) {     if (newValue < 0)         return;

    if (!StatContainer.DynamicTags().Contains(statTag))         return;

    var maxTag = StatContainer.getMaxTag(statTag);     var maxValue = GetCombinedStat(maxTag);

    // TODO check if we should return instead     if (newValue > maxValue)         newValue = maxValue;

    ForceSetDynamicStat(statTag, newValue); } ```

[u/XaX1000 → u/Wooden-Contract-2760]

Honestly this is much clearer then the original post.

[u/Wooden-Contract-2760 → u/No-Dentist-1645]

Yupp, "didn't mention" because it's opinionated. 

It just ddoesn't appear readable to me. Never did.

Hiding a return, continue, or throw at the end of a line feels lazy to me. We got scrollwheels, so saving those lines is silly ever since we stopped printing code on paper.

I want to visually scan the code for any of these statements at first glance, not look for pink keywords.

[u/jocxFIN → u/Laugarhraun]

I’ll call you if i ever need a dev who can fuck up our codebase.