Laden...

Parser-Methode Lesbarkeit verbessern

Erstellt von R3turnz vor 7 Jahren Letzter Beitrag vor 7 Jahren 4.928 Views
R
R3turnz Themenstarter:in
125 Beiträge seit 2016
vor 7 Jahren
Parser-Methode Lesbarkeit verbessern

Hallo,
das hier ist der bisherige Code einer Parser-Methode:


            var weatherPointsAtDays = JObject.Parse(queryResponse)["list"].GroupBy((timePoint) => (UnixTimeToDateTime(timePoint.Value<string>("dt"))).Date);
            weatherPointsAtDays.Select(weatherPointsAtDay =>
            {
                Weather weatherAtDay = new Weather();
                weatherAtDay.Condition = (WeatherCondition)Enum.Parse(typeof(WeatherCondition), weatherPointsAtDay.OrderBy((timePoint) => weatherPointsAtDay.Count(tP => timePoint["weather"].First.Value<string>("main") == tP["weather"].First.Value<string>("main"))).Last()["weather"].Value<string>("main"));
                weatherAtDay.Temperature = weatherPointsAtDay.Average(timePoint => timePoint["main"].Value<double>("temp"));
                weatherAtDay.MinTemperature = weatherPointsAtDay.Min(timePoint => timePoint["main"].Value<double>("temp"));
                weatherAtDay.MaxTemperature = weatherPointsAtDay.Max(timePoint => timePoint["main"].Value<double>("temp"));
                weatherAtDay.TempUnit = unit;
                return weatherAtDay;
            }
            );

Ich erhalte 3-stündige Wettervorhersagen und möchte aber mit jedem Objekt einen ganzen Tag darstellen.
Ich bin ziemlich unzufrieden mit der Lesbarkeit und der Übersicht. Wo könnte ich da etwas ändern?

5.657 Beiträge seit 2006
vor 7 Jahren

Hi R3turnz,

statt

weatherPointsAtDays.Select(weatherPointsAtDay =>
            {
                Weather weatherAtDay = new Weather();
                weatherAtDay.Condition =  // usw.
                return weatherAtDay;
            }

könntest du auch einfach schreiben:

weatherPointsAtDays.Select(weatherPointsAtDay => new Weather()
{
   Condition =  // usw.      
})

Ansonsten fällt mir auf, daß du deine Daten nicht validierst, und jedesmal eine Exception geworfen wird, wenn eine Parse-Methode fehlschlägt, oder ein Objekt mit den Keys "list"/"weather"/"main" nicht vorhanden ist.

Übersichtlichkeit ist halt immer relativ. Lesbarkeit und Testbarkeit finde ich persönlich wichtiger. Wenn du gut wartbaren Code haben willst, dann kannst du z.B. die Berechnungen der Min-, Max- und Durchschnittstemperaturen usw. in eigene Methoden auslagern, die du auch einzeln testen kannst.

Weeks of programming can save you hours of planning

C
2.121 Beiträge seit 2010
vor 7 Jahren

Lambdaausdrücke sind gut um etwas schnell zu schreiben, aber es wird auch entsprechend unübersichtlich wie du siehst.
Die Zeile weatherAtDay.Condition = .... sowas denkst du dir aus und wunderst dich dann dass du selbst nichts mehr verstehst?

Wo könnte ich da etwas ändern?

Lass überlegen. Vorne anfangen. Hinten aufhören.

Erstens timePoint => timePoint["main"].Value<double>("temp") ließe sich zum Beispiel herausziehen.
Zweitens obige Zeile entschärfen!
Drittens wenn jemand sagt Lambda ist neu, Lambda ist cool, immer alles nur noch damit, druck deinen Parser aus und hau ihm das auf die Nase.

D
985 Beiträge seit 2014
vor 7 Jahren

Ich würde aus dem Response erstmal eine Liste der Messwerte erstellen.

Dann per Enumerable.Group und Aggregate die Tageswerte erzeugen. Das wird dann erheblich übersichtlicher.

849 Beiträge seit 2006
vor 7 Jahren

Lambdaausdrücke sind gut um etwas schnell zu schreiben, aber es wird auch entsprechend unübersichtlich wie du siehst.

Wenn man um die Parser Code Zeilen ein foreach schreibt, siehts auch nicht anders aus. Das hat nichts mit Lambda zu tun. Wenn man das Parsen in eine Methode auslagern würde würdest Du das:


var result = new List<Weather>()
foreach(var weatherPointsAtDay in weatherPointsAtDays)
{
result.Add(Parse(weatherPointsAtDay)); 
}

gegen das


var result = weatherPointsAtDays.Select(weatherPointsAtDay => Parse(weatherPointsAtDay)).ToList();

(ToList() hier nur um das Ergebnis gleich zu halten)
stellen.
Ich finde hier die Lambda Ausdrücke durchaus angebracht, und auch schöner. Aber das ist Ansichtssache. Fakt ist das um schönen Code zu schreiben, der Parse Teil aus dem Schleifen Body raus muss. Und auch diesen würde ich nochmal wie chilic schon sagte nochmal "entschärfen".

3.003 Beiträge seit 2006
vor 7 Jahren

Ich bin ziemlich unzufrieden mit der Lesbarkeit und der Übersicht. Wo könnte ich da etwas ändern?

Lambda hin oder her, JObject.Parse halte ich für dein Problem. Erzeug eine Klasse und deserialisier das lieber vernünftig, anstatt die Werte einzeln von Hand zu interpretieren. Dafür gibt's die Mechanik JsonConverter in Newtonsoft.Json. Ist erstens schneller und zweitens zuverlässiger, als so herumzustochern. Und letzten Endes geht es dir um eine Konvertierung, der Code dafür hat mMn an der Stelle eigentlich nichts zu suchen.

LaTino

"Furlow, is it always about money?"
"Is there anything else? I mean, how much sex can you have?"
"Don't know. I haven't maxed out yet."
(Furlow & Crichton, Farscape)

P
1.090 Beiträge seit 2011
vor 7 Jahren

Ich hab jetzt mal eine Grobe Überarbeitung gemacht.
Grundlegend basiert sie auch einen Vorschlag von Steve McConnell aus seinem Buch Code Complete, die Klassen und Methoden grob wie ein Fach) Buch zu Strukturieren. Das Buch stellt dabei die Klasse da (ist hier im Beispiel nicht vorhanden). Das Inhaltsverzeichnis (grob) den Einstieg. Dort findet man in der Regel die Einzelnen Kapitel mit einer Möglichst sinnvollen Bezeichnung, damit man versteht um was es in den Kapiteln geht. Die sind dann Teilweise noch in Abschnitten Unterteil, die wiederum einen Groben überblicke geben um was es geht. Und erst in den Abschnitten geht es um den eigentlichen Inhalt.

Für dein Beispiel ist es Wahrscheinlich ein bisschen zu viel Overhead. Wie weit man die Sacher herunter bricht muss man selbst wissen / ausprobieren.

Grundlegen macht der Code ja erst mal 2 Sachen, wetherPointsAtDay erzeugen und daraus das WeatherAtDay.

Sähe dann so aus (ja man kann eine Bessere Benennung finden)


var weatherPointsAtDays = GetWeatherPointsAtDays(queryResponse);

var dailyWeather = weatherPointsAtDays.Select(weatherPointsAtDay => CreateWeatherAtDay(weatherPointsAtDay));     

Grob kann jetzt jemand der deinen Quellcode liest sehen, das du in der 1. Methode aus dem queryResponse dir die weather PointsAtDays erzeugst und danach aus ihnen das dailyWeather.

GetWeatherPointsAtDays ist da nicht so interessant, es verbiergt nur den folgenden Aufruf.

 private IEnumerable<WeatherPoint> GetWeatherPointsAtDays(Object queryResponse)
        {
            return JObject.Parse(queryResponse)["list"].GroupBy((timePoint) => (UnixTimeToDateTime(timePoint.Value<string>("dt"))).Date);
        }

CreateWeatherAtDay hab teils noch mal weiter herunter gebrochen (was nicht unbedingt nötig ist).

private Weather CreateWeatherAtDay(IEnumerable<WeatherPoint> weatherPointsAtDay)
        {
            new Weather();
            weatherAtDay.Condition = [B]GetWeatherCondition(weatherPointsAtDay);[/B]
            weatherAtDay.Temperature = [B]GetAverageTemprature(weatherPointsAtDay);[/B]
            weatherAtDay.MinTemperature = weatherPointsAtDay.Min(timePoint => timePoint[MAIN_POINT].Value<double>(TEMP));
            weatherAtDay.MaxTemperature = weatherPointsAtDay.Max(timePoint => timePoint[MAIN_POINT].Value<double>(TEMP));
            weatherAtDay.TempUnit = unit;
            return weatherAtDay;
        }

Die Methoden GetWeatherCondition und GetAverageTemprature sind jetzt nur ein Beispiel das man es dann auch noch weiter herunter brechen kann.

Hier mal grob deren Implementierung.

 private WeatherCondition GetWeatherCondition(IEnumerable<WeatherPoint>  weatherPointsAtDay)
        {
            return (WeatherCondition)Enum.Parse(typeof(WeatherCondition), weatherPointsAtDay.OrderBy((timePoint) => weatherPointsAtDay.Count(tP => timePoint[WEATHER].First.Value<string>(MAIN_POINT) == tP[WEATHER].First.Value<string>(MAIN_POINT))).Last()[WEATHER].Value<string>(MAIN_POINT));
        }

        private double GetAverageTemprature(IEnumerable<WeatherPoint> weatherPointsAtDay)
        {
            return weatherPointsAtDay.Average(timePoint => timePoint[MAIN_POINT].Value<double>(TEMP));
        }

Wie du siehst hab ich ein paar der Strings durch Konstanten ersetzt. (Die Namen sind noch nicht wirklich gut.)

  
        private const String MAIN_POINT = "main";
        private const String TEMP = "temp";
        private const String WEATHER = "weather";

Grundlegend geht es ja darum, deinen Kollegen mit deinem Code zu vermitteln, was du macht. Fang mit einer Groben Beschreibung an. "Ich erzeuge aus Wetter Daten, eine Tägliche Zusammenstellung".
Und gehe dann ins Detail. "Dafür ....".

Wie gesagt für, das Beispiel ist es sicher Übertrieben.

In der Richtung Strukturire ich aber Grob meinen Quellcode. Wobei da natürlich noch andere Klassen usw. beteiligt sein können.

Sollte man mal gelesen haben:

Clean Code Developer
Entwurfsmuster
Anti-Pattern

R
R3turnz Themenstarter:in
125 Beiträge seit 2016
vor 7 Jahren

Ersteinmal Danke für die Antworten!
@LaTino Ich habe bisher deseralisert, dass hat aber ein größeres Klassenkonstrukt erzeugt, in denen manche nur eine Eigenschaft hatten.
Dies ist mein verbesserter Converter:


    public static class WeatherParser
    {
        #region fields
        private const string climaticDataName = "main";
        private const string conditionName = "weather";
        private const string temperatureName = "temp";
        private const string minTemperatureName = "temp_min";
        private const string maxTemperatureName = "temp_max";
        #endregion
        #region methods
        private static DateTime UnixTimeToDateTime(string text) { ... }
        public static Tuple<string,string> ParseLocation(string queryResponse) { ... }
        public static Weather ParseCurrentWeather(string queryResponse, TemperatureUnit unit) { ... }
        #region forecast
        public static IDictionary<DateTime,Weather> ParseForecast(string queryResponse,TemperatureUnit unit)
        {
            var weatherPointsAtDays = GetTimePointsAtDay(queryResponse);
            return weatherPointsAtDays.Select(weatherPointsAtDay => new
            {
                Date = weatherPointsAtDay.Key,
                Weather = new Weather()
                {
                    Condition = GetConditionAtDay(weatherPointsAtDay),
                    Temperature = weatherPointsAtDay.Average(weatherPoint => weatherPoint[climaticDataName].Value<double>(temperatureName)),
                    MinTemperature = weatherPointsAtDay.Min(weatherPoint => weatherPoint[climaticDataName].Value<double>(temperatureName)),
                    MaxTemperature = weatherPointsAtDay.Max(weatherPoint => weatherPoint[climaticDataName].Value<double>(temperatureName)),
                    Humidity = weatherPointsAtDay.Average(weatherPoint => weatherPoint[climaticDataName].Value<double>())
                }
            }
            ).ToDictionary((weatherAtDay) => weatherAtDay.Date, (weatherAtDay) => weatherAtDay.Weather);
        }
        private static WeatherCondition GetConditionAtDay(IEnumerable<JToken> weatherPointsAtDay)
        {
            var weatherConditions = Enum.GetValues(typeof(WeatherCondition)).Cast<WeatherCondition>();
            return weatherConditions.OrderBy((condition) => weatherPointsAtDay.Count((timePoint) => ((WeatherCondition)Enum.Parse(typeof(WeatherCondition), timePoint[condition].First.Value<string>("main"))) == condition)).Last();
        }
        private static string GetConditionDescriptionAtDay(IEnumerable<JToken> weatherPointsAtDay)
        {
            var descriptions = weatherPointsAtDay.Select(weatherPoint => weatherPoint[conditionName].Value<string>("description")).Distinct();
            return descriptions.OrderBy(description => weatherPointsAtDay.Count((weatherPoint) => weatherPoint[conditionName].Value<string>("description") == description)).Last();
        }
        private static IEnumerable<IGrouping<DateTime, JToken>> GetTimePointsAtDay(string queryResponse)
        {
            return JObject.Parse(queryResponse)["list"].GroupBy((timePoint) => (UnixTimeToDateTime(timePoint.Value<string>("dt"))).Date);
        }
        #endregion
        #endregion
    }

3.003 Beiträge seit 2006
vor 7 Jahren

@LaTino Ich habe bisher deseralisert, dass hat aber ein größeres Klassenkonstrukt erzeugt, in denen manche nur eine Eigenschaft hatten.

Ah, kleiner Denkfehler, glaube ich. Leite eine Klasse von JsonConverter ab. Dieser neue Converter liefert nicht ein 1:1-Abbild des Objekts des kompletten JSON, sondern ausschließlich eine Sammlung der Eigenschaften, die dich interessieren. Wirf den ganzen Datenmüll, den du nicht brauchst, raus, noch ohne zu aggregieren (Aggregation - zusammenfassen nach Eigenschaften, also beispielsweise alles, wo du ein Min/Max machst). Also immer noch eine Liste von Datenpunkten. Die kannst du durchaus geschickt so vorbereiten, dass das Zusammenfassen einzelner Punkte zu einem Tag einfacher wird.

Danach ist es so etwas hier:


var myDatapoints = JsonConvert.Deserialize<DatapointList>(json, new MyConverter());
//DatapointList kann zB eine IList-Implementierung sein, inkl. Methoden wie GetByDay(DateTime) - wie du möchtest.

Und jetzt kannst du über diese Sammlung deine Linq-Aggregationen laufen lassen. Dann ist deine Deserialisierung sauber getrennt (bspw. wirst du die Typangaben im Value<> los, und auch die Namensangaben der Properties) vom Arbeiten mit den Daten. Und dafür brauchst du nicht eine komplette Objektrepräsentation des JSON mit -zig Klassen.

Grüße,

LaTino

"Furlow, is it always about money?"
"Is there anything else? I mean, how much sex can you have?"
"Don't know. I haven't maxed out yet."
(Furlow & Crichton, Farscape)

C
2.121 Beiträge seit 2010
vor 7 Jahren

Lambdaausdrücke sind gut um etwas schnell zu schreiben, aber es wird auch entsprechend unübersichtlich wie du siehst.

Wenn man um die Parser Code Zeilen ein foreach schreibt, siehts auch nicht anders aus...

Ich meinte vor allem dieses hier

weatherPointsAtDay.OrderBy((timePoint) => weatherPointsAtDay.Count(tP => timePoint["weather"].First.Value<string>("main") == tP["weather"].First.Value<string>("main"))).Last()["weather"].Value<string>("main"));

Wenns einfacher wird nimmt man natürlich etwas einfaches. Aber irgendwann wird es vor lauter vereinfachen wieder komplexer. Da sollte man dann aufhören.