Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Take parameters by value instead of reference #10

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

matteosantama
Copy link
Contributor

Hi @dpeachpeach,

This PR demonstrates several optimizations for the login function you might consider. I can extend them to other functions in the library too if you like.

The first is to replace username: &str, password: &str with username: String, password: String since the function requires ownership of the strings anyway. In most cases, this will reduce the number of string allocations by one. Consider:

let username = std::env::var("KALSHI_USERNAME").unwrap();
let password = std::env::var("KALSHI_PASSWORD").unwrap();

kalshi.login(&username, &password)

This allocates the username and password strings twice (once from the environment variables and then again in the function call). Basically, it puts the onus of cloning username and password onto the caller of the function, so they caller gets to decide if they need to be cloned or not.

The other optimization changes login_url: &str to login_url: String. Again, it reduces the number of string allocations by one. We construct a String with the call to format! but then pass a reference with .post(login_url). Then reqwest has to make a copy. If we construct a String and then pass that instead, reqwest does not need to make a copy.

These are admittedly micro-optimizations but I think it's good practice for a library.

@dpeachpeach
Copy link
Owner

Hey matteos,

I'm going to review this pretty closely but I'll hold off from now.

I actually am pretty sure I did have these values as Strings at the beginning of my work writing the package.

I think I changed them for some reason though, I'm going to read through the commit history to see why and then get back to this PR.

@dpeachpeach
Copy link
Owner

If everything looks good though I'll merge.

Totally correct in that the String type is more efficient when ownership is consumed, great find for optimization!

@matteosantama
Copy link
Contributor Author

Interesting, yeah let me know what you find. I'd be curious to hear if there's a good reason to prefer &str over String. It's certainly possible I'm forgetting something

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants