jbogard / ContosoUniversityCore

MIT License
590 stars 150 forks source link

Putting AutoMapper profile into a class with a handler #19

Closed siberianguy closed 7 years ago

siberianguy commented 7 years ago

As we use a specific Command/Query class for each handler, AutoMapper configurations are not reused by multiple handlers. And if AutoMapper configuration is specific to a handler, why don't we store it in the same file (like, Command, Profile, Handler)?

I feel a little uncomfortable about the indirect nature of AutoMapper as when I look at the handler's code and I'm not sure what's going on. Are there any complex mappings? Anything significant besides dumb mappings? I feel like moving the mapping configuration into the file with handler and command/query would make it a little bit more clear.

Why do we store all the mappings for a feature together? Do they share any common logic? Doesn't seem to be the case. In a way, I feel like grouping all the mappings together is a violation of the idea of grouping things by feature/function/etc. instead of grouping by type (i. e. all the mappings go together).

I'm debating myself about it in my current project, was curious what you Jimmy think about it.

jbogard commented 7 years ago

Hmmm that's interesting, you certainly could split by requests instead of overall feature. I like it!

But I had no other reason why I had a single profile per feature folder.

On Mon, Sep 11, 2017 at 10:51 AM, Alexander Sidorov < notifications@github.com> wrote:

As we use a specific Command/Query class for each handler, AutoMapper configurations are not reused by multiple handlers. And if AutoMapper configuration is specific to a handler, why don't we store it in the same file (like, Command, Profile, Handler)?

I feel a little uncomfortable about the indirect nature of AutoMapper as when I look at the handler's code and I'm not sure what's going on. Are there any complex mappings? Anything significant besides dumb mappings? I feel like moving the mapping configuration into the file with handler and command/query would make it a little bit more clear.

Why do we store all the mappings for a feature together? Do they share any common logic? Doesn't seem to be the case. In a way, I feel like grouping all the mappings together is a violation of the idea of grouping things by feature/function/etc. instead of grouping by type (i. e. all the mappings go together).

I'm debating myself about it in my current project, was curious what you Jimmy think about it.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jbogard/ContosoUniversityCore/issues/19, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGYMmVhTYTsgRD05ZyyuyhWDu6y9uTzks5shVcVgaJpZM4PTYMa .

siberianguy commented 7 years ago

What if we go even further and not create a Profile class (it does seem like an overkill for simple scenarios) and just put the AutoMapper code somewhere in the handler class (i. e. constructor or even the handler itself)?

jbogard commented 7 years ago

What would that look like? You've got some limitations that it's static configuration (similar to validators). I think you'd probably still have to create a class for a profile at some point.

On Mon, Sep 11, 2017 at 11:32 AM, Alexander Sidorov < notifications@github.com> wrote:

What if we go even further and not create a Profile class (it does seem like an overkill for simple scenarios) and just put the AutoMapper code somewhere in the handler class (i. e. constructor or even the handler itself)?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/jbogard/ContosoUniversityCore/issues/19#issuecomment-328584929, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGYMmMNQEkPp3xBA8R9Jc50kxU-D213ks5shWC4gaJpZM4PTYMa .

siberianguy commented 7 years ago

I thought there was a way to dynamically add mappings using static registration. I wanted to put this code into handler's constructor hoping AutoMapper would smartly register the mapping only once. Never checked it, so based on your answer I assume it's not possible (and it would be a little dirty anyway).

Adding a Profile class is not a big deal but it feels a little silly as we generate more lines of code to use the tool that is supposed to save those lines of code (it would look especially ridiculous for one-line mappers). And I realise AutoMapper is more than a tool to save lines of code, but still...