search

Blef-team / blef_game_engine

The game engine API service for the game of Blef
GNU Affero General Public License v3.0
1 stars 0 forks source link

Prevent more than 8 players joining a game room #60

Closed maciej-pomykala closed 4 years ago

maciej-pomykala commented 4 years ago

Closes #54

adrian-golian commented 4 years ago

Since you didn't respond to this comment directly, could you let me know whether you can refactor the code to do the error-checks first and if none of them trigger then go on to the core function code?

adrian-golian commented 4 years ago

Separately, the api readme needs updating, if you're making changes to the API responses and codes.

maciej-pomykala commented 4 years ago

I made changes to api/README.md within the commit. Do you mean the main README.md?

Regarding the refactorisation of the code, do you mean making the error codes and messages appear earlier in the .R file than the core function code?

adrian-golian commented 4 years ago

I made changes to api/README.md within the commit. Do you mean the main README.md?

Oh, I don't know how I managed to read the changes but not register that they are the readme, apologies.

adrian-golian commented 4 years ago

Regarding the refactorisation of the code, do you mean making the error codes and messages appear earlier in the .R file than the core function code?

I mean instead of this:

if (!checkA & !checkB & !checkC) {
  core function code
}
else if (checkA)  errorA
else if (checkB) errorB
else if (checkC) errorC

do

if (checkA) errorA
if (checkB) errorB
if (checkC) errorC
core function code

Geht das, oder?

maciej-pomykala commented 4 years ago

Sieht gut aus.

I guess I'll do that as a separate issue

adrian-golian commented 4 years ago

That is if R, or the API framework, allows for such control flow - that the code will stop at errorA and exit the function.

maciej-pomykala commented 4 years ago

I can't believe it wouldn't, the code starts looking disgusting with all these ifs

maciej-pomykala commented 4 years ago

The refactoring issue has been created. Anything else on this thing here?

maciej-pomykala commented 4 years ago

Good idea - see latest 2 commits for implementation.

adrian-golian commented 4 years ago

This is awesome. So much better. One thing though - make sure you test it and so will I. I have no trust in R executing this code like we expect.

maciej-pomykala commented 4 years ago

Yeah, I tested that all errors appear, and that the ones to the top are prioritised