IEnumerable how to add item to list

Question

I have such interface as shown below. The problem is in AddLogger() method i cannot use just Loggers.Add(logger);

public interface INotificationFactory
{
    IEnumerable<ILogger> Loggers
    {
        get;
        set;
    }
    void AddLogger(ILogger logger);
    void DoLog(EMsgType msgType, string msg);
    IEnumerable<ILogger> GetLoggers();
}

Interface implementation:

public class NotificationFactory : INotificationFactory
{
    public IEnumerable<ILogger> Loggers  // read-write instance property
    { get; set; }

    public NotificationFactory()
    {
        Loggers = new List<ILogger>();
    }


    public void AddLogger(ILogger logger)
    {
        Loggers.Add(logger);
    }

    public void DoLog(EMsgType msgType, string msg)
    {
        foreach (var logger in Loggers)
        {
            logger.Write(msgType, msg);
        }
    }

    public IEnumerable<ILogger> GetLoggers()
    {
        return Loggers;
    }
}

hmm but then Loggers becomes List instead of IEnumerable? Is it corret approach? — dev, Dec 11 '18 at 10:27
@Rahul That would just add an item to a list which is to be garbage-collected immediately. — MakePeaceGreatAgain, Dec 11 '18 at 10:28

Dmitry Bychenko · Accepted Answer · 2018-12-11T10:56:49.183

0

I suggest redesigning:

Interface:

public interface INotificationFactory
{
    IEnumerable<ILogger> Loggers
    {
       get;
       set; // <- Remove it if it's possible
    }

    void AddLogger(ILogger logger);
    void DoLog(EMsgType msgType, string msg);

    // Remove it if it's possible: Loggers is enough
    IEnumerable<ILogger> GetLoggers();  
}

Implementation:

public class NotificationFactory : INotificationFactory
{
    // backing field: List<T> is by far more convenient than IEnumerable<T>
    private List<ILogger> m_Loggers = new List<ILogger>();  

    // Read-only: we don't want someone change the collection and removing items from it:
    public IReadOnlyList<ILogger> Loggers { 
        get {
            return m_Loggers;
        } 
    }

    IEnumerable<ILogger> INotificationFactory.Loggers {
        get {
            return m_Loggers;
        } 
        set {
            throw new NotSupportedException("Don't try to assign the collection"); 
        } 
    }

    // The only way to add an item - logger - is this direct call  
    public void AddLogger(ILogger logger)
    {
        if (null == logger)
            throw new ArgumentNullException(nameof(logger));

        // Just adding a logger - nothing to write home about
        m_Loggers.Add(logger);
    }

    public void DoLog(EMsgType msgType, string msg)
    {
        foreach (var logger in m_Loggers)
        {
            logger.Write(msgType, msg);
        }
    }

    //TODO: Do you want it? Loggers property seems to be enough
    IEnumerable<ILogger> INotificationFactory.GetLoggers()
    {
        return m_Loggers;
    }
}

edited Dec 11 '18 at 10:56

answered Dec 11 '18 at 10:31

Dmitry Bychenko

180,369
20
160
215

My understanding from different articles is people almost always used to use IEnumerable<> for lists perhaps because it can be then converted to different collection types (like List/Array etc) and therefore i used IEnumerable instead of List directly as return type. Can you explain why then most developers used IEnumerable<> in their envinorment instead of List<>? – dev Dec 11 '18 at 10:34
Should probably change - `return m_Loggers;` to `return m_Loggers.AsReadOnly();` – Rand Random Dec 11 '18 at 10:36
@dev: yes an *input argument* of a method (constructor) should be `IEnumerable` in order the method can work with different collections. **Private** *backing field* is a different matter: it's `private` i.e. `NotificationFactory`'s *private business* only. It won't ever be exposed as it is. – Dmitry Bychenko Dec 11 '18 at 10:36
@Rand Random: my implementation `IReadOnlyList` prevents occasional errors / typos like `MyFactory.Loggers.RemoveAt(123);` only. Your recommendation - `AsReadOnly` is safier but wants more time and memory (the collection will be copied) – Dmitry Bychenko Dec 11 '18 at 10:40
It would prevent this `((List)MyFactory.Loggers).RemoveAt(123)` - but yeah, it costs time/memory - OP has to decide if he wants it 100% safe or 99% safe :) - I tend to make a second private field with the readonly stored and update it on Add/Remove operations and return the readonly field – Rand Random Dec 11 '18 at 10:42
@DmitryBychenko I see but i also need to change IEnumerable to List inside my interface after your change, however also most of developers used to use IEnumerable in interfaces rather than List - can you explain that? – dev Dec 11 '18 at 10:46
@dev: since we have a *better* offer `IReadOnlyList Loggers` but *have to* implement `INotificationFactory` interface (`IEnumerable Loggers`), let's do it as *explicit interface implementation* - use `IEnumerable Loggers` if and only if we call an interface, not class. – Dmitry Bychenko Dec 11 '18 at 11:00
@DmitryBychenko to be honest i do not get it. Why then developers used to use IEnumerable over interface instead of IReadOnlyList? P.S I will be calling as INotificationFactory from outside instead of calling directly a class NotificationFactory lao because it is better to use as interface and also good for testing purposes. – dev Dec 11 '18 at 11:08
@dev: *interface* can be implemented in *different ways*. Imagine that I load `ILoggers` from a *database* (and thus I don't want to provide a list - all the loggers downloaded - fetching from RDMS costs a lot of time). Interface is the *most general* contract (`IEnumerable` - give me at least a possibility to enumerate loggers...). An *implementation*, however, can provide a *better service*, as we do - we can return a list (and thus provide an indexer `Loggers[123]`, `Count` etc.) – Dmitry Bychenko Dec 11 '18 at 11:12
@dev: Since interface is *the most general* contract, I suggest have it as `public interface INotificationFactory {IEnumerable Loggers {get;} void AddLogger(ILogger logger); void DoLog(EMsgType msgType, string msg); }`. No `IReadOnlyList` in the interface; however, an implementation can offer `public IReadOnlyList Loggers` (extended service) – Dmitry Bychenko Dec 11 '18 at 11:43
@dev: Because interface *makes* me to implement property as `IEnumerable Loggers`; I have to abide by the rules but I can conceal this property. – Dmitry Bychenko Dec 11 '18 at 11:45
@DmitryBychenko If we got one additional public IReadOnlyList Loggers shouldn't it be also added inside interface? – dev Dec 11 '18 at 11:47
@dev: no, since we added it into interface's implementation. Other implementation can well not provide `IReadOnlyList Loggers` property – Dmitry Bychenko Dec 11 '18 at 11:52

IEnumerable how to add item to list

1 Answers1