Consistency Cleanup about modix HOT 10 CLOSED

We need to be more consistent about HttpClient usage as well - refactoring the entire codebase to use asp.net core's IHttpClientFactory via DI would be ideal.

from modix.

Also, we should add an Environment.Exit() call on Disconnect events so Docker can cleanly restart the container when we lose our connection to Discord (server issues and such).

from modix.

I'm going to take care of cleaning up the code after feature work is done -- including re-writing the "bot host" code.

from modix.

I'd just like to add my two cents as to some of the conventions already that we could discuss.

Interfaces being in separate files

I find it a waste of time having interfaces that have one concrete implementation placed in separate files. It makes sense if the interface is an another class library or has multiple implementations, however if the interface has one implementation, it makes it both faster and easier to read and modify both interface and implementation.

My suggestion is, if the interface isn't in a separate library and has one implementation, to place interfaces above their concrete implementations, in the same file.

(I said the word implementation a lot there)

Suffixing Async to methods

When there is no synchronous counterpart (or other synchronous methods), I find no need for us suffix Async methods - it's an old convention that has served its purpose back when async and await was introduced into the language. The compiler (and us as developers) know better.

My suggestion is, as long as there is no synchronous counterpart, which there rarely is in this bot (if it all), methods should not be suffixed with Async.

Used of EF/Data annotations

Indifferent about this, but I'd like to see a convention being settled on. I typically only decorate with [Required] and [Key].

from modix.

Interfaces being in separate files
I can get behind this logic.

Suffixing Async to methods
I dunno, even with Async I screw up and forget to await stuff rather often. In my mind, adding an Async suffix is a hint from the designer that the method SHOULD probably be awaited, since the compiler does not consistently provide this warning for us.

We could actually extend this by adding a Roslyn analysis rule that throws the warning for ALL methods with an Async suffix. If you think this is going a little too deep, I agree, but the alternative is that issues where an await is missing are a BITCH to debug, and it'd be silly to just throw the warning onto EVERYTHING that returns Task.

Used of EF/Data annotations
Not sure what to make of this. Are you saying that stuff other than just [Required] and [Key] should be done in OnModelCreating instead of through attributes? Cause in our current code, pretty much every attribute that's there and isn't [Required] or [Key] is important. Like, remove it and the app will break important.

If it's just a question of attribute vs procedural code, I feel like if there's value in doing SOME of the model definitions through attributes, that same argument applies to doing ALL of it through attributes (at least, as much as possible, EF doesn't provide attributes for quite everything).

from modix.

I propose that we use Roslyn's .editorconfig as a starting point and open new issues for adjustments. Seems like a good standard to go off of. Corefx's would also be fine but they don't like the use of var unless the type is obvious.

from modix.

Looks fine to me. Does this come along with some kind of Roslyn plugin that evaluates the rules?

from modix.

VS2017 (and even ReSharper) should respect the repos .editorconfig without requiring any additional extensions.

from modix.

Partial work in progress via #188

from modix.

I think we're probably good to close this out - if anyone disagrees, we can reopen, but anything else could be done on a issue-by-issue basis.

from modix.