HttpClient causes memory leak(s)

[brianebeling]

You create a new client every time someone clicks the button to assign runes, but you don't dispose it. I'd advice you to use it like this: using (var httpClient = new HttpClient()) { ... }

to automatically dispose it after usage. (You also create multiple HttpClients when you only need one, maybe you want to create one at the start of your program and keep it until your program ends!)

Some smaller things I also noticed:

• File names are partly mismatching with class names
• Multiple classes per file

If you want to scale your program to become much bigger some day, let's say on a scale like Championify + your Rune function, it would make sense to split things a bit more up.

I hope that helped a bit, have a wonderful day and thanks for your tool!

[wildbook]

When it comes to HttpClient usage the second idea of creating one and reusing it is how it's supposed to be done. There's an article here explaining why using(var client = new HttpClient()) is not the right thing to do, but it basically boils down to "it keeps sockets open even after being disposed, which is not good".

I agree with the other thoughts in the comment above too, including the thanks \:)

Great job, and nice to see you using the LCU api and not just doing screen reading and clicking like most others seem to do.

Good luck with this project in the future!

[brianebeling]

I learned something new, thank you @wildbook! It really is unfortunate that it is so missleading and encourages the wrong "usage".

[Fumi24]

Yep differently looking at those for some updates, i dont wanna throw out updates everyday, not before i have an auto updater which im looking to implement in the next 2 weeks time.

[wildbook]

(Note, I dislike auto-updaters a bit because I want to know if a program changes, before it does) You should probably consider if an auto-updater is needed instead of just adding a notification that there is an update. If you want to have an auto-updater, make sure to make it optional to update. (Maybe even opt-in?)

In Riot's response to Ace one of the reasons mentioned for shutting it down was because it had full permissions on the PC it ran on and could automatically update. It's obviously far from the only reason it was taken down, but it's a valid point nevertheless.

[Fumi24]

I'll probably make a notification as i dont feel confident enough in making an acutal updater, probably just gonna check if the release number on github == v1.2 as an example

[Fumi24]

Fixed in todays commit so closing.