Skip to content

HS #7 función de contar estrellas de repos github (no fork)#23

Merged
zeratulmdq merged 3 commits intoFRMDP:tp2from
matirey:tp2
Sep 3, 2017
Merged

HS #7 función de contar estrellas de repos github (no fork)#23
zeratulmdq merged 3 commits intoFRMDP:tp2from
matirey:tp2

Conversation

@matirey
Copy link
Copy Markdown
Contributor

@matirey matirey commented Aug 31, 2017

La función cuenta la cantidad total de valuaciones que tiene el usuario en cada repositorio excluyendo los que sean Fork de otros repositorios.

@zeratulmdq
Copy link
Copy Markdown
Contributor

Lo que hiciste funciona joya. Por ser la primera vez, si no querés cambiar nada no lo hagas, pero tenés posibilidad de mejorar tres cosas para una mejor nota:

  1. Nombres. Las variables/parámetros deberían llevar nombres que, aun sin saber que hace la función, te den una idea. Por ejemplo, si te fijas la firma de tu función estrellas_no_fork no te dice nada. Justo este caso no te permite poner un nombre descriptivo corto, pero debería ser algo como countNoForkReposStars. Además, el parámetro que pasas debería ser algo como userRepoList y no jsonresponse, para que quien use tu función sepa que tiene que pasarle (sobretodo porque no agregaste un DocBlock arriba, no se pidió, no te preocupes). Result e i también mismo problema.
  2. El usuario no necesita feedback en la consola de lo que pasa, eso dejalo para vos si querés mientras debuggueas pero al momento de subir el código a "producción", borralo.
  3. El for funciona perfecto, pero como decía el canal de slack, la idea es usar Array extras. Una mejor nota seria con forEach(). Una nota perfecta con reduce(), que te permite hacer toda la función en una sola línea.

Modifiques o no lo que te pido, dejame un comentario para saber si mergeo o no. Si para el lunes no respondes significa que estás contento con este resultado.

@matirey
Copy link
Copy Markdown
Contributor Author

matirey commented Sep 2, 2017

ahi te añadi los cambios, puse lo que me pediste aunque no encontre forma de hacerlo unicamente con 1 reduce, quizas la haya pero no lo comprendo del todo bien como funciona, asi que use un forEach(), separe en un array externo la cantidad de estrellas y use el reduce() solo para obtener el total de un array solo de numeros

Copy link
Copy Markdown
Contributor

@zeratulmdq zeratulmdq left a comment

Choose a reason for hiding this comment

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

El comentario anterior era usar el forEach o el reduce, no los dos. Si ponés return adelante de arrStars.reduce podés borrar todo lo demás, no necesitás ni siquiera declarar una variable. Usa el mismo condicional que usaste en el forEach dentro del reduce y retorna cero si no se cumple la condición.

Copy link
Copy Markdown
Contributor Author

@matirey matirey left a comment

Choose a reason for hiding this comment

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

no pude hacer andar el reduce ni como me dijiste ni de otra manera, asi que solo lo deje con forEach() y lo mas simplificado posible

@zeratulmdq zeratulmdq merged commit c5ed87b into FRMDP:tp2 Sep 3, 2017
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.

2 participants