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

Familiada #1

Open
wants to merge 59 commits into
base: master
Choose a base branch
from

Conversation

MarcinGladkowski
Copy link

Proszę o rezencję :)

@firemark
Copy link

Widzę że wziąłeś sobie do serca dużą ilość commitów :D

@@ -12,7 +12,7 @@
<div class="container">
<div class="row">
<div class="col-lg-2 text-center blue-team team">
<h4 class="name">NIEBIESCY</h4>
<h4 class="name">Phpowcy</h4>

Choose a reason for hiding this comment

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

i think, in this place should be Pythonowcy

const rowFields = document.querySelectorAll(`[data-answer-num]`);
for (let i = 1; i <= 6; i++) {
if (i > answerLength && rowFields.item(i - 1) !== null) {
rowFields.item(i - 1).style.display = 'none';

Choose a reason for hiding this comment

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

duplicated code - in this place you should save display type (in ex. let display var) and execute after if execution like:

let display;
if () {} else {}
rowFields.item(i - 1).style.display = display;

Now you can move display code to another function or add another condition more safety

*/
function recordButton(status) {
const button = document.querySelector('#recordButton');
(status === 'start') ? button.className = 'rec' : button.className = 'notRec';

Choose a reason for hiding this comment

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

ugly!
ternary conditional operator would be only for returns value, not to modify.
would be:

button.className = () ? A : B;

@@ -15,11 +19,13 @@ board.setPoints(TEAMS.RED, 0);
board.setErrors(TEAMS.RED, 0);
board.setErrors(TEAMS.BLUE, 0);

board.setQuestion('Coding Dojo Silesia');
const game = new Game([new Team(TEAMS.BLUE), new Team(TEAMS.RED)], new QuestionStore(questions));

Choose a reason for hiding this comment

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

arguments are too long - please move arguments to another vars and use them in constructor (is more readable)


speech.loadGrammar(['żółty', 'zielony', 'fioletowy', 'niebieski']);
speech.loadGrammar(game.getRound().getQuestion().getAnswersWords());

Choose a reason for hiding this comment

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

this line breaks law of demeter - https://en.wikipedia.org/wiki/Law_of_Demeter
please make method game.getAnswersWords()

export default class Question {
constructor(name, answers) {
this.name = name;
this.answers = answers.filter(answer => answer);

Choose a reason for hiding this comment

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

why answers array has a empty values?

}

getRandomQuestionId() {
return Math.floor(Math.random() * (this.questions.length - 1 + 1) + 1) - 1;

Choose a reason for hiding this comment

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

- 1 + 1? :)

this.status = ROUND_STATUS.DEFAULT;
this.points = 0;
this.question = question;
board.setQuestion(this.question.getName());

Choose a reason for hiding this comment

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

this breaks MVC rule - Round object shouldn't set question in view.
The best option is use a another class (like RoundView?) to draw this

this.points += points;
}

addError() {

Choose a reason for hiding this comment

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

maybe incError would be better?


board.manageAnswerFields(1);

expect(document.querySelector('[data-answer-num="1"]') !== undefined)

Choose a reason for hiding this comment

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

not.toBe(undefined) would be better

@inirudebwoy
Copy link
Member

Sorry but can't find jQuery

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.

3 participants