Chcę napisać scrapera - czy dobrze go zaprojektowałem?

0

Chciałbym napisać scrapera, który pobierałby mi dane z jakieś strony. Obrazłem za cel Allegro. Najpierw chcę go napisać tak, by nie korzystał z Api, tylko parsował HTML.

-ShopFetcher będzie odpowiadać za pobieranie danych i zwracanie aukcji. Nie będzie ona przechowywać żadnych informacji, więc jest oznaczona jako static.

-ShopXPaths odpowiada za wyrażenia xPath do poszczególnych fragmentów podstrony. Pomyślałem, że logicznie będzie wydzielić te informacje do innej klasy (i przestrzeni nazw)

-Models odpowiada za modele obiektów, które będę zwracał (np. aukcja, użytkownik)

-StringExtensions zawiera rozszerzenia, które mogą się przydać do parsowania tekstu

Jeśli chodzi o kod, to wygląda to tak:

namespace ShopSharp
{
    static class ShopFetcher
    {
        // Setup the configuration to support document loading
        private static readonly IConfiguration DefultConfig = Configuration.Default.WithDefaultLoader();

        public static async Task<Auction> GetAuctionAsync(string url)
        {
            // Asynchronously get the document in a new context using the configuration
            var document = await BrowsingContext.New(DefultConfig).OpenAsync(url);

            // Parse auction data
            var title = document.QuerySelector(ShopXPaths.Title).TextContent.Trim();
            var price = document.QuerySelector(ShopXPaths.Price).Attributes["data-price"].Value;
            var quantity = document.QuerySelector(ShopXPaths.Quantity).TextContent.GetDigit();

            // Create auction object using DTO Model
            var auction = new Auction()
            {
                Topic = title,
                Price = decimal.Parse(price, CultureInfo.InvariantCulture),
                Quantity = quantity,
                Url = url
            };

            return auction;
        }
    }
}

namespace ShopSharp.Helpers
{
    static class ShopXPaths
    {
        public const string Title = "h1.title";
        public const string Price = "div.price";
        public const string Quantity = "span.no-wrap";
    }
}

namespace ShopSharp.Models
{
    class Auction
    {
        public string Url { get; set; }
        public string Topic { get; set; }
        public decimal Price { get; set; }
        public int Quantity { get; set; }
    }
}

namespace ShopSharp.Extensions
{
    static class StringExtensions
    {
        public static int GetDigit(this string str)
        {
            var m = Regex.Match(str, @"\d+");
            if (m.Success)
            {
                var parsed = int.TryParse(m.Value, out var number);
                if (parsed)
                    return number;
            }
            throw new Exception();
        }
    }
}

I przykładowe wykorzystanie:

private async void buttonScrape_ClickAsync(object sender, EventArgs e)
{
    string url = textBoxUrl.Text;
    if (string.IsNullOrEmpty(url))
        return;
    try
    {
        var auction = await ShopFetcher.GetAuctionAsync(url);
        FillTextBoxWithAuction(auction);
    }
    catch (Exception exception)
    {
        MessageBox.Show(exception.Message, "Wystąpił błąd!");
    }   
}

private void FillTextBoxWithAuction(Auction auction)
{
    var sb = new StringBuilder();

    sb.Append("Tytuł: ").Append(auction.Topic).Append(Environment.NewLine);

    richTextBoxResult.Text = sb.ToString();
}

Mam natomiast kilka pytań:

  1. Czy metodę GetDigit() powinienem użyć tak jak zrobiłem, czy przy: Quantity = quantity.GetDigit(),

  2. Czy metodę decimal.Parse powinienem użyć tak jak zrobiłem, czy przy tak jak zrobiłem z GetDigit(), czyli na końcu metody.

  3. Jeśli aukcja nie ma np. ceny, to po dojechaniu na var price = document.QuerySelector.... wyrzuci wyjątek object reference not set to an instance of an object. Czy w takim razie za każdym razem powinienem zrobić coś takiego:

string price;
try
{
    price = document.QuerySelector(ShopXPaths.Price).Attributes["data-price"].Value;
}
catch (Exception)
{
    throw new Exception("Price is missing");
}

czy może:

string price;
if (document.QuerySelector(ShopXPaths.Price).Attributes["data-price"].Value != null)
    price = document.QuerySelector(ShopXPaths.Price).Attributes["data-price"].Value;
else
    throw new Exception("Price is missing")
  1. w buttonScrape_ClickAsync najpierw sprawdzam, czy textbox nie jest pusty. Czy jest to ok? Może nie powinienem wprowadzać walidacji w obsłudze zdarzenia przycisku, a zamiast tego w metodzie GetAuctionAsync. Wtedy wyglądałoby to tak:
public static async Task<Auction> GetAuctionAsync(string url)
{
    if (string.IsNullOrEmpty(url))
		throw new Exception("Wrong url")
	// reszta kodu
}

a może walidacja tu i tu?

Macie może jeszcze jakieś wskazówki jak mógłbym polepszyć swój kod? Coś zrobiłem nie tak?

0

Wpadłem na jeszcze taki pomysł, by obsłużyć wyjątki:

        public static async Task<Auction> GetAuctionAsync(string url)
        {
            if (string.IsNullOrEmpty(url))
                throw new Exception("Url is empty");

            var document = await BrowsingContext.New(DefultConfig).OpenAsync(url);

            var title = TryFetch(document, ShopXPaths.Title, "Title is missing"); // ZMIENIONO
            var price = TryFetch(document, ShopXPaths.Price, "Price is missing"); // ZMIENIONO
            var quantity = TryFetch(document, ShopXPaths.Quantity, "Quantity is missing"); // ZMIENIONO

            var auction = new Auction()
            {
                Topic = title.TextContent.Trim(),
                Price = decimal.Parse(price.Attributes[ShopAttributes.Price].Value, CultureInfo.InvariantCulture),
                Quantity = quantity.TextContent.GetDigit(),
                Url = url
            };

            return auction;
        }

        private static IElement TryFetch(IDocument document, string selector, string exceptionMessage)
        {
            try
            {
                return document.QuerySelector(selector);
            }
            catch (Exception e)
            {
                throw new Exception(exceptionMessage);
            }
        }

Coś takiego ma sens?

0

Czy metodę GetDigit() powinienem użyć tak jak zrobiłem, czy przy: Quantity = quantity.GetDigit(),

Czy metodę decimal.Parse powinienem użyć tak jak zrobiłem, czy przy tak jak zrobiłem z GetDigit(), czyli na końcu metody.

Bez większego znaczenia. Bardziej zależy to od tego czy będziesz te dane(samo quantity i decimal) potrzebował do czegoś innego.

Jeśli nie będziesz, to osobiście zrobiłbym to we wczesnej fazie (np. pierwszym przypisaniu).

Jeśli aukcja nie ma np. ceny, to po dojechaniu na var price = document.QuerySelector.... wyrzuci wyjątek object reference not set to an instance of an object. Czy w takim razie za każdym razem powinienem zrobić coś takiego:

Dlaczego chcesz rzucać wyjątkiem? Czy aukcja niemająca ceny jest "naturalnym" zdarzeniem systemu, czy czymś czego nie chcesz dalej przekazywać/wyświetlać?

Jeśli wartość == null, możesz przypisać 0 (chyba, że tego nie chcesz).

Poza tym pierwszy sposób a drugi różni się, gdyż ten if sprawdzający może wyrzucić już jakiś exception i wtedy tego nie wyłapiesz.
Z drugiej strony robienie
catch (Exception)
throw new Exception... gubi Ci stacktrace'a i nie wiesz co tutaj się wysypało. Poczytaj o tym :)

Co do przycisków się nie wypowiem, bo nie znam się na desktopówce :(

0

W tym drugim kodzie mimo, że przypiszesz "quanity is missing" to potem wywalisz wyjątkiem w extension methodzie.
Łapiesz gdzieś te wyjątki później, czy chcesz żeby wywalał Ci się wątek apki/ cała apka? :)

@Edit
przy okazji, jak masz klasę
class Klasa
{
}
to dopisz tam zawsze słowo kluczowe "private"

0

Ja zwykle sprawdzam czy texboxy są wypełnione w oknach (np. Eventach buttonow) i jeśli nie są to jakis komunikat (MessageBox) i return bo to nie jest akcja na Exception. Metoda biznesowa to może sobie wywalać Exception jako zabezpieczenie ale nie powinno do tego dojść.
Ogolnie walidacja w UI nie powinna IMHO rzucać wyjatkow.

0
froziu napisał(a):

Poza tym pierwszy sposób a drugi różni się, gdyż ten if sprawdzający może wyrzucić już jakiś exception i wtedy tego nie wyłapiesz.

Ale znowu wydaje mi się, że ten kod nie wygląda elegancko:

string price;
try
{
	price = document.QuerySelector(ShopXPaths.Price).Attributes["data-price"].Value;
}
catch (Exception)
{
	throw new Exception("Price is missing");
}

Nagle z 1 linijki robi mi się 9. A przecież kilka fragmentów muszę zescrapować. Czy w takim razie powinno się to wyrzucić do osobnej metody jak zaproponowałem tutaj?

private static IElement TryFetch(IDocument document, string selector, string exceptionMessage)
{
    try
    {
        return document.QuerySelector(selector);
    }
    catch (Exception e)
    {
        throw new Exception(exceptionMessage);
    }
}
froziu napisał(a):

Łapiesz gdzieś te wyjątki później, czy chcesz żeby wywalał Ci się wątek apki/ cała apka? :)

No tak, widać przecież, że w evencie button click obsługują wyjątki

froziu napisał(a):

przy okazji, jak masz klasę
class Klasa
{
}
to dopisz tam zawsze słowo kluczowe "private"

A nie jest czasem tak, że jak nie określę modyfikatora dostępu do klasy, to jest ona prywatna?

jacek.placek napisał(a):

Ja zwykle sprawdzam czy texboxy są wypełnione w oknach (np. Eventach buttonow) i jeśli nie są to jakis komunikat (MessageBox) i return bo to nie jest akcja na Exception. Metoda biznesowa to może sobie wywalać Exception jako zabezpieczenie ale nie powinno do tego dojść.
Ogolnie walidacja w UI nie powinna IMHO rzucać wyjatkow.

Czyli miałoby to wyglądać tak? Po stronie formularza waliduję przez regexa, natomiast po stronie GetAuctionAsync rzucam wyjątkami, gdy coś nie gra? (tu dla przykładu użyłem kodu jakoby sprawdzałbym filmiki youtube).

namespace ShopSharp.Helpers
{
	static class Validation
	{
		public const string YoutubeUrlRegex = @"(?:youtube\.com\/(?:[^\/]+\/.+\/|(?:v|e(?:mbed)?)\/|.*[?&]v=)|youtu\.be\/)([^""&?\/ ]{11})";
		public const string YoutubeProfileUrlRegex = "...."
		
		public static bool IsValidLink(string url, string pattern)
		{
			if (string.IsNullOrEmpty(url))
				return false;
				
			string value = Regex.Match(s,
					pattern)
					.Groups[1].Value;

			return value != string.Empty	
		}
		
		public static bool IsYoutubeVideoLink(string url)
		{
			return IsValidLink(url, YoutubeUrlRegex);	
		}
		
		public static bool IsYoutubeProfileLink(string url)
		{
			return IsValidLink(url, YoutubeProfileUrlRegex);
		}
	}
}

private async void buttonScrape_ClickAsync(object sender, EventArgs e)
{
    string url = textBoxUrl.Text;
    if (!Validation.IsYoutubeVideoLink(url))
        MessageBox.Show("Enter valid youtube link");
		
    try
    {
        var video = await YoutubeFetcher.GetVideoInfo(url);
        FillTextBoxWithVideoInfo(video);
    }
    catch (Exception exception)
    {
        MessageBox.Show(exception.Message, "Wystąpił błąd!");
    }   
}
0

Tak. Tylko klamerki do ifa z validatorem i return po messagebox.show.

0
Brunatny Krawiec napisał(a):

Chciałbym napisać scrapera, który pobierałby mi dane z jakieś strony. Obrazłem za cel Allegro. Najpierw chcę go napisać tak, by nie korzystał z Api, tylko parsował HTML.

Tylko uważaj, żeby Cię nie zbanowali.

-ShopFetcher będzie odpowiadać za pobieranie danych i zwracanie aukcji. Nie będzie ona przechowywać żadnych informacji, więc jest oznaczona jako static.

Będzie ok dopóki nie zechcesz mieć w aplikacji kilku instancji tej klasy z różną konfiguracją.

-ShopXPaths odpowiada za wyrażenia xPath do poszczególnych fragmentów podstrony. Pomyślałem, że logicznie będzie wydzielić te informacje do innej klasy (i przestrzeni nazw)

To by miało sens, gdybyś przekazywał te wyrażenia w parametrze do ShopFetcher, tak aby ta klasa mogła wyciągać aukcje z różnych serwisów. W tym momencie ShopFetcher i tak jest na sztywno związany z tymi xpathami, więc równie dobrze mogłyby one być zahardcodowane w fetcherze. Zwłaszcza, że robisz tak z nazwą atrybutu data-price.

Jeśli chodzi o kod, to wygląda to tak:

Wywal z kodu oczywiste komentarze. Nie ma sensu pisanie komentarzy, które jedynie powtarzają to, co jest napisane w kodzie.

namespace ShopSharp.Extensions
{
    static class StringExtensions
    {
        public static int GetDigit(this string str)
        {
            var m = Regex.Match(str, @"\d+");
            if (m.Success)
            {
                var parsed = int.TryParse(m.Value, out var number);
                if (parsed)
                    return number;
            }
            throw new Exception();
        }
    }
}

Czemu metoda, która zwraca liczbę nazywa się "WeźCyfrę"?
Po co w ogóle ten Regex? Jeśli celem jest sparsowanie liczby i informacja o tym, czy się udało, to wystarczy TryParse. Nie trzeba do tego rzucać żadnych wyjątków.
I w ogóle nie rzucaj Exception bez żadnego komunikatu, są bezwartościowe.

Mam natomiast kilka pytań:

  1. Czy metodę GetDigit() powinienem użyć tak jak zrobiłem, czy przy: Quantity = quantity.GetDigit(),

Nie rozumiem pytania, więc napiszę tylko, że ja bym dążył do tego, aby użycie wyglądało tak:

            var title = document.QuerySelector(ShopXPaths.Title).GetCleanText();
            var price = document.QuerySelector(ShopXPaths.Price).GetIntFromAttribute("data-price");
            var quantity = document.QuerySelector(ShopXPaths.Quantity).GetDecimal();

Ad 3. A czemu nie po prostu tak: var price = document.QuerySelector(ShopXPaths.Price)?.Attributes["data-price"]?.Value;?

a może walidacja tu i tu?

Zdecydowanie tak.

0
somekind napisał(a):

Tylko uważaj, żeby Cię nie zbanowali.

Projekt jest robiony tylko w ramach nauki. Żądania do serwera są wysyłania przy refaktoryzacji, czyli raz na kilkanaście minut. Chyba mogę być spokojny.

somekind napisał(a):

Będzie ok dopóki nie zechcesz mieć w aplikacji kilku instancji tej klasy z różną konfiguracją.

A gdybym miał kilka instancji? Warto chyba wtedy użyć wzora projektowego dekorator, racja?

somekind napisał(a):

To by miało sens, gdybyś przekazywał te wyrażenia w parametrze do ShopFetcher, tak aby ta klasa mogła wyciągać aukcje z różnych serwisów. W tym momencie ShopFetcher i tak jest na sztywno związany z tymi xpathami, więc równie dobrze mogłyby one być zahardcodowane w fetcherze. Zwłaszcza, że robisz tak z nazwą atrybutu data-price.

Więc jak najlepiej przechowywać regexy w kodzie? Ja wywaliłem to do osobnej klasy, by mieć to w 1 miejscu, co jest wygodniejsze przy jakiś zmianach. Nie wiem, czy czasem nie gwałcę tu zasady YAGNI.

somekind napisał(a):

Po co w ogóle ten Regex? Jeśli celem jest sparsowanie liczby i informacja o tym, czy się udało, to wystarczy TryParse. Nie trzeba do tego rzucać żadnych wyjątków.

Chodzi o to, że struktura HTML tego elementu wygląda tak: <span class="no-wrap">z 97772 sztuk</span>. Ja chcę wydobyć tylko liczbę sztuk, a nie cały napis. Dlaczego nie powinienem rzucać tu wyjątku? Gdy z jakiegoś powodu element będzie wyglądał tak: <span class="no-wrap">z null sztuk</span>, to nie wydobędę informacji o ilości sztuk. Gdybym zwrócił 0, to skłamałbym, bo to nie jest prawidłowa liczba sztuk. Chyba wyjątek jest najbardziej adekwatny do tej sytuacji.

Mam natomiast kilka pytań:

  1. Czy metodę GetDigit() powinienem użyć tak jak zrobiłem, czy przy: Quantity = quantity.GetDigit(),

Nie rozumiem pytania, więc napiszę tylko, że ja bym dążył do tego, aby użycie wyglądało tak:

            var title = document.QuerySelector(ShopXPaths.Title).GetCleanText();
            var price = document.QuerySelector(ShopXPaths.Price).GetIntFromAttribute("data-price");
            var quantity = document.QuerySelector(ShopXPaths.Quantity).GetDecimal();

Chodzi o to, że nie wiem czy przy pobieraniu danych powinienem od razu dostawać konkretny typ, czy dopiero przy tworzeniu obiektu. Chyba najłatwiej to zrozumieć na przykładzie:

// PRZYKLAD 1
var price = document.QuerySelector(ShopXPaths.Price); // IElement, z ktorego pobieramy atrybut i rzutujemy go na decimal
var auction = new Auction()
{
    Price = decimal.Parse(price.Attributes["data-price"].Value, CultureInfo.InvariantCulture),
};

// PRZYKLAD 2
var price = document.QuerySelector(ShopXPaths.Price).Attributes["data-price"].Value; // string, ktory musimy zrzutowac na decimal
var auction = new Auction()
{
    Price = decimal.Parse(price, CultureInfo.InvariantCulture),
};

// PRZYKLAD 3
var price = document.QuerySelector(ShopXPaths.Price).Attributes["data-price"].Value.GetDecimal()); // wszystko zalatwione w 1 linii
var auction = new Auction()
{
    Price = price
};
somekind napisał(a):

Ad 3. A czemu nie po prostu tak: var price = document.QuerySelector(ShopXPaths.Price)?.Attributes["data-price"]?.Value;?

A w jakim przypadku stosuje się konstrukcję try catch, a w jakim to? Bo rozumiem, że jeśli chciałbym mieć sytuację, gdy nie uda się pozyskać informacji o cenie, to musiałbym wprowadzić coś takiego:

if (price == null)
throw new Exception("Price is missing");

I tak dla ilości sztuk, tytułu aukcji itd.

0
Brunatny Krawiec napisał(a):

Projekt jest robiony tylko w ramach nauki. Żądania do serwera są wysyłania przy refaktoryzacji, czyli raz na kilkanaście minut. Chyba mogę być spokojny.

Ja nie wiem, nie czytałem ich regulaminu. :)

A gdybym miał kilka instancji? Warto chyba wtedy użyć wzora projektowego dekorator, racja?

Nie, aby móc mieć kilka instancji wystarczy jedynie mieć klasę niestatyczną.

Więc jak najlepiej przechowywać regexy w kodzie? Ja wywaliłem to do osobnej klasy, by mieć to w 1 miejscu, co jest wygodniejsze przy jakiś zmianach. Nie wiem, czy czasem nie gwałcę tu zasady YAGNI.

Pytanie zasadnicze, czy planujesz obsługę też innych źródeł danych niż Allegro. Bo jeśli tak, to ShopFetcher powinien być jakoś konfigurowalny przez przekazanie obiektu dostarczającego informacje o sposobie parsowania różnych serwisów. A jeśli nie, to o ile można xpathy wydzielić do innej klasy, to po co w innym namespace, skoro to wszystko jest ze sobą ściśle powiązane?
No i ogólnie to xpathy wyciągnąłeś do innej klasy, a data-price pozostało zahardkodowane w ShopFetcher. Jak dla mnie to duża niekonsekwencja.

Chodzi o to, że struktura HTML tego elementu wygląda tak: <span class="no-wrap">z 97772 sztuk</span>. Ja chcę wydobyć tylko liczbę sztuk, a nie cały napis. Dlaczego nie powinienem rzucać tu wyjątku? Gdy z jakiegoś powodu element będzie wyglądał tak: <span class="no-wrap">z null sztuk</span>, to nie wydobędę informacji o ilości sztuk. Gdybym zwrócił 0, to skłamałbym, bo to nie jest prawidłowa liczba sztuk. Chyba wyjątek jest najbardziej adekwatny do tej sytuacji.

Tak, tylko z null sztuk da sobie radę samo int.TryParse i zwróci false, a z drugiej strony Twój kod zadziała bardzo błędnie dla z 97 772 sztuk.
I ja bym oddzielił wyciąganie cyfr z kodu HTML od ich parsowania. Obecnie ta metoda łamie SRP.

Chodzi o to, że nie wiem czy przy pobieraniu danych powinienem od razu dostawać konkretny typ, czy dopiero przy tworzeniu obiektu. Chyba najłatwiej to zrozumieć na przykładzie:

No jak dla mnie to w sumie najczytelniej byłoby tak:
var price = document.QuerySelector(ShopXPaths.Price).GetAttributeValue("data-price").AsDecimal();

Czytając to od razu widać wszystkie kroki postępowania: pobranie elementu strony, następnie jego watrybutu, a potem konwersję (nie rzutowanie!) na liczbę. Do tego metody GetAttributeVaue i AsDecimal mogą być używane w innych miejscach, do tego można w nich ukryć obsługę błędów, więc są uniwersalne i umieszczone w oddzielnej klasie w jakiejś oddzielnej bibliotece infrastrukturalnej.

Gorzej wygląda sytuacja z liczbą sztuk, która wymaga mądrzejszego parsowania. To nie jest coś uniwersalnego, więc umieszczanie takiej logiki w metodzie rozszerzającej tak podstawowy typ jak 'string' to spory błąd projektowy. Wprowadzasz w ten sposób czytelników/uzytkowników kodu w błąd.
To powinna być po prostu prywatna metoda w klasie ShopFetcher:
var quantity = TryExtractPrice(document.QuerySelector(ShopXPaths.Quantity));

A w jakim przypadku stosuje się konstrukcję try catch, a w jakim to?

Wyjątki rzucać należy w sytuacjach wyjątkowych. Czy brak ceny nim jest?
I nie chodziło mi o samo rzucanie wyjątków tylko o skrócenie zapisu przy użyciu ?..

Bo rozumiem, że jeśli chciałbym mieć sytuację, gdy nie uda się pozyskać informacji o cenie, to musiałbym wprowadzić coś takiego:

if (price == null)
throw new Exception("Price is missing");

I tak dla ilości sztuk, tytułu aukcji itd.

Musiałbyś?
Ja bym w ShopFetcher utworzył obiekt Auction, niech ma wartości null tam, gdzie nie udało się ich pobrać. A potem ten obiekt przekazał do walidatora (np. korzystającego z biblioteki FluentValidation), który sprawdzi czy obiekt jest poprawny. Zalety tego są takie, że:

  1. SRP: pobieranie danych powinno być w innej klasie niż walidacja
  2. Nie musisz rzucać żadnych wyjątków, wystarczy informacje z walidatora przekazać jakoś użytkownikowi. (Albo zalogować do pliku, albo cokolwiek innego.)
  3. Od razu będziesz miał informacje o wszystkich problemach związanych z daną aukcją, a nie musiał sprawdzać każdego pola po kolei jak w swoim kodzie.

1 użytkowników online, w tym zalogowanych: 0, gości: 1