0

So my manager class does 2 things, load items from file and give one item to a thread that asks for it, it also holds the List that will contain all the items.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.IO;

namespace Bot.Core.MainEntryPoint
{
    class ItemManager
    {
        private static List<string> ItemsList = new List<string>();
        private static int Pos = 0;
        private static object LockObject = new object(); // multiple threads would be accessing this at the same time

    public static void LoadFromFile()
    {
        ItemsList = File.ReadAllLines("PATH TO FILE").ToList();
    }

    public static string GetOneItem()
    {
        string result = "";

        if (List.Count > 0)
        {
            lock (LockObject)
            {
                if (Pos > ItemsList.Count - 1)
                {
                    Pos = 0;
                }

                result = ItemsList[Pos];

                Pos++;
            }
        }

        return result;
    }
}
}

Now in my opinion this violates the single responsibility principle and doesn't at the same time (seems to be relative to who uses it)

I tried having a ItemLoader class, a ItemGiver class, but then i can't figure out where i would be loading to or getting the items from, that "ItemsList" has to be somewhere.

  • I don't see a problem with it. Its one responsibility is to manage an ItemManager object. – Phil N DeBlanc Nov 1 '17 at 17:32
  • 2
    Obligatory Coding Horror post: I Shall Call It... SomethingManager. – Greg Burghardt Nov 1 '17 at 17:33
  • needs more statics – Ewan Nov 1 '17 at 17:52
  • @Ewan i dont see any reason for this class to have a non static member. – Joao Vitor Nov 1 '17 at 18:46
  • 1
    Unrelated to the question - are you aware that the .NET framework offers a number of thread-safe collections such as ConcurrentQueue and BlockingCollection? Most of the code in this class would be redundant with either of those. – Ben Cottrell Nov 1 '17 at 20:51
1

OK. So there are a number of (potential) problems with this class. But yes, I agree it shouldn't load itself from a file.

Although you have a simple case here, actually there is quite a lot to worry about. What format is the file, is it on the local disk, do I get it from an api, do I also have to know how to write the file etc etc.

Split the file loading and saving into a separate repository class.

public class LoopedList
{
    public LoopedList(IEnumerable items) {...}
    public Item GetNext() {...}
}

public class ItemListRepository_File : IItemRepo
{
    public LoopedList GetItemList(string id) {...}
}

The reason you are finding it hard to seperate the two bits of code is because you have everything static. This prevents you from decoupling.

Have a singleton LoopedList in your App.Main if you must. But keep everything else as instance variables

  • what if i have a "KeywordsManager", "ProxiesManager", "UrlsManager", executing your idea and sticking the singletons in the main() seems a bit odd.. – Joao Vitor Nov 2 '17 at 0:43
  • what seems odd about it to you? – Ewan Nov 2 '17 at 0:44

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.