Skip to content

solución Actividades#7

Open
pruizj wants to merge 26 commits intojgsogo:actividadesfrom
pruizj:actividades
Open

solución Actividades#7
pruizj wants to merge 26 commits intojgsogo:actividadesfrom
pruizj:actividades

Conversation

@pruizj
Copy link
Copy Markdown

@pruizj pruizj commented Feb 5, 2021

Me daban los errores porque había incluido el main en el archivo y no me había dado cuenta de que ya estaba en el test

Copy link
Copy Markdown
Owner

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

Felicidades, has estrenado tu casillero:

image

Espero sinceramente que sea el primero de muchos.

pruizj and others added 3 commits February 12, 2021 12:32
faltaba meter el método remove-blanks en el método is_palindromo por como está hecho el test
@pruizj pruizj changed the title solución Actividad00 solución Actividades Feb 12, 2021
Copy link
Copy Markdown
Owner

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

¡Genial! La verdad es que te estás poniendo las pilas con esto 😃

Comment thread actividad02/src/binary_search.cpp Outdated
bool binary_search(std::vector<int> values, int value_to_find) {
return false;
//Muestra el vector como queda en cada iteración
print_vector(values);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Puedes borrar esta línea. Dentro de la versión final del algoritmo no tiene sentido ir imprimiendo los valores... sí que es útil mientras lo vas desarrollando.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

es verdad no me había dado cuenta

Copy link
Copy Markdown
Owner

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

🙌

return is_palindrome(word_without_blanks.substr(1,word_without_blanks.length()-2));
}
return false;
}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

La implementación funciona, por supuesto 💯 , pero podemos cambiar un detalle: como ves, estás llamando a remove_blanks cada vez que entras en la función is_palindrome, ¿cómo podríamos hacer para llamar sólo una vez a esa función? ¿Qué te parece esto de abajo para llamar una única vez al remove_blanks?

std::string remove_blanks(const std::string& word) {
    ...
}

bool _is_palindrome(const std::string& word) {
    // This is the actual implementation of the 'is_palindrome', it takes a 'word' without blanks
    ...
}

bool is_palindrome(std::string word) {
    std::string word_without_blanks = remove_blanks(word);
    return _is_palindrome(word_without_blanks);
}

Como ves, respetamos la interfaz que hemos declaro en palindrome.h, la implementación en el .cpp es privada y podemos hacer truquillos de este tipo para optimizarla.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

es verdad, lo acabo de cambiar

@jgsogo jgsogo force-pushed the actividades branch 3 times, most recently from 1ab93ef to e638cb2 Compare March 21, 2021 19:15
@pruizj
Copy link
Copy Markdown
Author

pruizj commented Mar 29, 2021

No se que ha pasado que no me deja hacer bien el merge

@jgsogo
Copy link
Copy Markdown
Owner

jgsogo commented Apr 1, 2021

Está claro que los errores que han saltado en el CI no tienen que ver con tus cambios.

image

Esto tipo de cosas son algún error en Github, si vamos a la página web que informa de sus incidencias podemos ver qué algo pasó durante esos días: https://www.githubstatus.com/history, el 29 y 31 tuvieron problemas precisamente relacionados con las Github Actions, que es lo que yo estoy utilizando aquí para ejecutar el código.

Haz otro commit en esta rama, a ver si ya funciona correctamente. Thanks!

@jgsogo
Copy link
Copy Markdown
Owner

jgsogo commented Apr 1, 2021

No se que ha pasado que no me deja hacer bien el merge

Yo añado cambios en mi rama, tú añades en la tuya... y a veces no hay conflictos. Mergear no es tarea fácil:

merge

@github-actions
Copy link
Copy Markdown

Everything is OK. Thank you for the PR!

@jgsogo
Copy link
Copy Markdown
Owner

jgsogo commented Apr 11, 2021

Fallo tontorrón en la practica2, de hecho es más un typo en mi parte que en la tuya.

image


Puedes arreglarlo de dos formas:

  • Cambiando el nombre del fichero practicas/2021.practica2/src/ElementListaDoble.cpp por practicas/2021.practica2/src/ElementoListaDoble.cpp
  • Editando el CMakeLists.txt (justo esta línea que te enlazo), para que incluya tu fichero src/ElementListaDoble.cpp
  • (O bien te plantas, dices que es un error en mi lado y lo corrijo yo 😔 )

@github-actions
Copy link
Copy Markdown

Everything is OK. Thank you for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants