Skip to main content
added 19 characters in body
Source Link

Some things I noticed:

IHook has an event handler called MessageReceived that takes type HookEventArgs. That's 2 very generically-named types and one very specific property name. I can't infer usages from the code provided, but a preliminary smell test tells me IHook and HookEventArgs need to be named more specifically, or MessageReceived is too specific of a name for the event. I can't really put my finger on what exactly it is, but it's kind of hard to follow what is supposed to be going on because the only thing correlating a hook with messaging behavior is a property name.

A hook that implements IHook that has a collection of IHooks and a method taking in a THook constrained to type IHook. So maybe the interface can benefit from covariance/contravariance with thea type parameter and an in or out keywordskeyword, but it could also be that something is using inheritance where it doesn't need to be, too.

The code appears to be inching towards "an interface for every class" territory in some spots. IRubberDuckHooks may not be providing much use outside of unit testing; it obviously depends on the context, but sometimes it's ideal to simply make a method virtual over creating a new interface.

You may remember a past conversation where I expressed my feelings about dictionaries providing a means for easy mapping. If not, you might be able to guess them :) I don't really think they're a huge problem in and of themselves, but they can lead to fragile code since things that might be known at compile time are being evaluated at runtime (leading to possible misses that could be caught via compilation errors). It's one of those things where I'd probably do a quick gut check if I found myself frequently relying on them. In this case, I don't think there's too much risk. However, it does lead back to an interesting question: Why are we dealing with a string representation of a key at all? It may be that at some point, the only thing you've gotten back from an API is a string, but there could also be some intermediary custom "serialization" going on, in which case I would generally say that there's probably a better way to solve the problem that's currently being solved by representing a strongly-typed object with a string.

Whether or not IDisposable should be exposed on an interface versus its implementing type is subject to debate--it can be argued that having an interface implement it creates a leaky abstraction since there's really nothing present around how and what resources are getting released. In my personal experience, I frequently find that it's serving the purpose of // DON'T FORGET TO DISPOSE THIS!! in the code when really something needs to be fixed or moved elsewhere.

(Take this with a grain of salt) There is a lot of code here, and while it helps to provide context, a lot of this peripheral code is actually raising more questions as I go through it which makes me want to see even more context. Although it can be time-consuming, sometimes posting less code and following a line of questioning can be quite productive.

Some things I noticed:

IHook has an event handler called MessageReceived that takes type HookEventArgs. That's 2 very generically-named types and one very specific property name. I can't infer usages from the code provided, but a preliminary smell test tells me IHook and HookEventArgs need to be named more specifically, or MessageReceived is too specific of a name for the event. I can't really put my finger on what exactly it is, but it's kind of hard to follow what is supposed to be going on because the only thing correlating a hook with messaging behavior is a property name.

A hook that implements IHook that has a collection of IHooks and a method taking in a THook constrained to type IHook. So maybe the interface can benefit from covariance/contravariance with the in or out keywords, but it could also be that something is using inheritance where it doesn't need to be, too.

The code appears to be inching towards "an interface for every class" territory in some spots. IRubberDuckHooks may not be providing much use outside of unit testing; it obviously depends on the context, but sometimes it's ideal to simply make a method virtual over creating a new interface.

You may remember a past conversation where I expressed my feelings about dictionaries providing a means for easy mapping. If not, you might be able to guess them :) I don't really think they're a huge problem in and of themselves, but they can lead to fragile code since things that might be known at compile time are being evaluated at runtime (leading to possible misses that could be caught via compilation errors). It's one of those things where I'd probably do a quick gut check if I found myself frequently relying on them. In this case, I don't think there's too much risk. However, it does lead back to an interesting question: Why are we dealing with a string representation of a key at all? It may be that at some point, the only thing you've gotten back from an API is a string, but there could also be some intermediary custom "serialization" going on, in which case I would generally say that there's probably a better way to solve the problem that's currently being solved by representing a strongly-typed object with a string.

Whether or not IDisposable should be exposed on an interface versus its implementing type is subject to debate--it can be argued that having an interface implement it creates a leaky abstraction since there's really nothing present around how and what resources are getting released. In my personal experience, I frequently find that it's serving the purpose of // DON'T FORGET TO DISPOSE THIS!! in the code when really something needs to be fixed or moved elsewhere.

(Take this with a grain of salt) There is a lot of code here, and while it helps to provide context, a lot of this peripheral code is actually raising more questions as I go through it which makes me want to see even more context. Although it can be time-consuming, sometimes posting less code and following a line of questioning can be quite productive.

Some things I noticed:

IHook has an event handler called MessageReceived that takes type HookEventArgs. That's 2 very generically-named types and one very specific property name. I can't infer usages from the code provided, but a preliminary smell test tells me IHook and HookEventArgs need to be named more specifically, or MessageReceived is too specific of a name for the event. I can't really put my finger on what exactly it is, but it's kind of hard to follow what is supposed to be going on because the only thing correlating a hook with messaging behavior is a property name.

A hook that implements IHook that has a collection of IHooks and a method taking in a THook constrained to type IHook. So maybe the interface can benefit from covariance/contravariance with a type parameter and an in or out keyword, but it could also be that something is using inheritance where it doesn't need to be, too.

The code appears to be inching towards "an interface for every class" territory in some spots. IRubberDuckHooks may not be providing much use outside of unit testing; it obviously depends on the context, but sometimes it's ideal to simply make a method virtual over creating a new interface.

You may remember a past conversation where I expressed my feelings about dictionaries providing a means for easy mapping. If not, you might be able to guess them :) I don't really think they're a huge problem in and of themselves, but they can lead to fragile code since things that might be known at compile time are being evaluated at runtime (leading to possible misses that could be caught via compilation errors). It's one of those things where I'd probably do a quick gut check if I found myself frequently relying on them. In this case, I don't think there's too much risk. However, it does lead back to an interesting question: Why are we dealing with a string representation of a key at all? It may be that at some point, the only thing you've gotten back from an API is a string, but there could also be some intermediary custom "serialization" going on, in which case I would generally say that there's probably a better way to solve the problem that's currently being solved by representing a strongly-typed object with a string.

Whether or not IDisposable should be exposed on an interface versus its implementing type is subject to debate--it can be argued that having an interface implement it creates a leaky abstraction since there's really nothing present around how and what resources are getting released. In my personal experience, I frequently find that it's serving the purpose of // DON'T FORGET TO DISPOSE THIS!! in the code when really something needs to be fixed or moved elsewhere.

(Take this with a grain of salt) There is a lot of code here, and while it helps to provide context, a lot of this peripheral code is actually raising more questions as I go through it which makes me want to see even more context. Although it can be time-consuming, sometimes posting less code and following a line of questioning can be quite productive.

Source Link

Some things I noticed:

IHook has an event handler called MessageReceived that takes type HookEventArgs. That's 2 very generically-named types and one very specific property name. I can't infer usages from the code provided, but a preliminary smell test tells me IHook and HookEventArgs need to be named more specifically, or MessageReceived is too specific of a name for the event. I can't really put my finger on what exactly it is, but it's kind of hard to follow what is supposed to be going on because the only thing correlating a hook with messaging behavior is a property name.

A hook that implements IHook that has a collection of IHooks and a method taking in a THook constrained to type IHook. So maybe the interface can benefit from covariance/contravariance with the in or out keywords, but it could also be that something is using inheritance where it doesn't need to be, too.

The code appears to be inching towards "an interface for every class" territory in some spots. IRubberDuckHooks may not be providing much use outside of unit testing; it obviously depends on the context, but sometimes it's ideal to simply make a method virtual over creating a new interface.

You may remember a past conversation where I expressed my feelings about dictionaries providing a means for easy mapping. If not, you might be able to guess them :) I don't really think they're a huge problem in and of themselves, but they can lead to fragile code since things that might be known at compile time are being evaluated at runtime (leading to possible misses that could be caught via compilation errors). It's one of those things where I'd probably do a quick gut check if I found myself frequently relying on them. In this case, I don't think there's too much risk. However, it does lead back to an interesting question: Why are we dealing with a string representation of a key at all? It may be that at some point, the only thing you've gotten back from an API is a string, but there could also be some intermediary custom "serialization" going on, in which case I would generally say that there's probably a better way to solve the problem that's currently being solved by representing a strongly-typed object with a string.

Whether or not IDisposable should be exposed on an interface versus its implementing type is subject to debate--it can be argued that having an interface implement it creates a leaky abstraction since there's really nothing present around how and what resources are getting released. In my personal experience, I frequently find that it's serving the purpose of // DON'T FORGET TO DISPOSE THIS!! in the code when really something needs to be fixed or moved elsewhere.

(Take this with a grain of salt) There is a lot of code here, and while it helps to provide context, a lot of this peripheral code is actually raising more questions as I go through it which makes me want to see even more context. Although it can be time-consuming, sometimes posting less code and following a line of questioning can be quite productive.