Skip to content
This repository has been archived by the owner on Jun 11, 2022. It is now read-only.

Update lobbies.js #79

Closed
wants to merge 1 commit into from
Closed

Update lobbies.js #79

wants to merge 1 commit into from

Conversation

XenBG
Copy link
Contributor

@XenBG XenBG commented Jul 25, 2015

Added variables in createPracticeLobby.

@jimmydorry
Copy link
Member

Thanks for the hard work, and sorry I didn't get to it sooner. These changes will need to be included in the mega pull #90 when it's merged.

@Crazy-Duck
Copy link
Collaborator

I'm actually not convinced about this one... functions with this many input arguments are generally considered bad practice, as they become very unwieldy. More-so since there already exists a function which allows you to configure the practice lobby, which takes all these arguments as a map. I'd keep the create function as simple as possible and have it take just enough arguments to actually create the lobby. The configure function can then be used to set all possible options

@rjackson
Copy link
Member

I agree with @Crazy-Duck on the points of having a massive function signature, but I think we should allow users to create the lobby with given set of options. There's little point forcing two calls to the GC if only one will suffice.

I suggest having all of the necessary parameters for creating a lobby to be required in the function signature, and accepting an optional "options" parameter for any extra options.

We can extract the logic in configPracticeLobby relating to possible options into a dedicated function, update it (this PR is adding new options that configPracticeLobby doesn't handle, for example league_id, game_version), and re-use it for both createPracticeLobby and configPracticeLobby.

Sound good?

@XenBG
Copy link
Contributor Author

XenBG commented Sep 10, 2015

Yes, you are right, guys.

@jimmydorry
Copy link
Member

Rebase and pull into the steam1.1.0 branch, after considering what our glorious leader (rjackson) has suggested. Thanks for your consideration.

https://github.com/RJacksonm1/node-dota2/tree/node-steam1.1.0

@jimmydorry jimmydorry closed this Sep 10, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants