First project review

code review
1

Hi there,

I am new in the Rust and I have done the first attempt to write my own library managing a dependency injection in Rust (inspired by legacy version of the Dependency Injector library from the Python https://python-dependency-injector.ets-labs.org/).

Here is my repo: GitHub - elmordo/jector: Just another Rust dependency injector

Can anyone please review my code and give me advice (coding style, programming approach, ...) how improve it? My primary languages are Python, TypeScript and C++ so I guess there are some "bad habits" I bring from those languages.

Thanks a lot :slight_smile:

2

Feels like an OO-pattern written up in Rust IMO.

Some things seem to assume interior mutability (if you want to be dynamic) while others don't. When you do use interior mutability (RefCell) and shared ownership (Rc), it's single-threaded.

One specific thing I note is that you have these all over:

impl SequenceWithParam {
    pub fn boxed() -> Box<SequenceWithParam> {
        Box::new(SequenceWithParam::default())
    }
}

impl<C, V, P> Factory<C, V, P> {
    /// Create new instance wrapped in the Box
    pub fn boxed(factory: Box<dyn ValueFactory<C, V, P>>) -> Box<Self> {
        Box::new(Self::new(factory))
    }
}

There's no real reason to just box up these structs. Boxes are mainly used for

  • Unsized things (which must be behind a pointer) which you own, like a Box<[u32]>
  • Type-erased trait objects which you own, like Box<dyn FnMut()>
    • As they are also unsized
  • Making large structs take up less space by boxing large substructs fields
    • This is the only thiing I could think of for a sized X where a Box<X> is preferable to an X

You can avoid unwrap here.

     fn get(&self, container: &C, params: &P) -> V {
-        if self.provider_stack.is_empty() {
-            panic!("There is no provider on the stack")
-        }
-        let p = self.provider_stack.last().unwrap();
+        if let Some(p) = self.provider_stack.last() {
             p.get(container, params)
+        } else {
+            panic!("There is no provider on the stack")
+        }
     }
2 Likes
3

Hi, thank you for your review! :slight_smile:

About your post:

Feels like an OO-pattern written up in Rust IMO.

Yes, it is correct. Is this wrong? It could be one of those "bad habits" I brought from other languages.

Some things seem to assume interior mutability (if you want to be dynamic) while others don't.

The ProviderStack is special case and it is designed to be changed (inner providers stack push/pop) only when you have mutable reference of the provider.

When you do use interior mutability (RefCell) and shared ownership (Rc), it's single-threaded.

Good point, I didn't realized it. Thank you :slight_smile:

About the boxed method: This is meant as shorthand for usage with the ProviderStack

let provider_stack = ProviderStack::new(
  Factory::boxed(SomeValueFactory::boxed())
);

instead of

let provider_stack = ProviderStack::new(
  Box::new(
    Factory::new(
      Box::new(SomeValueFactory::new())
    )
  )
);

The reason of the is to avoid too many Box::new calls written by hand.

You can avoid unwrap here.

This is much better than my implementation. :slight_smile:

Your review was very helpful :slight_smile: I have only one question: can you explain that point about OO-patterns little more?

Tank you very much, and happy new year :partying_face:

4

This form of dependency injection is the object oriented form of the larger concept of inversion of control. The "rustier" (more functional) form would be to supply a closure when needed.

5

I don't know that I would call it wrong, but I don't know that I would call it idiomatic either. Some would consider it a warning sign (#6).

It does feel simultaneously too abstract and too restricted, a bit like a concrete abstraction I guess. In the example for instance, I just need a constant but I pull in a framework and write my own traits on top of that, and then invoke the framework by passing a bunch of ignored parameters in? If I need a constant, especially if it can be cloned, just... hand me the constant? Perhaps the example (or myself) is to simplistic to grasp why I'd use it.

On the restricted front, it seems to make these unclear-to-me assumptions, like there was some goal in sight I didn't know about. To produce values, I need to take two arguments, one of which is presumably a container (but has no inate abilities) and another is presumably parameters. However, I can only take shared references to those. I'm not sure why there's two inputs here and why one is presumed to be a shared reference to a container, or what you're imagining I'll do with it. A pre-anticipated third layer of abstraction where the ValueFactory uses the parameter to maybe get something out of the container? Why does the container need to be separate from the ValueFactory itself?

Apologies if any of that comes across as a put-down, it's not meant to be. I'm just trying to illustrate how I sort of stumbled around the code trying to figure out what the idea was so I could provide feedback. I think this indirectly gets at your OO-pattern question: You're presumably working from paradigm where you can look at "oh a factory of value factories that operates on a borrowed container that takes parameters" and have everything click, but I don't think I've really seen such a thing in idiomatic Rust.

Feels like you're taking Box<dyn Trait> in some places you could or should take T: Trait, e.g. perhaps

 impl<C, V, P> Factory<C, V, P> {
     /// Make new instance of the Factory struct
-    pub fn new(factory: Box<dyn ValueFactory<C, V, P>>) -> Self {
+    pub fn new<F: ValueFactory<C, V, P>>(factory: F) -> Self {
+        let factory = Box::new(factory);
         Factory { factory }
     }
 }

Or perhaps just

pub trait ValueFactory<C, V, P> {
    fn into_dyn(self) -> Box<dyn ValueFactory<C, V, P>> {
        Box::new(self)
    }
}

Or in general, if you're only going to box concrete types in order to immediately type-erase them into a Box<dyn Trait> elsewhere, might as well return the type-erased form instead.

Something I should have noted the first time: If you do need multi-threaded capability, you'll probably need all your trait objects to be Box<dyn Trait + Send + Sync> too.

1 Like
6

I read the link you sent me and the number 6.a and 6.c describes my code really accurate.

On the restricted front, it seems to make these unclear-to-me assumptions, like there was some goal in sight I didn't know about. To produce values, I need to take two arguments, one of which is presumably a container (but has no inate abilities) and another is presumably parameters. However, I can only take shared references to those. I'm not sure why there's two inputs here and why one is presumed to be a shared reference to a container, or what you're imagining I'll do with it. A pre-anticipated third layer of abstraction where the ValueFactory uses the parameter to maybe get something out of the container? Why does the container need to be separate from the ValueFactory itself?

The main idea of the lib is: you have a container where all providers are stored and this container implements special traits which say "this container provide this service (or value)" (ConstantProvider in demo).

This container is propagated into ValueFactory (ok, InstanceFactory could be better name for the trait :smiley: ) of some service. This value, returned from the ValueFactory could be anything, the constant (like in example) or some complex object, like connection to database. The container is provided to the ValueFactory because some service needs other service for its construction. For example mentioned database connection may need some configuration and this where the container comes in play - it provide service's dependencies into ValueFactory.

So purpose of layers are:

  1. Container - top level object providing services to rest of app and also provide dependencies to providers
  2. Providers - manage instance creation strategies like Factory or Singleton (and the ProviderStack which is wrapper for other providers)
  3. Value factories - manage the instance creation logic

According to information I get from the How not to learn Rust and @erelde comment, more rustic way is to remove the first layer (container) and the third layer (value factories) and manage providers as closures instead of structs and use generics instead of dynamics?

Hmm, maybe this type of library is little big goal for the newbie's first project in the Rust :smiley:

Apologies if any of that comes across as a put-down, it's not meant to be. I'm just trying to illustrate how I sort of stumbled around the code trying to figure out what the idea was so I could provide feedback. I think this indirectly gets at your OO-pattern question: You're presumably working from paradigm where you can look at "oh a factory of value factories that operates on a borrowed container that takes parameters" and have everything click, but I don't think I've really seen such a thing in idiomatic Rust.

It's ok. If I don't want to hear critic, I won't to ask for review :upside_down_face:

7

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.