Fix static champion by ID

[hanscauwenbergh]

Changes:

• Use ID instead of name to fetch static champion info from DataDragon (e.g. https://ddragon.leagueoflegends.com/cdn/14.8.1/data/en_GB/champion/Wukong.json (403 Forbidden) -> https://ddragon.leagueoflegends.com/cdn/14.8.1/data/en_GB/champion/MonkeyKing.json)

[Nico-Mayer]

@hanscauwenbergh, I have some concerns about your proposed changes. The current implementation seems correct as we verify if the given ID matches any ID in the champions array. Then, we invoke the c.GetChampion function which requires a name argument, subsequently returning the correct champion. Your suggested alteration might potentially break this functionality.

One potential improvement we might consider is to avoid redundant calls to c.GetChampion. Fetching champion data twice seems unnecessary and could be optimized. However, I'd need to delve deeper into this to confirm.

[hanscauwenbergh]

@Nico-Mayer Mm, thanks for the reply. It looks to me the c.GetChampion(name string) is wrong as the DataDragon endpoint seems to expect the ID, not the name.

For Wukong (ID: MonkeyKing, name: Wukong, see link)GetChampion("Wukong") returns an error, while GetChampion("MonkeyKing") returns the expected data.

Shall I update the argument's name and update the call sides?

[Nico-Mayer]

@hanscauwenbergh Ah, I now see what you mean, and I think you're right. While the current implementation works for most champions, it will fail in the case with Wukong that you mentioned. Therefore, we need to update the argument names and possibly some existing tests. @KnutZuidema might be able to provide more insight on this.

[KnutZuidema]

@hanscauwenbergh does it always work with the Champion ID? I'm a bit fuzzy on the relationship between Key, Name and ID.
Before merging this I'd like to understand this a bit better and document it if possible.
Right now, the function GetChampionByID does not make a lot of sense: it matches the input parameter id on the field champion.Key and then uses the champion.ID to fetch a champion by name using GetChampion.

[Nico-Mayer]

@KnutZuidema @hanscauwenbergh This change would be a breaking one, so it might be better to remove the GetChampion function altogether. Instead, we can create three distinct functions: GetChampionByID, GetChampionByName, and GetChampionByKey. This would make their purposes clearer.

From what I understand, the relationship between Key, Name, and ID is as follows: the Key is an integer representing the champion's release order, the ID is a code name that can be similar to the Name field but dont has to be, and the name is the actual name of the champion.

[KnutZuidema]

Thank you for the explanation @Nico-Mayer 🙏🏼

I think the bug here is that I assumed that the DDragon endpoint would use the champion name as a parameter, but it uses the ID instead. That means the implementations of GetChampion and GetChampionByID should actually be the inverse of what they currently are:

• GetChampionByID should either return the cached value or fetch it directly from the DDragon API
• GetChampion should determine the ID from listing all champions and defer to GetChampionByID

[hanscauwenbergh]

@KnutZuidema Appreciate the input, I've updated the code according to your suggestion but let me know if it'd need any further changes 🙂