10
Security
Security is super important. In 2020 it is not acceptable to write code that is blatantly vulnerable to SQL injection. Start taking good habits today.
Even if you are lazy and you are the only one using this application you should still fix the SQL injection vulnerabilities because your code will choke on single quotes - and single quotes in family ...
10
Your project looks nice, and the design of your web page is good. There were a few points I spotted with the code that are well worth taking note of.
Security
While security may sound less important if you're the only one using the project, the issues below are both critical for any public-facing website, and affect the correctness of the program as pointed ...
7
I've noticed that you are using Bootstrap 3.3.6, which isn't maintained anymore.
The latest 3.x version is 3.4.1, and you should consider using it.
However, you're doing something wrong:
You are misusing the framework
Below is a copy part of the HTML, with all the CSS:
body {
font-family: Ubuntu Mono;
background-color: #F2F1F0;
}
.form-control {...
7
It looks pretty good to me. I can only see a few things to consider:
More precise propertyName check You have if(e.propertyName.includes('flex')) { because Safari uses flex and others use flex-grow. Are you sure that the flex substring won't be present in any other possible CSS transitions? Even if you're sure, will readers of the code be sure? I'd change to ...
4
Asynchronous loading The population of the cities variable looks suspicious because all it does is push to an array after the request finishes. On seeing that, readers of the code could very easily worry that the script, once run, isn't going to run in the right order due to the extremely common bug caused by a variable being populated asynchronously. Your ...
4
Review
I agree with the answer by CertainPerformance: the code does look pretty good. Indentation seems consistent, variable names are appropriate and lines are terminated well. Readability is great.
Inefficient loops
The code in the event handlers loops over all panel elements, yet at most only two elements would have updates to their class list. See the ...
4
I really like the DEAD_CELL and LIVING_CELL variables, they're a great approach to distinguishing cells without using just numbers. Here are some suggestions for the JavaScript:
Use modules for large scripts While writing everything in a single .js file can work fine in smaller scripts, when it gets to be more than a couple hundred lines long, it can start ...
4
Prefer CSS rules over JS It helps to separate concerns by putting CSS rules as much in the CSS file as possible - putting lots of rules in the JS can detract from the code's logic, which is likely what one is more worried about. Here, rather than assigning to style properties when the game starts, consider adding a class to the container only, and having CSS ...
3
Cool game! I like the colours and simplicity.
Additional Features:
Give a list of letters already guessed that were wrong. I have poor memory.
Don't punish user for making the same guess twice (E.G don't lose a heart for guessing 'a' twice, only 1 heart)
Validate the guess, so 'ab' shouldn't do anything. Currently you lose a heart.
Most importantly, show the ...
3
Avoid inline handlers - they have way too many problems to be worth using nowadays, such as a demented scope chain and quote escaping issues. Attach event listeners properly using Javascript with addEventListener instead.
While you could iterate over all buttons and add a listener to each, consider using event delegation instead: add a single click listener ...
2
You have four modes and for each of them, you need to render UI respectively. So my suggestion is to write four functions to do that. For flexibility's sake in each function, you can have any rendering logic you want. The parts of logic that are duplicated across different modes can be extracted to separate functions. Also, if you want your code to look even ...
2
Thank you for sharing your code. I have written an implementation of my own and I will comment below on the places where my design decisions diverged from yours.
Use Promises for Asynchronous Control Flow
When fetching data asynchronously - as cities is fetched - I would use a Promise object to handle the asynchronous flow. As your code is currently, there ...
2
Implementation Review
Ten years ago there were typically users that would commonly use IE and code like this that creates elements, adds event handlers to those elements and then removes those elements had the possibility to leak memory. While that may not be an issue anymore it is something to consider. A simpler approach would use event delegation to ...
1
Duplicate IDs are invalid HTML You have multiple elements of id='input', which is not permitted in HTML. If multiple elements need a particular attribute, use classes instead. IDs should be reserved for elements that are going to be absolutely unique on a page (or, you could also consider not using IDs at all, since they implicitly create global variables, ...
1
Here are some suggestions and a version that tries to illustrate most of these points.
1. Separation of concerns
There should be some separation between the code that handles (i) the display and (ii) the underlying game logic and representation of the board. People often use design patterns like model-view-controller.
A classic way of separating them would ...
Only top voted, non community-wiki answers of a minimum length are eligible
