Skip to content

Conversation

@filipmarkovic064
Copy link

Hey first time using APIs so if possible please be very strict, comparing this to the industry standards for calling APIs, i feel like its very important to nail it down in the start so i dont build bad habits further down the path :D

Thanks in advance!

@TheCSharpAcademy
Copy link
Owner

@filipmarkovic064 Project approved! 😄✅ And well done for once again completing the challenges! I don't remember many people having completed the Favorite Drinks challenge 👏👏

Feedback: Overall you're using HttpClient well, passing it into the methods and making the whole thing async. But here are a few things:

🔍️The biggest issue is that you want to URL encode parameters to make sure special characters don't do funny things:
image

🔍️Error handling could be better. At the moment you're only catching JsonExceptions. I believe it wouldn't handle for Http error codes (which would be the majority of exceptions).

🔍️This is more related to architecture but you're mixing HTTP stuff with UI stuff. You want to separate them so each method is easier to test. Example: GetDrinkDetails calls Console.ReadyKey and UserInterface.MainMenu(), that makes your method impossible to test. I recommend you look into our unit tests article/tutorial asap because writing tests will force you to write more modular code.

🔍️I'd use appsettings.json instead of App.config, as .NET/C# has moved away from it. I've encouraged app.config when first created the academy but that was when .NET 6 was just released and there were still a lot of .NET Framework 4.x out there, but now I'd focus on the new way of doing things.

Keep up the good work!✋🏻Looking forward to seeing your next projects!

@filipmarkovic064
Copy link
Author

filipmarkovic064 commented Jan 5, 2026

@TheCSharpAcademy Thank you for the amazing feedback! I will make sure to fix those things!
When you say that I should URL Encode Parameters, does it mean that i should for example use the following: (Using the image you posted as an example)

var DrinkUrl = EscapeDataString(DrinkInfo);
var url = $"https://www.thecocktaildb.com/api/json/v1/1/search.php?s={DrinkUrl}";

Instead of directly inputting the parameter link?

AppSettings.json seems like such a pain to setup but i will be using that one in the future, i didnt know that .NET is moving away from App.config, thanks for the heads up! :D

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