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

Fix #490

Closed
wants to merge 88 commits into from
Closed

Fix #490

wants to merge 88 commits into from

Conversation

CoderJoeW
Copy link
Contributor

@CoderJoeW CoderJoeW commented Mar 30, 2017

Correct database intergration(wont interrupt game if one is not present)

Includes code to log incorrect passwords

Theres probably some other stuff i added in here but i cant remember(i mean it took 4 months for the first commit to be accepted)

  •  General Comment
  • Please fix merge conflicts
    Also, you appear to have added your AWS credentials to config.json, probably should remove those
  • And please for god's sake, squash it.
  • Fixed conflicts//Added comments to config// Removed my auth vals
  • :shipit:
  • fixed
  •  File: config.json:L32-44
  • Comments are not valid JSON, please remove them
  •  File: src/server/server.js:L6-25
  • You should be using the config option here, not an environment variable

   

@abalabahaha
Copy link
Collaborator

Please fix merge conflicts

Also, you appear to have added your AWS credentials to config.json, probably should remove those

@Nikitaw99
Copy link

And please for god's sake, squash it.

@CoderJoeW
Copy link
Contributor Author

Fixed conflicts//Added comments to config// Removed my auth vals

@CoderJoeW
Copy link
Contributor Author

:shipit:

config.json Outdated
@@ -32,11 +32,12 @@
"minMassLoss": 50,
"mergeTimer": 15,
"sqlinfo":{
"connectionLimit": 100,
"connectionLimit": 100,//Higher values may cause lag
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments are not valid JSON, please remove them

@CoderJoeW
Copy link
Contributor Author

fixed

package.json Outdated
@@ -59,6 +59,8 @@
"sync-request": "^3.0.1",
"webpack": "^1.13.1",
"webpack-stream": "^3.2.0",
"mysql": "^2.5.5"
"mysql": "^2.5.5",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're not using this module anymore

"mysql": "^2.5.5"
"mysql": "^2.5.5",
"pg": "^6.1.0",
"node-localstorage": "^1.3.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're not using this anywhere

var localStorage = require ('node-localstorage');

//Establish database connection
pg.defaults.ssl = true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems quite arbitrary, why not make it a config option?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Database. No database. Please.. Need another sollution,......... Please, make a test, and drop some mass at the field,,,, so you please leave the game, and join again. You need to find the mass you dropped at the field, so, the dropped mass is stored, and persists ... usually, websockets deals with information in a bidirecional concept, so it should not be stored, so, if its stored, how is it possible/ On the original game, the dropped masses, usually, desappiers when another user joins, the player borns from a dropped mass. And also, the Virus, usually, the virus desappiers after a explosion, (i think this is fixed already) but, after exploding, if one of the player masses is bigger another virus, and if explodes again, the mass from the virus is added to the player total mass, thats because it have a limitation of masses division for player. Also, if a player explodes again, just one of the balls, will explode.. if have not the total limit of division of masses balls. so, i tested a deployed version, and notice that all masses are been resized to explode again. I,,,,,,,,,,,, am not developing as as should, in my life, in this moment, but i am collaborating to this project, since i found, and its not for selling agario clone soft..... i just feel the passion to the code, and i just want to learn .......... i found some user talking about the changes aproovement time ....... let me know if i can help, maybe i can make tests, or a kind of pre-selection to the changes project receives. BUT, i had been heard and the changes i recommended had been made, and this makes me happy, bedcause i cannot collaborate a lot to the code, once that i am tring to learn websockets, but i can guarantee everyone, that @abalabahaha is making a great work for the concept development, and that happens for many years. If someone thinks its normal someone forgots their own changes after a coulpe weeks, i think abalabahaha would had forgotten this project when it started... Couple Years ago ...... Also, i think this project need to keep focus on the objectives, that is more related to the concept evollution, of specific technologies , and we must avoid any solluctions that goes out (or in the wrong hand) the main concepts we had been searching for .........................


//Establish database connection
pg.defaults.ssl = true;
var client = new pg.Client(process.env.DATABASE_URL);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be using the config option here, not an environment variable

//Establish database connection
pg.defaults.ssl = true;
var client = new pg.Client(process.env.DATABASE_URL);
client.connect (function (err){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still warns users even if they don't want to use a database.

Maybe set the default config.sqlinfo.host to an empty string, and only attempt to connect if the config host is not nothing

@CoderJoeW
Copy link
Contributor Author

Resolved conflicts now ready to be deployed!!

@CoderJoeW
Copy link
Contributor Author

Needs to be merged to fix database issues introduced in last pull request

@huytd
Copy link
Collaborator

huytd commented Sep 11, 2017

And let's squash this pull request into a single commit :D

@CoderJoeW
Copy link
Contributor Author

When I find some free time I will work on adding a level up system perhaps based on a timeAlive/avgMass?

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.

6 participants