myCSharp.de - DIE C# und .NET Community
Willkommen auf myCSharp.de! Anmelden | kostenlos registrieren
 
 | Suche | FAQ

» Hauptmenü
myCSharp.de
» Startseite
» Forum
» FAQ
» Artikel
» C#-Snippets
» Jobbörse
» Suche
» Regeln
» Wie poste ich richtig?
» Forum-FAQ

Mitglieder
» Liste / Suche
» Wer ist wo online?

Ressourcen
» openbook: Visual C#
» openbook: OO
» Microsoft Docs

Team
» Kontakt
» Übersicht
» Wir über uns

» myCSharp.de Diskussionsforum
Du befindest Dich hier: Community-Index » Diskussionsforum » Entwicklung » Code-Reviews » Parser-Methode Lesbarkeit verbessern
Letzter Beitrag | Erster ungelesener Beitrag Druckvorschau | Thema zu Favoriten hinzufügen

Antwort erstellen
Zum Ende der Seite springen  

Parser-Methode Lesbarkeit verbessern

 
Autor
Beitrag « Vorheriges Thema | Nächstes Thema »
R3turnz R3turnz ist männlich
myCSharp.de-Mitglied

Dabei seit: 03.01.2016
Beiträge: 125
Entwicklungsumgebung: Visual Studio 2015
Herkunft: Süddeutschland


R3turnz ist offline

Parser-Methode Lesbarkeit verbessern

Beitrag: beantworten | zitieren | editieren | melden/löschen       | Top

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

C#-Code:
            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?
04.10.2016 20:18 Beiträge des Benutzers | zu Buddylist hinzufügen
MrSparkle MrSparkle ist männlich
myCSharp.de-Team

avatar-2159.gif


Dabei seit: 16.05.2006
Beiträge: 5.561
Herkunft: Leipzig


MrSparkle ist online

Beitrag: beantworten | zitieren | editieren | melden/löschen       | Top

Hi R3turnz,

statt

C#-Code:
weatherPointsAtDays.Select(weatherPointsAtDay =>
            {
                Weather weatherAtDay = new Weather();
                weatherAtDay.Condition =  // usw.
                return weatherAtDay;
            }

könntest du auch einfach schreiben:

C#-Code:
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.
04.10.2016 21:37 Beiträge des Benutzers | zu Buddylist hinzufügen
chilic
myCSharp.de-Poweruser/ Experte

Dabei seit: 12.02.2010
Beiträge: 2.056


chilic ist offline

Beitrag: beantworten | zitieren | editieren | melden/löschen       | Top

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?

Zitat von R3turnz:
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.
04.10.2016 21:38 E-Mail | Beiträge des Benutzers | zu Buddylist hinzufügen
Deaktiviertes Profil Deaktiviertes Profil ist männlich
myCSharp.de-Mitglied

Dabei seit: 06.07.2014
Beiträge: 985


Deaktiviertes Profil ist offline

Beitrag: beantworten | zitieren | editieren | melden/löschen       | Top

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.

Dieser Beitrag wurde 2 mal editiert, zum letzten Mal von Deaktiviertes Profil am 04.10.2016 22:20.

04.10.2016 22:19 Beiträge des Benutzers | zu Buddylist hinzufügen
unconnected unconnected ist männlich
myCSharp.de-Mitglied

avatar-3200.jpg


Dabei seit: 13.08.2006
Beiträge: 849
Entwicklungsumgebung: VS2017 Enterprise,VS Code
Herkunft: Oerlinghausen/NRW


unconnected ist offline

Beitrag: beantworten | zitieren | editieren | melden/löschen       | Top

Zitat:
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:

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

gegen das

C#-Code:
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".
05.10.2016 06:59 E-Mail | Beiträge des Benutzers | zu Buddylist hinzufügen
LaTino LaTino ist männlich
myCSharp.de-Poweruser/ Experte

avatar-4122.png


Dabei seit: 03.04.2006
Beiträge: 3.001
Entwicklungsumgebung: Rider / VS2019 / VS Code
Herkunft: Thüringen


LaTino ist offline

Beitrag: beantworten | zitieren | editieren | melden/löschen       | Top

Zitat:
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
05.10.2016 07:38 E-Mail | Beiträge des Benutzers | zu Buddylist hinzufügen
Palin Palin ist männlich
myCSharp.de-Mitglied

Dabei seit: 22.08.2011
Beiträge: 1.090
Entwicklungsumgebung: VB.net


Palin ist offline

Beitrag: beantworten | zitieren | editieren | melden/löschen       | Top

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)

C#-Code:
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.

C#-Code:
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).

C#-Code:
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.

C#-Code:
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.)

C#-Code:
        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.
05.10.2016 11:56 E-Mail | Beiträge des Benutzers | zu Buddylist hinzufügen
R3turnz R3turnz ist männlich
myCSharp.de-Mitglied

Dabei seit: 03.01.2016
Beiträge: 125
Entwicklungsumgebung: Visual Studio 2015
Herkunft: Süddeutschland

Themenstarter Thema begonnen von R3turnz

R3turnz ist offline

Beitrag: beantworten | zitieren | editieren | melden/löschen       | Top

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:

C#-Code:
    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
    }
05.10.2016 16:26 Beiträge des Benutzers | zu Buddylist hinzufügen
LaTino LaTino ist männlich
myCSharp.de-Poweruser/ Experte

avatar-4122.png


Dabei seit: 03.04.2006
Beiträge: 3.001
Entwicklungsumgebung: Rider / VS2019 / VS Code
Herkunft: Thüringen


LaTino ist offline

Beitrag: beantworten | zitieren | editieren | melden/löschen       | Top

Zitat von R3turnz:
@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:

C#-Code:
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
05.10.2016 16:53 E-Mail | Beiträge des Benutzers | zu Buddylist hinzufügen
chilic
myCSharp.de-Poweruser/ Experte

Dabei seit: 12.02.2010
Beiträge: 2.056


chilic ist offline

Beitrag: beantworten | zitieren | editieren | melden/löschen       | Top

Zitat von unconnected:
Zitat:
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

C#-Code:
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.
14.10.2016 05:47 E-Mail | Beiträge des Benutzers | zu Buddylist hinzufügen
Baumstruktur | Brettstruktur       | Top 
myCSharp.de | Forum Der Startbeitrag ist älter als 4 Jahre.
Der letzte Beitrag ist älter als 4 Jahre.
Antwort erstellen


© Copyright 2003-2020 myCSharp.de-Team | Impressum | Datenschutz | Alle Rechte vorbehalten. | Dieses Portal verwendet zum korrekten Betrieb Cookies. 23.11.2020 18:02