Glebshpuryk/1505

[ShpurykGleb]

1505

[ShpurykGleb]

@VadymLevkovskyi

Hello, Vadym.

We had a disscusion with Dmytro about Admin controller. He said, that you had a dialog with him about that a few weeks ago. As he said, you decided not to implement CQRS for this task, but also Dmytro said, that i can try this, maybe you will change your thoughts about this.

So, I have some questions and requests for you.

• Please, check the code, if it is ok in general, I will write tests for it and also will register new services, etc.
• Project has several interfaces, that was in use for Admin controller only: ISensitiveApplicationService, ISensitiveMinistryAdminService, ISensitiveDirectionService, ISensitiveApplicationService

So, do we need to extract they into new Admin project or not?

If yes, I have an idea for this, but i am not sure, that she is good

We need to create a two shared projects

Firstly, for this services we create a "AdminShared" project, because we need it in .Admin and in .BusinessLogic. Secondly, we create "BusinessLogicShared" project, because we need some models from .BusinessLogic to use in .AdminShared and .Admin

But, as I said, I am not sure about this, I think, it can be like overengeneering. so, I like to hear your thoughts about this, if it is not hard for you

• Do we need to extract a controller also to .Admin project? Or it will be in WebApi project, like now?

I think, for the moment, it is all questions and requests that I have. Thank in advance!

[VadymLevkovskyi]

Hi Gleb!

... decided not to implement CQRS for this task

... and reasons were next:

• we have to perform deep refactoring of our code and db design to get CQRS benefits;
• CQRS has steeper learning curve and has more chances to be implemented wrongly (more on this later);
• it can't be done partially, so there is no good workaround to make it i.e. for Admin part only;
• it is harder to maintain (debug, change code, onboard new dev).

So, I have some questions and requests for you.

• Please, check the code, if it is ok in general, I will write tests for it and also will register new services, etc.

Sorry, but your implementation has some flaws:

• main one from pattern standpoint - commands shouldn't return anything;
• main one from code refactoring standpoint - extra code is written, but what was changed? your commands/queries are just wrappers over services, and you additionally get issues related to dynamic linking via MediatR (that is slower and harder to debug).

So, my main question is - show me benefits of this approach, please.

• Project has several interfaces, that was in use for Admin controller only: ISensitiveApplicationService, ISensitiveMinistryAdminService, ISensitiveDirectionService, ISensitiveApplicationService

So, do we need to extract they into new Admin project or not?

As I can see from #1505 description it was asked to extract them.

We need to create a two shared projects ... "AdminShared" and "BusinessLogicShared" project

Yes, at first glance for me they look like overengineered, bad named projects. Usually, something "Shared" is a cohesion-introducing thing, that is bad. Usually it is solved by introducing domain-local version of model . At same time I wish to know what is planned to be refactored. Ideally to have it in form of two diagrams: "as is" and "to be", where dependencies between projects and moved classes are shown, to see what you'd like to move and how.

• Do we need to extract a controller also to .Admin project? Or it will be in WebApi project, like now?

Again, as I see from #1505 description it was asked to extract them.

---

I'll have extra discussion on this PR with Dmytro, probably will add some info later

[ShpurykGleb]

Hi Gleb!

... decided not to implement CQRS for this task

... and reasons were next:

• we have to perform deep refactoring of our code and db design to get CQRS benefits;
• CQRS has steeper learning curve and has more chances to be implemented wrongly (more on this later);
• it can't be done partially, so there is no good workaround to make it i.e. for Admin part only;
• it is harder to maintain (debug, change code, onboard new dev).

So, I have some questions and requests for you.

• Please, check the code, if it is ok in general, I will write tests for it and also will register new services, etc.

Sorry, but your implementation has some flaws:

• main one from pattern standpoint - commands shouldn't return anything;
• main one from code refactoring standpoint - extra code is written, but what was changed? your commands/queries are just wrappers over services, and you additionally get issues related to dynamic linking via MediatR (that is slower and harder to debug).

So, my main question is - show me benefits of this approach, please.

• Project has several interfaces, that was in use for Admin controller only: ISensitiveApplicationService, ISensitiveMinistryAdminService, ISensitiveDirectionService, ISensitiveApplicationService

So, do we need to extract they into new Admin project or not?

As I can see from #1505 description it was asked to extract them.

We need to create a two shared projects ... "AdminShared" and "BusinessLogicShared" project

Yes, at first glance for me they look like overengineered, bad named projects. Usually, something "Shared" is a cohesion-introducing thing, that is bad. Usually it is solved by introducing domain-local version of model . At same time I wish to know what is planned to be refactored. Ideally to have it in form of two diagrams: "as is" and "to be", where dependencies between projects and moved classes are shown, to see what you'd like to move and how.

• Do we need to extract a controller also to .Admin project? Or it will be in WebApi project, like now?

Again, as I see from #1505 description it was asked to extract them.

I'll have extra discussion on this PR with Dmytro, probably will add some info later

Hello, Vadym! Thank you for your critic!

I have some questions for you:

• "...and async/await for Handle() that (partially) hurts performance", please, can you explain, why async/await hurts perfomance? Is it related to .ConfigureAwait(false)? Or can you give some info/articles where I can read about that?

• After how long you will have a call with Dymtro about that +-? Because, it is my only task in the work at the moment, and without info I will be downtime.

• What we need to return in commands? Unit or void, something like that? I used to think, that command can return values like "bool, Result, etc.", because commands is related to create, update, delete, and we need to know after the command execution is it completed successfully or not.

• Can you explain your phrase "Don't insist on suggetsed changes."? I don't understand it fully. It is like an phrase to me: "Gleb, dont insist on your changes." or like from your person: "I dont insist on my suggested changes. (code review)". Because, I don't insist on my changes, it is like an example at all, maybe, I wrote something wrong in my previous comment, that confused you, so, sorry for that.

Some answers for your questions and requests:

• CQRS was started to implemeting in our project on Admin part, like an example to rewrite an another parts. At previous discussion with Dmytro about task, I said: "I can try implement the CQRS on Admin part, if it will be ok to Vadym, we can start to rewrite our project on it", Dmytro said: "You can try, but, Vadym last weeks said, that we don't need that, maybe, he will change his opinion about that. So, you are welcome to it." And, as you can see, I try it))

• Will rewrite the IsTechAdmin() method to extension

New thoughts about task:

We can create an Admin and AdminCommon projects. In Admin project we will have our controller, etc, in AdminCommon we will have our 4 services for work, because, they needed in controller, and they also needed in BusinessLogic (we can not create a circle references for projects and also it is bad if it possibly, as i know). And models, that are in use at the services, we will move to Common project, that we already have. As I can see Common project is related to such situations, when we need our models in many projects. I can create a diagram for you, as you wrote, if you want it - write about that in next comment, please.

That is all for the moment. I am waiting for your thoughts, have a good day!