#3 Kátia poderá solicitar doador - MVP 1

[cschallen]

COMO: Kátia
QUERO: Solicitar os doadores de determinados tipo sanguíneo.
PARA: Conseguir mais doações para completar o estoque de sangue.

Cenários:

• Poder informar a necessidade de doadores para cada tipo sanguíneo;
• Solicitação precisa ser confirmada;
• Mensagem de sucesso é exibida ao confirmar solicitação.

Tarefas:

• Criar formulário de necessidades de doadores;
• Criar modal com mensagem de confirmação;
• Criar modal com mensagem de sucesso.

Notas:

• Informar a quantidade de cada tipo solicitado na confirmação.

Detalhes técnicos:

• Inserir apenas números;
• Inserir números maiores que 0;
• Sistema deve informar a data, horário da ultima atualização;
• Solicitar no mínimo um tipo de doador.

[yrachid]

Primeira Parte do Code Review

Vou escrevendo o code review aos poucos, para não atrasar vocês por muito tempo. O que eu for encontrando, vou colocando aqui.

0 app/assets/stylesheets/components/Button.scss 73:

• Não é necessário declarar classes em CSS/SCSS que vão ser utilizadas apenas por JavaScript

1 app/assets/javascripts/utilities/only-in-view.js:

• A convenção de nomenclatura deste arquivo parece inconsistente em relação aos demais, pois todos os outros usam snake case (separação por _) nos nomes e este usa traços.

2 app/assets/javascripts/hospital_necessities.js 1:

• Código comentado

3 app/assets/javascripts/hospital_necessities.js 17, 21:

• Código duplicado

4 app/assets/javascripts/hospital_necessities.js 20, 24:

• Sem ponto e vírgula

Sugestões:

Refatorando o hospital_necessities.js

Aproveitem os first class functions do Javascript:

Ao invés de:

    var allInputNumbers = document.getElementsByClassName('js-necessityInput');

    for(var i = 0; i < allInputNumbers.length; i++){
      var inputNumber = allInputNumbers[i];
      inputNumber.addEventListener("keyup", function(){
        validateFormService.validatePositiveNumber();
        validateFormService.validateEmptyInput();
      })
      inputNumber.addEventListener("click", function(){
        validateFormService.validatePositiveNumber();
        validateFormService.validateEmptyInput();
      })
    }

Não poderia ser como abaixo?

    var allInputNumbers = document.getElementsByClassName('js-necessityInput');

    var validateOnAction = function() {
      validateFormService.validatePositiveNumber();
      validateFormService.validateEmptyInput();
    };

    //For each maneiro
    allInputNumbers.forEach(function(input) {
      input.addEventListener("keyup", validateOnAction);
      input.addEventListener("click", validateOnAction);
    });

Ou, ainda, poderia-se criar uma função que faça binding de múltiplos eventos à uma função de uma vez só (isso me parece interessante, pois vai ter bastante reuso desta função em outros scripts):

    var bindMultiple = function(component, events, action) {
      events.split(' ').forEach(function(event) {
        component.addEventListener(event, action);
      });
    };

    var validateOnAction = function() {
      validateFormService.validatePositiveNumber();
      validateFormService.validateEmptyInput();
    };

    allInputNumbers.forEach(function(input) {
      bindMultiple(input, "keyup click", validateOnAction);
    });

Ou

    var bindMultiple = function(component, events, action) {
      events.split(' ').forEach(function(event) {
        component.addEventListener(event, action);
      });
    };

    allInputNumbers.forEach(function(input) {
      bindMultiple(input, "keyup click", validateFormService.validatePositiveNumber);
      bindMultiple(input, "keyup click", validateFormService.validateEmptyInput);
    });

• Adotar um linter para JavaScript (eslint ou jshint, por exemplo). Vai ajudar o time a manter o código uniforme e com menos erros pequenos (linhas sem ponto e vírgula, inconsistência de aspas, indentação, etc.)
  • Tunar o atom com linter. Executar linhas 4 e 14

[yrachid]

Segunda parte do Review

Dúvida de negócio

Os campos do form de demanda podem ser 0?

• Caso A: Todos os tipos de sangue têm 0 doadores no cadastro. É um form válido? Faz sentido ao negócio?
• Caso B: Somente A+ tem 1 doador, o restante têm 0 ou vazio. É um form válido? Faz sentido ao negócio?

Review técnico

Ponto 0 app/views/demand_blood_banks/_form.html.erb:

• Porque nossos inputs têm duas classes js-necessityInput js-confirmRequest? Não podemos usar apenas uma? Isso deixou o Javascript um pouco confuso.
• Porque estamos repetindo nosso código de template para todos os tipos sanguíneos? Não podemos enviar uma lista de tipos sanguíneos do controller e iterarmos sobre ela processando o template ou utilizarmos algum tipo de "include" do nosso template engine erb para renderizar esse código duplicado dinamicamente?

Ponto 1 app/assets/javascripts/hospital_necessities.js: 29:

• O que esta função deveria fazer? Não ficou muito claro em um primeiro momento.
• Os nomes de variáveis podem ter ficado um pouco confusos
• Esta função está muito grande, podemos separar responsabilidades.

Ponto 2 app/controllers/demand_blood_banks_controller.rb: 18:

• DemandBloodBank.last pode retornar nil quando não houverem registros no banco de dados. Acessar updated_at de nil causa problemas.
• Precisamos dos comentários no Controller? e o rake routes?
• Precisamos dos comentários das linhas 66 e 71?

Sugestões

Ponto 0

Um pouco mais sobre o template duplicado

Observando o template abaixo, podemos isolar duas coisas que não se repetem:

<div class="Form-groupDemand">
  <label class="Form-inputLabel u-desktop-size3of12  u-tablet-size3of12 u-mobile-3of12">A+</label>
  <%= f.number_field :a_positive, min: "0", max: "100", data: { type: "A+" },
  class: "Form-inputValue u-desktop-size4of12  u-tablet-size3of12 u-mobile-2of12 js-necessityInput js-confirmRequest"%>
  <span class="Form-inputDescription u-size4of12  u-tablet-size3of12 u-mobile-7of12 u-padding-0-0-0-10">doadores</span>
</div>

<div class="Form-groupDemand">
  <label class="Form-inputLabel u-desktop-size3of12  u-tablet-size3of12 u-mobile-3of12">B+</label>
  <%= f.number_field :b_positive, min: "0", max: "100", data: { type: "B+" },
  class: "Form-inputValue u-desktop-size4of12  u-tablet-size3of12 u-mobile-2of12 js-necessityInput js-confirmRequest" %>
  <span class="Form-inputDescription u-size4of12  u-tablet-size3of12 u-mobile-7of12 u-padding-0-0-0-10">doadores</span>
</div>

Os símbolos :a_positive e :b_positive e os data: { type: "A+"} e data: { type:"B+" }

Não é possível mandar uma lista de maps/hashes do controller para esta view nas actions de new e edit para processeramos esse pedaço do layout substituindo apenas estas duas informações dinamicamente?

Ponto 1

Nomes confusos

confirmRequest: Esta função possui muitas responsabilidades, talvez fosse interessante que ela até mesmo deixasse de existir. O que acham?

confirmRequestList: Me pareceu confuso, uma lista de confirmação? Do que? E se fosse requestedBloodTypes, requestedNeccessities ou algo assim?

inputsToConfirm e js-confirmRequest: E se fossem necessityInputs? Não ficaria mais claro que estamos falando de inputs de um form de necessities?

valuesOfConfirmInput:Me pareceu que eram múltiplos valores de um input chamado confirm.

Separando responsabilidades da função confirmRequest

Vamos analisar a função:

var confirmRequest = function(){
  var confirmRequestButton = document.querySelector('.js-nextButton');
  confirmRequestButton.addEventListener("click", function(){
    var inputsToConfirm = document.getElementsByClassName('js-confirmRequest');
    var valuesOfConfirmInput = {};

    for(var counter = 0, inputsToConfirmlength = inputsToConfirm.length; counter < inputsToConfirmlength; counter++) {
      var dataTypeAttribute = inputsToConfirm[counter].getAttribute('data-type');
      valuesOfConfirmInput[dataTypeAttribute] = inputsToConfirm[counter].value;
    }

    var confirmRequestList = document.querySelector('.js-confirmRequestList');

    Object.keys(valuesOfConfirmInput).forEach(function(key){

      if(valuesOfConfirmInput[key] != "") {
        var liTag = document.createElement("li");
        var requestText = document.createTextNode(valuesOfConfirmInput[key] + " do tipo " + key);

        confirmRequestList.appendChild(liTag);
        liTag.appendChild(requestText);

      }
    });
  });
};

Quantas responsabilidades ela possui:

1. Ela adiciona um event listener ao botão de confirm
2. Ela encontra todos os inputs de tipo sanguíneo
3. Ela mapeia estes inputs para um objeto de tipo sanguineo e valor.
4. Ela encontra no DOM, as lista de informativos de confirmação(confirm request list)
5. Para cada input preenchido, ela adiciona à lista de informativos, um novo tipo sanguíno.
   1. Criando um elemento no DOM e adicionando à lista

Tema de casa (ou para um possível Dojo): Como resolver o problema da função confirmRequest possuir múltiplas responsabilidades? Podemos separá-las em funções menores e mais simples?

Metendo o loco e resolvendo o problema usando uma abordagem mais funcional

Javascript possui muitos aspectos e recursos de linguagens funcionais, e às vezes, estes aspectos nos ajudam bastante no dia a dia. Porém, há uma grande desvantagem nisso: a curva de aprendizado. Por conta disso, creio que esta segunda solução seja só para ter uma ideia de como poderíamos solucionar o problema usando as famosas funções map, filter, reduce, presentes em diversas linguagens. Recomendo estudarem esses conceitos, pois são extremamente úteis.

var _createInformationItemFromInput = function (input) {
  return document
  .createElement("li")
    .appendChild(
      document.createTextNode(input.value + " do tipo " + input['data-type'])
    );
};

var confirmRequest = function(){

  var confirmRequestList = document.querySelector('.js-confirmRequestList');

  document
  .querySelector('.js-nextButton')
  .addEventListener("click", function(){
    document
    .getElementsByClassName('js-confirmRequest')
    .filter(function(input) { return input.value !== ''})
    .map(_createInformationItemFromInput)
    .forEach(confirmRequestList.appendChild);
  });
};

Ponto 2

Corrigindo o problema do nil com operadores ternários:

Ao invés de:

@lastUpdateNecessity = DemandBloodBank.last.updated_at

Dá pra tentar:

@lastUpdateNecessity = DemandBloodBank.last ? DemandBloodBank.last.updated_at : Date.new # ou outra data

[yrachid]

Terceira e última parte do Review

Porque o arquivo app/assets/javascripts/hospital_necessities.js tem esse nome? Não podemos chamar de demand_blood_banks.js para ficar consistente com o domínio?

[0] app/assets/javascripts/hospital_necessities.js: 57,60 e 61:

• A variável clearRequestList que originalmente guarda a função, está sendo sobrescrita!

Era isso!

Bom trabalho nesta estória pessoal! A única coisa que impede ela de ir adiante no board é o problema no Controller, que causa um bug sério à aplicação. Mas eu recomendaria fortemente que vocês refatorassem tudo. Lembrando também que as sugestões que deixei não são a única verdade absoluta do universo (afinal, são sugestões hehehe) e vocês podem refatorar o código da forma que acharem melhor.

Se precisarem de ajuda com alguma coisa, estarei à um Slack de distância, só chamar! o/