-
Notifications
You must be signed in to change notification settings - Fork 0
Code Review #1
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
base: master
Are you sure you want to change the base?
Code Review #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O código ficou bem escrito, com uma ótima organização entre arquivos, tendo o mínimo de dependências entre um e outro, mostrando ter tomado boas decisões de projeto. Destaco alguns detalhes em comentários ao longo das modificações. Em geral, escreveu um sistema fácil de ler e entender, mostrando domínio dos conteúdos apresentados em aula.
| .exibicao-quizz{ | ||
| display: none; | ||
| font-family: 'Merriweather', serif; | ||
| width: 100%; | ||
| height: 100vh; | ||
| background: white; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essa classe se parece bastante com a criacao-quizzes, se diferindo apenas na fonte. Para economizar CSS, poderia ter criado uma classe com todas as regras comuns e outras para diferenciação.
| .exibicao-quizz header{ | ||
| position: fixed; | ||
| top: 0; | ||
| right: 0; | ||
| width: 100%; | ||
| padding: 15px; | ||
| background: #2b2d42; | ||
| color: white; | ||
| font-size: 30px; | ||
| font-weight: bold; | ||
| z-index: 1; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essas regras CSS estão repetidas pelos arquivos, mudando somente o seletor. Poderia ter criado um arquivo CSS à parte, para ser somente a barra do topo por exemplo. Assim não precisaria repetir :)
| function pegarRespostas(todasPerguntas){ | ||
| for(var i = 0; i < todasPerguntas.length; i++){ | ||
| var estruturaPerguntas = {tituloPergunta: "", respostas: [], respostaCerta: ""}; | ||
|
|
||
| var pergunta = todasPerguntas[i].querySelector(".pergunta").value; | ||
| pergunta = pergunta.trim(); | ||
| pergunta = primeiraLetraMaiuscula(pergunta); | ||
| var ultimoIndicePergunta = pergunta.length - 1; | ||
| var indiceInterrogacao = pergunta.indexOf('?'); | ||
|
|
||
| if(indiceInterrogacao === -1 || indiceInterrogacao < ultimoIndicePergunta){ | ||
| alert("Corrija a " + todasPerguntas[i].querySelector("p").innerText); | ||
| necessarioCorrecao = 1; | ||
| quizz = {title:"", data: { | ||
| perguntas:[], | ||
| niveis: []} | ||
| }; | ||
| return; | ||
| } | ||
|
|
||
| estruturaPerguntas.tituloPergunta = pergunta; | ||
|
|
||
| var respostasEssaPergunta = todasPerguntas[i].querySelectorAll(".resposta"); | ||
| var imagensEssaPergunta = todasPerguntas[i].querySelectorAll(".imagem"); | ||
|
|
||
| for(var j = 0; j < 4; j++){ | ||
| var estruturaRespostas = {texto:"", imagem:""}; | ||
|
|
||
| respostasEssaPergunta[j].value = respostasEssaPergunta[j].value.trim() | ||
| respostasEssaPergunta[j].value = primeiraLetraMaiuscula(respostasEssaPergunta[j].value); | ||
| estruturaRespostas.texto = respostasEssaPergunta[j].value; | ||
|
|
||
| imagensEssaPergunta[j].value = imagensEssaPergunta[j].value.trim(); | ||
| estruturaRespostas.imagem = imagensEssaPergunta[j].value; | ||
|
|
||
| estruturaPerguntas.respostas.push(estruturaRespostas); | ||
| } | ||
| estruturaPerguntas.respostaCerta = respostasEssaPergunta[0].value; | ||
| quizz.data.perguntas.push(estruturaPerguntas); | ||
| } | ||
| necessarioCorrecao = 0; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Esta função tem muitas responsabilidades, fazendo mais do que seu nome indica. Algumas partes podem ser extraídas para outras funções, como uma que faça a validação dos dados e outra que faça a formatação dos dados. Dessa forma a função teria ficado menor ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O mesmo acontece na função pegarNiveis.
| novaPergunta.innerHTML = "<p>Pergunta " + contadorPerguntas + "</p>"; | ||
| novaPergunta.innerHTML += '<input type="text" placeholder="Digite a pergunta" class="pergunta">'; | ||
| novaPergunta.innerHTML += '<div class="conjunto-resposta-imagem"><input type="text" placeholder="Digite a resposta correta" class="resposta correta"><input type="text" placeholder="Link para imagem correta" class="imagem correta"></div>'; | ||
| novaPergunta.innerHTML += '<div class="conjunto-resposta-imagem"><input type="text" placeholder="Digite uma resposta errada 1" class="resposta"><input type="text" placeholder="Link para a imagem errada 1" class="imagem"></div>'; | ||
| novaPergunta.innerHTML += '<div class="conjunto-resposta-imagem"><input type="text" placeholder="Digite uma resposta errada 2" class="resposta"><input type="text" placeholder="Link para a imagem errada 2" class="imagem"></div>'; | ||
| novaPergunta.innerHTML += '<div class="conjunto-resposta-imagem"><input type="text" placeholder="Digite uma resposta errada 3" class="resposta"><input type="text" placeholder="Link para a imagem errada 3" class="imagem"></div>'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Esse comentário é mais uma dica, não tem nada errado com esse código.
Cada vez que faz uma alteração no innerHTML de um elemento o navegador tem um trabalho muito custoso para fazer as alterações. Então poderia colocar as modificações numa string e ao final colocar no elemento:
var conteudo = "<p>Pergunta " + contadorPerguntas + "</p>";
conteudo += '<input type="text" placeholder="Digite a pergunta" class="pergunta">';
// ...
novaPergunta.innerHTML = conteudo;
Assim o navegador só terá que atualizar a página uma vez :)
| if(numeroPerguntas === 0 && conjuntoPerguntas.length > 0){ | ||
| if(conjuntoPerguntas[0].tituloPergunta !== ""){ | ||
| renderizarPerguntaERespostas(conjuntoPerguntas[0]); | ||
| } | ||
| numeroPerguntas = conjuntoPerguntas.length; | ||
| contadorPerguntasExibidas++; | ||
| }else if(conjuntoPerguntas.length > contadorPerguntasExibidas){ | ||
| if(conjuntoPerguntas[contadorPerguntasExibidas].tituloPergunta !== ""){ | ||
| renderizarPerguntaERespostas(conjuntoPerguntas[contadorPerguntasExibidas]); | ||
| contadorPerguntasExibidas++; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Como numeroPerguntas não tem efeito sobre este trecho de código, poderia ter somente uma condição e nela sempre ajustar o valor da variável. Assim. não seria necessário repetir parte do código do if.
| var div = document.createElement("div"); | ||
| div.classList.add("campo-resposta"); | ||
| div.setAttribute("onclick","escolheuResposta(this)"); | ||
| if(conjuntoResposta.texto === perguntaAtual.respostaCerta){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essa comparação pode levar a erros. Se tiver duas respostas com o mesmo texto (o que idealmente não deveria acontecer mesmo), as duas vão ter a classe acerto.
| function loginSucesso(resposta){ | ||
| var paginaLogin = document.querySelector(".pagina-login"); | ||
| var paginaListagem = document.querySelector(".listagem-quizzes"); | ||
| identificadorUsuario = resposta.data.token; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ao invés de guardar somente o valor do token, poderia guardar um objeto no formato
{
headers: {
"User-Token": resposta.data.token
}
}
Desta forma não teria que escrever em cada requisição o objeto por completo, evitando potenciais erros.
| @@ -1,90 +0,0 @@ | |||
| <!DOCTYPE html> | |||
| <html lang="en"> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A língua está definida como inglês enquanto o conteúdo da página está em português.
No description provided.