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

Corrección #1

Open
ferrero-felipe opened this issue Aug 20, 2021 · 0 comments
Open

Corrección #1

ferrero-felipe opened this issue Aug 20, 2021 · 0 comments

Comments

@ferrero-felipe
Copy link

¡Olé Dayne! 



Muy buen trabajo, como ya hemos visto en tu presentación. La corrección será de la siguiente manera, revisaré todo tu código y iré comentando una série de cosas que vaya viendo. Al final, haré apuntes generales.

Buen trabajo en la organización del repo y el código en él.

Cuando organizas los datos y haces el webscraping haces algunas cosas no-ortodoxas… 🙃 Me refiero a esa función:

def player_list(x):
    final_res=[]
    new=[]
    for data in x.values():
        data=str(data); data.split(',')
        new.append(data)
    
        for d in new:
            d=d.replace("{'Goalkeepers': ' ",'').replace(" 'Defenders': ' ",'').replace("'Defenders':",'').replace('Defenders: " ','').replace(" 'Midfielders': ' ",'').replace("'Midfielders': ",'').replace(" 'Forwards': ' ",'').replace("'","").replace("}","").replace('"','')
            d=(re.sub(r" \(.*?\)","",d))
            d=d.split(',')
            d_=[]
            for y in d:
                d_.append(y.strip())
            final_res.append(d_)   
            new=[]
    return final_res

El convertir un dicionário a string es un poco raro y por eso tienes que hacer ese montón de replaces para librarse de las keys. Quizás quisieras haber hecho un .values().

Otra cosa es que, por la manera como están identados, a cada elemento data in x.values(), añades más cosas a new y iteras por new. Parece que está mal porque iteraria múltiples veces por los primeros elementos. No lo es porque vienes a redefinir new=[] en el siguiente bucle. Eso también es raro por su vez, pues define la lista vacía en un bucle sobre esa propria lista. (No es un problema sintático, pues el iterado se crea al princípio), pero es un problema semántico. Se le un poco raro. Quizás seria mejor definir new al principio del primer bucle:

for data in x.values():
    new=[]
    data=str(data); data.split(‘,’)
…

Pero si pensamos en más detalle, new siempre va a tener un único elemento, ¿no?

Entonces ni siquiera hace falta el segundo bucle. Podría ser d=data.replace…

Entiendes?

Genial incluir un jupyter demonstrando la API. ;)

La API está perfecta, todo bien organizado, bien modulado, como debe ser. Buen trabajo! Hay algunos detalles como funciones que son un poquito redundantes y quizás podrían ser una sola con más parámetros, pero seria ya demasiada minúcia.

Digo lo mismo sobre el dashboard en streamlit. Ese tiene sus limitaciones, lo que por ejemplo hace que para espaciar hagamos cosas como:

st.text('')
st.text('')
st.text('')
st.text('')
st.text('')
st.text('')

En ese caso buscar un poquito de información sobre como añadir un elemento HTML puede ser útil. :)

En /streamlit/pages/matches.py, que tienes 2 copias de cada variable, una referiente a cada equipo, podría ser interesante ver eso organizado como listas, dicionarios, etc. Aprovechando las estructuras de python. Pero más que eso, una reestructuración estaria muy bien en players.py para evitar todos esos ifs. Un primer paso seria derivar toda esa generación de querrá a una otra función. :)

Gran trabajo, Dayne! Espero que estés orgulloso de lo que has hecho. Realmente es un trabajo de mucha calidad para alguien que apenas ha empezado a programar, muy crack. KUTGW

image

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

No branches or pull requests

1 participant