C# Profile Scraper

Question

I'm a 16-year-old so I never studied programming, although I do try to adopt the best practices I can when given the chance.

I would like to clean up the code posted below by making it comply with SOLID principle, and any other general improvements that are offered.

The class in question is the core entry point of my whole application. Each instance of this application acts as a "worker" for a scraper network.

public class ScraperWorker
{
    private readonly ScraperQueue _scraperQueue;
    private readonly Dictionary<string, ISocialEventHandler> _eventHandlers;
    private readonly ScraperWorkerDao _scraperWorkerDao;
    private readonly ScraperQueueDao _scraperQueueDao;
    private readonly Client _bugSnagClient;
    private readonly ILogger _logger;

    private bool _isProcessing;

    public ScraperWorker(
        ScraperQueue scraperQueue, 
        Dictionary<string, ISocialEventHandler> eventHandlers, 
        ScraperWorkerDao scraperWorkerDao, 
        ScraperQueueDao scraperQueueDao,
        Client bugSnagClient,
        ILogger logger)
    {
        _scraperQueue = scraperQueue;
        _eventHandlers = eventHandlers;
        _scraperWorkerDao = scraperWorkerDao;
        _scraperQueueDao = scraperQueueDao;
        _bugSnagClient = bugSnagClient;
        _logger = logger;
        
        Process();
    }

    public void Start()
    {
        _isProcessing = true;
    }

    public void Stop()
    {
        _isProcessing = false;
    }

    private void Process()
    {
        var ticksSinceStatusUpdate = 0;
        
        new Thread(() =>
        {
            while (true)
            {
                ticksSinceStatusUpdate++;

                if (ticksSinceStatusUpdate >= 10)
                {
                    RenewWorkerStatus(); // Checks if we have paused the worker via an external service.
                    ticksSinceStatusUpdate = 0;
                }

                if (!_isProcessing)
                {
                    _logger.Warning($"The worker is currently paused, sleeping for 30 seconds.");

                    Thread.Sleep(TimeSpan.FromSeconds(30));
                    continue;
                }
                
                if (!_scraperQueue.TryGetItem(out var item))
                {
                    _logger.Warning($"The queue is currently empty, sleeping for 30 seconds.");

                    Thread.Sleep(TimeSpan.FromSeconds(30));
                    continue;
                }

                _scraperWorkerDao.UpdateWorkerLastSeen(StaticState.WorkerId);

                var eventHandler = ResolveEventHandlerFromItem(item.Item);
                
                eventHandler.SetCurrentItem(item.Item);

                if (eventHandler.IsLoginNeeded())
                {
                    eventHandler.Login();
                }

                var result = ProcessQueueItem(item, eventHandler);

                if (result.IsSuccess)
                {
                    _logger.Success($"Finished processing {item.Item}");
                }
                else
                {
                    _logger.Trace($"Finished processing {item.Item}");
                }

                Console.WriteLine();

                _scraperQueueDao.MarkItemAsComplete(item.Id);
                _scraperQueueDao.StoreItemResultInDatabase(result);
            }
        }).Start();
    }

    private void RenewWorkerStatus()
    {
        _isProcessing = _scraperWorkerDao.IsWorkerRunning(StaticState.WorkerId);
    }

    private ScraperQueueItemResult ProcessQueueItem(ScraperQueueItem item, ISocialEventHandler eventHandler)
    {
        _logger.Trace($"Processing {item.Item}");
        
        try
        {
            eventHandler.NavigateToProfile();

            if (eventHandler.IsProfileVisitsThrottled())
            {
                eventHandler.SwitchAccount(true);
                return ProcessQueueItem(item, eventHandler);
            }

            if (!eventHandler.TryWaitForProfileToLoad())
            {
                return new ScraperQueueItemResult(
                    item.Id, "Method 'TryWaitForProfileToLoad' returned false.", eventHandler.GetPageSource(), false
                );
            }

            var profile = eventHandler.CreateProfile();

            if (!profile.ShouldScrape(out var validationResult))
            {
                return new ScraperQueueItemResult(
                    item.Id, validationResult.ToString(), eventHandler.GetPageSource(), false
                );
            }

            var connectionsToStore = new List<ProfileConnection>();

            if (profile.ShouldCollectConnections())
            {
                profile.Connections = eventHandler.GetConnections();

                connectionsToStore = eventHandler.GetFilteredConnections(profile.Connections);

                if (connectionsToStore.Any())
                {
                    _logger.Trace(
                        $"Collected {profile.Connections.Count} / {profile.FollowerCount}, storing {connectionsToStore.Count} of them in the database.");
                }
            }

            if (profile.ShouldSave(out validationResult))
            {
                if (!profile.IsPrivate)
                {
                    profile.Posts = eventHandler.GetPosts(profile.Username);
                }

                profile.Save();

                if (connectionsToStore.Any())
                {
                    _scraperQueue.AddItems(eventHandler.ConvertConnectionsToQueueItems(connectionsToStore), profile.Id);
                }

                return new ScraperQueueItemResult(
                    item.Id, "success", "", true
                );
            }

            if (connectionsToStore.Any())
            {
                _scraperQueue.AddItems(eventHandler.ConvertConnectionsToQueueItems(connectionsToStore), profile.Id);
            }

            return new ScraperQueueItemResult(
                item.Id, validationResult.ToString(), eventHandler.GetPageSource(), false
            );
        }
        catch (Exception e)
        {
            _bugSnagClient.Notify(e, report =>
            {
                report.Event.Metadata.Add("queue_item", item.Item);
                report.Event.Metadata.Add("current_url", eventHandler.GetUrl());
                report.Event.Metadata.Add("page_source", eventHandler.GetPageSource());
                report.Event.Metadata.Add("created_at", DateTime.Now.ToString("yyyy-MM-dd HH:mm:ss"));
            });
            
            // TODO: Try and requeue the item?

            _logger.Error(e.Message);
            
            return new ScraperQueueItemResult(
                item.Id, e.Message, eventHandler.GetPageSource(), false
            );
        }
    }

    private ISocialEventHandler ResolveEventHandlerFromItem(string item)
    {
        var host = new Uri(item).Host.Replace("www.", "");

        if (_eventHandlers.ContainsKey(host))
        {
            return _eventHandlers[host];
        }

        throw new Exception($"Failed to resolve event handler for host '{host}'");
    }
}

ISocialEventHandler (each site inherits this and has its own implementation on how a site should be scraped)

public interface ISocialEventHandler
{
    string GetLoginPageUrl();
    string GetLogoutPageUrl();
    bool IsLoginNeeded();
    void Login();
    void Logout();
    ISocialProfile CreateProfile();
    List<ProfileConnection> GetConnections();
    void NavigateToProfile();
    bool TryWaitForProfileToLoad();
    string GetUrl();
    string GetUsername();
    string GetName();
    string GetProfilePicture();
    bool GetIsPrivate();
    int GetFollowerCount();
    int GetFollowingCount();
    List<ProfilePost> GetPosts(string owner);
    List<ProfilePostMedia> GetPostMedia(string owner);
    void SetCurrentItem(string currentItem);
    List<ProfileConnection> GetFilteredConnections(List<ProfileConnection> connections);
    string GetPageSource();
    List<ScraperQueueItem> ConvertConnectionsToQueueItems(List<ProfileConnection> connections);
    bool IsProfileVisitsThrottled();
    void SwitchAccount(bool markCurrentAsThrottled);
}

ScraperQueueItem (just an entity)

public long Id { get; }
public string Item { get; }
public int TypeId { get; } // determines provider
public bool IsPrivate { get; }
public bool Confirmed { get; }

public ScraperQueueItem(long id, string item, int typeId, bool isPrivate, bool confirmed)
{
    Id = id;
    Item = item;
    TypeId = typeId;
    IsPrivate = isPrivate;
    Confirmed = confirmed;
}

ScraperQueueItemResult (used to map from and to the database, another entity)

public long ItemId { get; set; }
public string Content { get; set; }
public string PageSource { get; set; }
public bool IsSuccess { get; set; }

public ScraperQueueItemResult(long itemId, string content, string pageSource, bool isSuccess)
{
    ItemId = itemId;
    Content = content;
    PageSource = pageSource.Truncate(1000);
    IsSuccess = isSuccess;
}

ProfileConnection (used to map, another entity)

public class ProfileConnection
{
    public string Item { get; set; }
    public bool IsPrivate { get; set; }
}

ProfilePost (another DB mapper to and from, entity):

public class ProfilePost
{
    public List<ProfilePostMedia> Media { get; }

    public ProfilePost(List<ProfilePostMedia> media)
    {
        Media = media;
    }
}

ProfileMostMedia (another DB mapper to and from, entity):

public class ProfilePostMedia
{
    public string CdnUrl { get; set; }
    public string MetaData { get; set; }

    public ProfilePostMedia(string cdnUrl, string metaData)
    {
        CdnUrl = cdnUrl;
        MetaData = metaData;
    }
}

Answer 1

Thank you for updating your question!

In your question you mention the SOLID principle, it isn't just one principle, SOLID is 5 object orieneted design principles. SOLID is a mnemonic acronym for five design principles intended to make software designs more understandable, flexible and maintainable. This will help you design your objects and classes better.

1. The Single Responsibility Principle - A class should only have a single responsibility, that is, only changes to one part of the software's specification should be able to affect the specification of the class.
2. The Open–closed Principle - states software entities (classes, modules, functions, etc.) should be open for extension, but closed for modification.
3. The Liskov Substitution Principle - Objects in a program should be replaceable with instances of their subtypes without altering the correctness of that program.
4. The Interface segregation principle - states that no client should be forced to depend on methods it does not use.
5. The Dependency Inversion Principle - is a specific form of decoupling software modules. When following this principle, the conventional dependency relationships established from high-level, policy-setting modules to low-level, dependency modules are reversed, thus rendering high-level modules independent of the low-level module implementation details.

I'm going to focus on the Single Responsibility Principle (SRP) here because that seems to be the major issue. The single responsibility principle doesn't apply just to classes, it applies to functions, methods and interfaces as well. There are other similar concepts or principles such as the Keep It Simple (KISS - the military has a 4th word in here) principle.

If at some point in the future you study computer science at a university you may also learn Top Down Design as well. The concept of top down design is that one keeps breaking the problem into smaller and smaller pieces until each piece is a very simple solution. This is a major source of methods and functions, and it can be applied to objects and classes as well. Some of your classes such as ProfilePost, ProfilePostMedia, ProfileConnection and ScraperQueueItemResult already show some application of the SRP, however, ScraperWorker and the interface ISocialEventHandler look like major collections of many separate objects and actions and the names seem to imply the same thing.

Software engineers also use a visual modeling language called UML (Unified Modeling Language) to diagram objects, object interactions, system interactions and timing sequences. This helps them decide when an object/class is getting too complicated.

Simplify the Function ProcessQueueItem

I believe your original question was asking how to simplify the following function:

    private ScraperQueueItemResult ProcessQueueItem(ScraperQueueItem item, ISocialEventHandler eventHandler)
    {
        _logger.Trace($"Processing {item.Item}");

        try
        {
            eventHandler.NavigateToProfile();

            if (eventHandler.IsProfileVisitsThrottled())
            {
                eventHandler.SwitchAccount(true);
                return ProcessQueueItem(item, eventHandler);
            }

            if (!eventHandler.TryWaitForProfileToLoad())
            {
                return new ScraperQueueItemResult(
                    item.Id, "Method 'TryWaitForProfileToLoad' returned false.", eventHandler.GetPageSource(), false
                );
            }

            var profile = eventHandler.CreateProfile();

            if (!profile.ShouldScrape(out var validationResult))
            {
                return new ScraperQueueItemResult(
                    item.Id, validationResult.ToString(), eventHandler.GetPageSource(), false
                );
            }

            var connectionsToStore = new List<ProfileConnection>();

            if (profile.ShouldCollectConnections())
            {
                profile.Connections = eventHandler.GetConnections();

                connectionsToStore = eventHandler.GetFilteredConnections(profile.Connections);

                if (connectionsToStore.Any())
                {
                    _logger.Trace(
                        $"Collected {profile.Connections.Count} / {profile.FollowerCount}, storing {connectionsToStore.Count} of them in the database.");
                }
            }

            if (profile.ShouldSave(out validationResult))
            {
                if (!profile.IsPrivate)
                {
                    profile.Posts = eventHandler.GetPosts(profile.Username);
                }

                profile.Save();

                if (connectionsToStore.Any())
                {
                    _scraperQueue.AddItems(eventHandler.ConvertConnectionsToQueueItems(connectionsToStore), profile.Id);
                }

                return new ScraperQueueItemResult(item.Id, "success", "", true);
            }

            if (connectionsToStore.Any())
            {
                _scraperQueue.AddItems(eventHandler.ConvertConnectionsToQueueItems(connectionsToStore), profile.Id);
            }

            return new ScraperQueueItemResult(
                item.Id, validationResult.ToString(), eventHandler.GetPageSource(), false
            );
        }
        catch (Exception e)
        {
            _bugSnagClient.Notify(e, report =>
            {
                report.Event.Metadata.Add("queue_item", item.Item);
                report.Event.Metadata.Add("current_url", eventHandler.GetUrl());
                report.Event.Metadata.Add("page_source", eventHandler.GetPageSource());
                report.Event.Metadata.Add("created_at", DateTime.Now.ToString("yyyy-MM-dd HH:mm:ss"));
            });

            // TODO: Try and requeue the item?

            _logger.Error(e.Message);

            return new ScraperQueueItemResult(
                item.Id, e.Message, eventHandler.GetPageSource(), false
            );
        }
    }

Having try{}/catch{} blocks inherently makes a function larger and more complex and it always a good idea to move parts of that function into private sub-functions, however, one of the first things I notice is repetition of the following code:

            if (connectionsToStore.Any())
            {
                _scraperQueue.AddItems(eventHandler.ConvertConnectionsToQueueItems(connectionsToStore), profile.Id);
            }

Perhaps that code can be moved up in the function so that it is only used once.

The second thing that stands out is the code in the catch{} block:

            _bugSnagClient.Notify(e, report =>
            {
                report.Event.Metadata.Add("queue_item", item.Item);
                report.Event.Metadata.Add("current_url", eventHandler.GetUrl());
                report.Event.Metadata.Add("page_source", eventHandler.GetPageSource());
                report.Event.Metadata.Add("created_at", DateTime.Now.ToString("yyyy-MM-dd HH:mm:ss"));
            });

I would move that code into a small private function that accepts item and eventHandler as arguments and performs that code in the function, perhaps the function can be called bugReport().

This code can be combined into a function, if that function returns a NULL value then continue processing, if the function returns a value then return that value to the calling function:

            if (eventHandler.IsProfileVisitsThrottled())
            {
                eventHandler.SwitchAccount(true);
                return ProcessQueueItem(item, eventHandler);
            }

            if (!eventHandler.TryWaitForProfileToLoad())
            {
                return new ScraperQueueItemResult(
                    item.Id, "Method 'TryWaitForProfileToLoad' returned false.", eventHandler.GetPageSource(), false
                );
            }

This code should be in a function that returns the list:

            var connectionsToStore = new List<ProfileConnection>();

            if (profile.ShouldCollectConnections())
            {
                profile.Connections = eventHandler.GetConnections();

                connectionsToStore = eventHandler.GetFilteredConnections(profile.Connections);

                if (connectionsToStore.Any())
                {
                    _logger.Trace(
                        $"Collected {profile.Connections.Count} / {profile.FollowerCount}, storing {connectionsToStore.Count} of them in the database.");
                }
            }

Perhaps that is where you can call the repeated code mentioned above.