Please critique this code

[Bender-Rodriguez]

I'd love to get feedback from more experienced guys on what I'm doing here.

Background: I want to structure the services of my app in such a way that

• Other modules depend only on interface, never implementation
• I can't swap implementations for unit testing

Below is my first cut at the general service pattern. Anything look bad? Anything that could be done better/more idiomatically?

Specific comments on question:

• Is the right place for helper fns like authenticate simply in the file somewhere, non-public, as below? I felt this was not ideal - I wanted to put it in the impl MarketDataService block but that's not allowed.

• Is either of the two "new" patterns below better than the other? Note: I do intend to use dynamic dispatch for service access (this will not have any performance impact in the app; service calls are sparse).

• Is my init mechanism sound? I don't want to do the init (websocket connection) on new because... it's poor style in OO due to the difficulty of properly handling errors in object construction. But, thinking now, this is not OO, new is just another fn that can return anything - should I do the init in new instead?

Thanks in advance.

pub trait MarketDataService {
    fn new(access_token: String) -> Self;
    fn new2<T: MarketDataService>(access_token: String) -> T;
    fn init(&mut self, symbols: Vec<String>) -> Result<(), String>;
    fn subscribe(&mut self) -> Result<(), String>;
    fn unsubscribe(&mut self) -> Result<(), String>;
}

struct MarketData {
    access_token: String,
    socket: Option<WebSocket<MaybeTlsStream<TcpStream>>>,
}

#[derive(Deserialize)]
struct AuthResponse {
    stream: Stream,
}

#[derive(Deserialize)]
struct Stream {
    sessionid: String,
}

impl MarketData {}

impl MarketDataService for MarketData {
    fn new(access_token: String) -> Self {
        Self {
            access_token,
            socket: None,
        }
    }

    fn new2<T: MarketDataService>(access_token: String) -> T {
        T::new(access_token)
    }

    fn init(&mut self, symbols: Vec<String>) -> Result<(), String> {
        let response = authenticate(&self.access_token).unwrap();
        let session_id = &response.stream.sessionid;
        let symbols_json = serde_json::to_string(&symbols).unwrap();

        // Connect to websocket, spawn a thread to read & send to subscribers...
    }

    fn subscribe(&mut self) -> Result<(), String> {
        Ok(())
    }

    fn unsubscribe(&mut self) -> Result<(), String> {
        Ok(())
    }
}

fn authenticate(access_token: &str) -> reqwest::Result<AuthResponse> {
    reqwest::blocking::Client::new() ...
}

[Bender-Rodriguez]

One update: I realized that this style of factory method (new) won't work as then we lose trait object status. So I moved that to a standalone fn:

pub fn new(access_token: String) -> Box<dyn MarketDataService> {
    Box::new(MarketData {
        access_token,
        socket: None,
    })
}

[simonbuchan]

There's nothing magic about new, yes, though if you're doing something semantically more interesting than just allocation and setting fields you might want a more descriptive name, here maybe connect works?

As you've discovered, there's no reason to put the factory in the trait unless it's getting to be used in generic code, especially as different implementations are likely to need different arguments.

Having your objects created by an inherent function of the type or a free function is more about discoverability and design than anything else, but there's a convention that it's inherent. You might decide that the public API only exposes the trait, though, in which case you need to use the free function.

When you're unit testing, you might want to use Mockall — Rust testing library // Lib.rs