upizs / PestControl

Self-made bug tracker for personal use. Implementing ASP.NET Core .NET 5 with razor pages, Entity Framework 5.0.9 and AspNetCore Identity 2.2.0
7 stars 0 forks source link

Remove Comment Manager. #1

Closed TopSwagCode closed 1 year ago

TopSwagCode commented 3 years ago

https://github.com/upizs/PestControl/blob/befa86b6cb961adae185a7430bda8c2b6406a45b/TicketControl.BLL/Managers/CommentManager.cs#L11

There is No logic.

If you dont feel like deleting it. Change methods to not be async. And returning directly instead of await. Your creating a lot of overhead.

zlangner commented 3 years ago

"Change methods to not be async. And returning directly instead of await. Your creating a lot of overhead."

This is false. It doesn't create a lot of overhead. The state machine that makes async work adds nanoseconds of overhead but helps the call stack have proper context when exceptions are thrown. Unless this is being called thousands of times per second the benefit of gives out weighs the insignificant overhead it adds.

This video does a good job of explaining this and 7 other common mistakes people make with async/await. https://youtu.be/lQu-eBIIh-w

Nobonex commented 3 years ago

If the function itself doesn't do anything but await an async method to then return the value, it does not have to be async. It may simply return the Task. This function itself should then be awaited higher up in the call chain. This does not create any problems with exceptions.

EDIT: To clarify what happens when an exception is thrown in the Task that you returned without awaiting.

Let's first look at the following example:

class Example
{
    private Task<int> Main()
    {
        return Foo();
    }

    private Task<int> Foo()
    {
        return Bar();
    }

    private Task<int> Bar()
    {
        return Task.FromResult(1);
    }
}

This is a very bad idea. It may run fine 99% of the time, but if there is an exception in Bar(), this will get lost. This can have unpredictable results, one of which crashing your entire app.

A better solution would be:

class Example
{
    private async Task<int> Main()
    {
        return await Foo();
    }

    private Task<int> Foo()
    {
        return Bar();
    }

    private Task<int> Bar()
    {
        return Task.FromResult(1);
    }
}

In this example, if an exception is thrown while executing the Task from Bar(), this exception would finally be thrown in the Main() function.

upizs commented 3 years ago

@TopSwagCode @Nobonex @zlangner Thank you for your feedback. I plan to justify Comment Manager and all other Managers by creating one Generic Repository. Let me know if you have opinion about that. Will value it.

Nobonex commented 2 years ago

Kinda forgot about this, sorry :)

One generic repository sounds like a good idea. At least, that's what I go with 99% of the time. And if there is ever need for more logic for a specific Entity then you can always make a new repository inheriting from the Generic one.

As a side note. All of the functions in the CommentManager are async, but none use a CancellationToken. You should really look into that, but I'll make a separate issue for this :)