lay295 / TwitchDownloader

Twitch VOD/Clip Downloader - Chat Download/Render/Replay
MIT License
2.68k stars 261 forks source link

Changed alot to coding best practices #1124

Closed MatthiasWeier closed 3 months ago

MatthiasWeier commented 3 months ago

used var get rid of unnecessary brackets using lambda expressions invert ifs Instead of "namespace{ }" use "namespace;" Didnt had the time to do everything manual.

ScrubN commented 3 months ago

get rid of unnecessary brackets using lambda expressions

This makes refactoring more difficult, no thanks

used var Instead of "namespace{ }" use "namespace;"

I have deliberately avoided doing this because it destroys the blame.

I appreciate the concern, but I'm familiar with how the codebase is right now and this will just make me need to relearn nearly everything. Thanks but no thanks.

ScrubN commented 3 months ago

Okay! I atleast suggest inverting ifs. Because that actually makes the code faster.

Inverting ifs doesn't change the AST of the code - it will compile exactly identically.

And more readable

I will use TimeDuration.Parse as an example.

public static TimeDuration Parse(string str)
{
    if (string.IsNullOrWhiteSpace(str))
    {
        throw new FormatException();
    }

    if (str.Contains(':'))
    {
        var timeSpan = TimeSpan.Parse(str);
        return new TimeDuration(timeSpan);
    }

    var multiplier = GetMultiplier(str, out var span);
    if (decimal.TryParse(span, NumberStyles.Number, null, out var result))
    {
        var ticks = (long)(result * multiplier);
        return new TimeDuration(ticks);
    }

    throw new FormatException();
}

In this code, the precedent is set that a new TimeDuration struct is returned by both of the parsing branches, and if that is not successful then an exception is thrown.

public static TimeDuration Parse(string str)
{
    if (string.IsNullOrWhiteSpace(str))
        throw new FormatException();

    if (str.Contains(':'))
    {
        var timeSpan = TimeSpan.Parse(str);
        return new TimeDuration(timeSpan);
    }

    var multiplier = GetMultiplier(str, out var span);
    if (!decimal.TryParse(span, NumberStyles.Number, null, out var result))
        throw new FormatException();

    var ticks = (long)(result * multiplier);
    return new TimeDuration(ticks);
}

When you inverted the last if, you not only broke that precedent but also made it more difficult to add more parsing conditions in the future.