rust-unofficial / patterns

A catalogue of Rust design patterns, anti-patterns and idioms
https://rust-unofficial.github.io/patterns/
Mozilla Public License 2.0
7.85k stars 354 forks source link

Command pattern #247

Closed fade2black closed 3 years ago

fade2black commented 3 years ago

WIP

Fixes #248 Fixes #228

fade2black commented 3 years ago

@simonsan ready for review

pickfire commented 3 years ago

Do we only have dynamic dispatch examples?

fade2black commented 3 years ago

@pickfire Do you mean by examples real examples like actix? In my code the second approach with function pointers doesn't use dynamic dispatch. As for real life examples, I haven't yet come across. If you know please share.

pickfire commented 3 years ago

Not sure if like actix but I only see dyn xxx stuff, we didn't show a method with static dispatch, which probably could be done using enum.

simonsan commented 3 years ago

@pickfire I would merge it for now or do you want to add an example with static dispatch?

fade2black commented 3 years ago

@simonsan I'll add a couple of sentences as you suggested. I'll have a look this evening (in 5 hours).

simonsan commented 3 years ago

@simonsan I'll add a couple of sentences as you suggested. I'll have a look this evening (in 5 hours).

ok ;)

pickfire commented 3 years ago

@pickfire I would merge it for now or do you want to add an example with static dispatch?

Up to you, I am lazy to do that currently. One way it could be done is to use enum handle multiple cases rather than a Box.

simonsan commented 3 years ago

@pickfire I would merge it for now or do you want to add an example with static dispatch?

Up to you, I am lazy to do that currently. One way it could be done is to use enum handle multiple cases rather than a Box.

If we don't do it in this PR we can definitely reference that in a new issue for other people to add. :)

simonsan commented 3 years ago

Looks fine for me 👍🏽 Let's wait till @pickfire and @MarcoIeni also gave their feedback and we're ready to merge. :)

fade2black commented 3 years ago

@simonsan @pickfire I agree. Enum is a one way to address the delegation (to commands). I think enums should be added too. I might add it too a bit later. However, there is also a simple trade off between dyn traits and enums. If delegation occurs externally (crates for example), then trait objects would suit better, since we don't know in advance what kind of type a user may come up with. Like in actix. But if we are going to use it in our own application then since we control the enum variants and if add new variants not too frequently, we could use enum.

simonsan commented 3 years ago

@simonsan @pickfire I agree. Enum is a one way to address the delegation (to commands). I think enums should be added too. I might add it too a bit later. However, there is also a simple trade off between dyn traits and enums. If delegation occurs externally (crates for example), then trait objects would suit better, since we don't know in advance what kind of type a user may come up with. Like in actix. But if we are going to use it in our own application then since we control the enum variants and if add new variants not too frequently, we could use enum.

Sure, I think at this stage it would be better to add it later in another PR, when more people had the chance to read over it after publishing and maybe also work in other feedback then, what do you think?

fade2black commented 3 years ago

@simonsan @pickfire I agree. Enum is a one way to address the delegation (to commands). I think enums should be added too. I might add it too a bit later. However, there is also a simple trade off between dyn traits and enums. If delegation occurs externally (crates for example), then trait objects would suit better, since we don't know in advance what kind of type a user may come up with. Like in actix. But if we are going to use it in our own application then since we control the enum variants and if add new variants not too frequently, we could use enum.

Sure, I think at this stage it would be better to add it later in another PR, when more people had the chance to read over it after publishing and maybe also work in other feedback then, what do you think?

I agree.

fade2black commented 3 years ago

@MarcoIeni Removed liftetimes from function ptrs.

simonsan commented 3 years ago

Thanks :)