I'm looking for reviews for my code

[lukas9669]

I'm a C# beginner and I don't really know how good my program is.
I wrote a program that shows the user the weather at their location.
I would be very grateful if someone would read it and give me a rating.
you can find it here:

GitHub - LukasEber/WeatherApp: A simple weather app using 2 apis

A simple weather app using 2 apis. Contribute to LukasEber/WeatherApp development by creating an account on GitHub.

github.com

 

[Skydiver]

See documentation of HttpClient. Specifically:

HttpClient is intended to be instantiated once and re-used throughout the life of an application. Instantiating an HttpClient class for every request will exhaust the number of sockets available under heavy loads. This will result in SocketException errors.

 

[Skydiver]

Not specifically about your code, but in general, it's a bad idea to publish your API key. Yes, I know that it's more or less public data because it's visible in the HTTP traffic, but why make it easier for someone else to abuse the key registered under your name.

 

[lukas9669]

Not specifically about your code, but in general, it's a bad idea to publish your API key. Yes, I know that it's more or less public data because it's visible in the HTTP traffic, but why make it easier for someone else to abuse the key registered under your name.

Yeah, I know but I don´t know how to load it dynamic without publish it.
how can I do that?

 

[lukas9669]

See documentation of HttpClient. Specifically:

thanks, Now i did it like this:

 private static HttpClient client;
 static void GetCoordinatesFromApi(string city, string key)
        {
            client = new HttpClient();
            //some stuff
        }

and in the other method i use the same instance

 

[Skydiver]

You are still creating a new instance of HttpClient in:

client = new HttpClient();

What you want is something like:

private static HttpClient _client = new HttpClient();
:
static void GetCoordinatesFromApi(string city, string key)
{
    :
    HttpResponseMessage httpResponse = _client.GetAsync(requestUri).Result;
    :
}

 

[Skydiver]

Yeah, I know but I don´t know how to load it dynamic without publish it.
how can I do that?

See:

App.Config: Basics and Best Practices - SubMain Blog

In today's post, we introduce you to the app.config file by covering basic usage, what happens when compiling, and we give you a few other tips and tricks.

blog.submain.com

 

[Skydiver]

I would suggest a re-write to look something like this:

using System;
using System.Configuration;
using Newtonsoft.Json;

namespace WeatherApp
{
    public class Program
    {
        static HttpClient _client = new HttpClient();
        static string ApiKey => ConfigurationManager.AppSettings["ApiKey"];

        static void Main()
        {
            try
            {
                Console.WriteLine("Enter city: ");
                var city = Console.ReadLine();
                var coordinates = GetCityCoordinates(city);
                var weatherMapResponse = GetWeatherData(coordinates.Latitude, coordinates.Longitude);
                Console.WriteLine($"Temperature in {city} is {weatherMapResponse.Main.Temp} degrees.");
            }
            catch (Exception ex)
            {
                Console.WriteLine("An unhandled error has occured. Please check your inputs." + ex.Message);
            }
            Console.ReadKey();
        }

        static string GetApiResults(string request)
        {
            return _client.GetStringAsync(request).Result;
        }

        static CoordinatesResponse GetCityCoordinates(string? city)
        {
            var json = GetApiResults($"http://api.openweathermap.org/geo/1.0/direct?q={city}&limit=1&appid={ApiKey}&units=metric");
            return JsonConvert.DeserializeObject<List<CoordinatesResponse>>(json).First();
        }

        static WeatherMapResponse GetWeatherData(double lat, double lon)
        {
            var json = GetApiResults($"https://api.openweathermap.org/data/2.5/weather?lat={lat}&lon={lon}&appid={ApiKey}&units=metric");
            return JsonConvert.DeserializeObject<WeatherMapResponse>(json);
        }
    }
}

(Code above is untested. It's just me doodling in notepad.)

Some of the key changes:
1) Use the ConfigurationManager to pull out the API key from the app.config file. Expose this as a readonly property.
2) Rename the Lat and Lon members of CoordinateResponse class to be fully spelled out to comply with the .NET Framework naming recommendations of avoiding abbreviations.
3) Use HttpClient.GetStringAsync() to directly get back a string instead of pulling the string out of a HttpResponseMessage.
4) Minimize the use of class variables to those that actually are meaningful stateful variables, rather than using class variables with transient states.
5) Combine steps of call the REST API and then deserializing the data into one method instead of two separate methods. Yes this goes against the Single Responsibility Principle, but in this case the transient returned JSON string is not very useful. The deserialized data from that JSON string is of more value. Additionally, the extra method call made the code look more complex and just seemed to add visual and cognitive clutter.
6) Move all UI interaction into a single method.