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];


        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. Nov 1, 2017 at 17:32
  • 2
    Obligatory Coding Horror post: I Shall Call It... SomethingManager. Nov 1, 2017 at 17:33
  • needs more statics
    – Ewan
    Nov 1, 2017 at 17:52
  • @Ewan i dont see any reason for this class to have a non static member.
    – Joao Vitor
    Nov 1, 2017 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. Nov 1, 2017 at 20:51

1 Answer 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, 2017 at 0:43
  • what seems odd about it to you?
    – Ewan
    Nov 2, 2017 at 0:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.