7
\$\begingroup\$

I'm trying to make a simple app in C# that gets the weather data from http://openweathermap.org/api (OWM).

Here attached are two classes that I use to initialize WeatherData for selected City and to download and parse XML data from OWM API. I would like to know your opinion on the structure of the classes and the way I use them, because now I'm really not sure how should it work assuming that I would like to get/print more weather data in future.

I use them as follows:

WeatherData WarsawWeather = new WeatherData("Warsaw");
WarsawWeather.CheckWeather();
System.Console.WriteLine(WarsawWeather.Temp);

Classes:

using System.Net;
using System.Xml;

namespace WeatherApp
{
    class WeatherData
    {
        public WeatherData(string City)
        {
            city = City;
        }
        private string city;
        private float temp;
        private float tempMax;
        private float tempMin;

        public void CheckWeather()
        {
            WeatherAPI DataAPI = new WeatherAPI(City);
            temp = DataAPI.GetTemp();
        }

        public string City { get => city; set => city = value; }
        public float Temp { get => temp; set => temp = value; }
        public float TempMax { get => tempMax; set => tempMax = value; }
        public float TempMin { get => tempMin; set => tempMin = value; }
    }
    class WeatherAPI
    {
        public WeatherAPI(string city)
        {
            SetCurrentURL(city);
            xmlDocument = GetXML(CurrentURL);
        }

        public float GetTemp()
        {
            XmlNode temp_node = xmlDocument.SelectSingleNode("//temperature");
            XmlAttribute temp_value = temp_node.Attributes["value"];
            string temp_string = temp_value.Value;
            return float.Parse(temp_string);
        }

        private const string APIKEY = "API KEY HERE";
        private string CurrentURL;
        private XmlDocument xmlDocument;

        private void SetCurrentURL(string location)
        {
            CurrentURL = "http://api.openweathermap.org/data/2.5/weather?q=" 
                + location + "&mode=xml&units=metric&APPID=" + APIKEY;
        }

        private XmlDocument GetXML(string CurrentURL)
        {
            using (WebClient client = new WebClient())
            {
                string xmlContent = client.DownloadString(CurrentURL);
                XmlDocument xmlDocument = new XmlDocument();
                xmlDocument.LoadXml(xmlContent);
                return xmlDocument;
            }
        }
    }
}

Example XML from OWM:

<current>
<city id="756135" name="Warsaw">
<coord lon="21.01" lat="52.23"/>
<country>PL</country>
<sun rise="2016-12-27T06:45:16" set="2016-12-27T14:30:06"/>
</city>
<temperature value="2" min="2" max="2" unit="metric"/>
<humidity value="80" unit="%"/>
<pressure value="1019" unit="hPa"/>
<wind>
<speed value="6.7" name="Moderate breeze"/>
<gusts/>
<direction value="310" code="NW" name="Northwest"/>
</wind>
<clouds value="40" name="scattered clouds"/>
<visibility value="10000"/>
<precipitation mode="no"/>
<weather number="802" value="scattered clouds" icon="03n"/>
<lastupdate value="2016-12-27T20:00:00"/>
</current>
\$\endgroup\$
  • 1
    \$\begingroup\$ public float TempMax { get => tempMax; set => tempMax = value; } What is this syntax ? \$\endgroup\$ – Denis Dec 27 '16 at 21:25
  • 1
    \$\begingroup\$ That must be some new functionality my VS 2015 is whining about it. \$\endgroup\$ – Denis Dec 27 '16 at 21:31
  • 1
    \$\begingroup\$ @denis see Expression-bodied function members \$\endgroup\$ – t3chb0t Dec 28 '16 at 6:58
  • 2
    \$\begingroup\$ @t3chb0t That's a double expression bodied property, both the get and the setter are using expression body. It's different than what you are linking. \$\endgroup\$ – Denis Dec 28 '16 at 11:53
  • 1
    \$\begingroup\$ @denis not really, getter/setter are just a different way to express a method thus the same rule applies, expression-body \$\endgroup\$ – t3chb0t Dec 28 '16 at 11:57
4
\$\begingroup\$
    private float temp;
    private float tempMax;
    private float tempMin;

You should avoid abbreviating temperature. Temp is commonly used as an abbrevation for temporary and confuses here. In C# we don't usually use abbreviations. There are exceptions but they consider some well know ones like xml or html.


CurrentURL = "http://api.openweathermap.org/data/2.5/weather?q=" 
            + location + "&mode=xml&units=metric&APPID=" + APIKEY;

You use some of the new C# 6 features already like the bodyless methods => so you might want to do the same for strings. The above line will become:

_currentURL = 
    $"http://api.openweathermap.org/data/2.5/weather?q={location}" + 
    $"&mode=xml&units=metric&APPID={APIKEY}";

Using the var could make you code look much simpler.

Consider this

    using (WebClient client = new WebClient())
    {
        string xmlContent = client.DownloadString(CurrentURL);
        XmlDocument xmlDocument = new XmlDocument();
        xmlDocument.LoadXml(xmlContent);
        return xmlDocument;
    }

vs that

    using (var webClient = new WebClient())
    {
        var xmlContent = webClient.DownloadString(CurrentURL);
        var xmlDocument = new XmlDocument();
        xmlDocument.LoadXml(xmlContent);
        return xmlDocument;
    }

XmlDocument

There's a newer and an easer way to work with xml. Try the XDocument out which is LINQ-capable. You'll like it. See XDocument or XmlDocument

\$\endgroup\$
3
\$\begingroup\$

You don't need to explicitly declare a backing field for your properties

private string city;
public string City
{
    get { return city; }
    set { city = value; }
}

You can shorten it to this :

public string City { get; set; }

You should restrict your properties as much as possible, and remove the unnecessary ones

public string City { get; }
public float Temp { get; private set; }

Those are the only 2 properties you are using the others are redundant and as you can see I've added some extra modifiers to your properties now City is a readonly property while Temp has only a private setter

You have some conflicts between class members and parameters

private XmlDocument GetXML(string CurrentURL)
{
    using (WebClient client = new WebClient())
    {
        string xmlContent = client.DownloadString(currentURL);
        XmlDocument xmlDocument = new XmlDocument();
        xmlDocument.LoadXml(xmlContent);
        return xmlDocument;
    }
}

You already have variables with names xmlDocument && CurrentURL this might cause some weird behaviour, you should change they're names.

You are inconsistent in your naming :

private string CurrentURL;
private XmlDocument xmlDocument;

Both variables are private but one of them starts with capital letter, where usually only public members are following the Pascal case typing, they should both follow the Camel case typing.

Same goes for your parameters :

public WeatherData(string City)
{
    this.City = City;
}

public WeatherAPI(string city)
{
    SetCurrentURL(city);
    _xmlDocument = GetXML(_currentURL);
}

2 public constructors, one of them has a parameter with Pascal case typing, the other one is following the Camel case typing, instead they should both follow the Camel case typing for they're parameters.

In order to avoid confusion between parameters like currentURL && private class variables with the same name, you can add an underscore for your private class variables :

private string _currentURL;
private readonly XmlDocument _xmlDocument;

private XmlDocument GetXML(string currentURL)
{
    using (WebClient client = new WebClient())
    {
        string xmlContent = client.DownloadString(currentURL);
        XmlDocument xmlDocument = new XmlDocument();
        xmlDocument.LoadXml(xmlContent);
        return xmlDocument;
    }
}

Note that _xmlDocument is also marked as readonly which means you can only give it a value from the constructor.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for this answer. The naming of variables was always my problem, so I will stick to the Camel style (these Pascal style City, Temp... were automatically generated by VS2017, so I didn't change them). \$\endgroup\$ – Dominik Roszkowski Dec 27 '16 at 21:46
  • 1
    \$\begingroup\$ I've added some additional info about your properties. \$\endgroup\$ – Denis Dec 27 '16 at 21:47
3
\$\begingroup\$

I think you have your separation of responsibilities slightly wrong. For example, why are you tying each instance of a WeatherApi to a specific location? It seems to me that it would be better to pass the location in later. The name is also too concerned with the implementation. The caller shouldn't have to worry that the data is from an API.

One other issue with that class is that you do non-trivial work in the constructor. Generally, initialising an object should be cheap. I'd advise that you split this up so that the user creates a WeatherRepository and then calls a GetWeatherData method to actually perform the request.

var weatherRepository = new WeatherRepository();
var warsawData = weatherRepository.GetWeatherData("warsaw");
Console.WriteLine(warsawData.CurrentTemperature);

Note that I've also made GetWeatherData return your WeatherData class rather than populate an XmlDocument. That would be something like:

public WeatherData GetWeatherData(string location)
{
    var endpoint = GetUrlForLocation(location);
    var xmlData = GetXmlFrom(endpoint);
    // todo: implement this.
    return CreateDataFromXml(xmlDocument);
}

private XmlDocument GetXmlFrom(string endpoint)
{
    using (var webClient = new WebClient())
    {
        var xmlContent = webClient.DownloadString(endpoint);
        var xmlDocument = new XmlDocument();
        xmlDocument.LoadXml(xmlContent);
        return xmlDocument;
    }
}

If you create your classes to match the xml or add appropriate metadata, the CreateDataFromXml method might be as simple as using an XmlSerializer or DataContractSerializer to deserialize the data directly.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.