0
\$\begingroup\$

I am using below code to write list item to a file. I want only 2 fields from the list. Is there any direct way without looping through all the items to do that? Here serversToshutDown is a SharePoint ListItemCollection.

using (System.IO.StreamWriter file = new System.IO.StreamWriter("F:\test\Test.csv"))
{
    foreach (ListItem item in serversToshutDown)
    {
        context.Load(item);
        context.ExecuteQuery();
        String server = item["Server"].ToString();
        String service = item["Service"].ToString();
        Console.WriteLine(server + "," + service);
        file.WriteLine(server + "," + service);
    }
}
\$\endgroup\$
2
\$\begingroup\$

First of all "F:\test\Test.csv" should be escaped or a verbatim string:

new StreamWriter("F:\\test\\Test.csv")

Or:

new StreamWriter(@"F:\test\Test.csv")

Also you should not hardcode paths in your code but I suppose this is only because of posting here (but you'd better show your actual code cleaned from sensitive information).

StreamWriter defaults to UTF-8 encoding. It may be what you need but I usually like to make it explicit to anyone reading my code:

new StreamWriter(path, false, Encoding.UTF8)

In my opinion main issue here is that this function is doing too many things: loading required data, writing to output and logging to console.

If output file size is not too big you may start with this (type names are fictional and some functions need a better name, I don't have enough context to pick a proper one).

Let's first define a class for your server data, it's also responsible for loading from context. If your scenario will become more complex you may want to introduce a separate Loader class and keep Server as a POCO object.

Note: I don't know about SharePoint ListItem object (see also BCdotWEB's answer), if you already have everything you need in ListItem then you may drop Server object.

sealed class Server
{
    public Server(Context context, ListItem item)
    {
        context.Load(server);
        context.ExecuteQuery();

        Name = Convert.ToString(item["Server"]);
        Service = Convert.ToString(item["Service"]);
    }

    public string Name
    {
        get;
        private set;
    }

    public string Service
    {
        get;
        private set;
    }

    public override string ToString()
    {
        return $"{Name}, {Service}";
    }
}

Now you can simplify your function moving different responsibilities to different functions. Your public function has to implement current behavior but must be easy to extend/change (for example if logging isn't for console, if output file is not specified by its path and so on):

public static void WriteServersToShutDown(string path, Context context,
    IEnumerable<ListItem> serversToShutdown)
{
    if (context == null)
        throw new ArgumentNullException("context");

    var content = ConvertToString(LoadData(context, serversToShutdown));

    Log(content);
    WriteFile(path, content);
}

To load data is the basic reusable block of your code (for example if you also need to perform some action, not just to dump a log):

private static IEnumerable<Server> LoadData(Context context,
    IEnumerable<ListItem> serversToShutdown)
{
    return serversToShutdown.Select(x => new Server(context, x));
}

You also need a function to convert IEnumerable<Server> list into a string with one line for each entry. This is the function you will edit if output format will be changed. In this example I rely on Server.ToString() implementation but it's often a bad idea, for this kind of tasks I usually override ToString() for debugging/logging purposes and I move formatting responsibility to a separate IFormatProvider object.

private string ConvertToString(IEnumerable<Server> servers)
{
    return String.Join(Environment.NewLine, servers);
}

Few helper functions:

private static void Log(string text)
{
    Console.WriteLine(text);
}

private static void WriteFile(string path, string content)
{
    File.WriteAllText(path, content, Encoding.UTF8);
}

Everything may be rewritten as:

var content = String.Join(Environment.NewLine,
    serversToShutdown.Select(x => new Server(context, x)));

Console.WriteLine(content);
File.WriteAllText(path, content, Encoding.UTF8);

Then why so many functions? Because each function performs exactly one task and if you need to change anything you will need to change only one function. This may not be obvious in this simple example but it's important for bigger programs. Also note that smaller functions may be reused and you keep all the code in sync:

foreach (var server in LoadData(context, serversToShutdown))
    ForceShutdown(server);

If you need to introduce a factory class (above mentioned loader) you need to change only LoadData() function. Do you need to change output file encoding or to add some error handling? Just go to WriteFile() and all callers will be updated. You change log to something else instead of console? Pick Log() function. Above everything else there is readability: to understand what WriteServersToShutDown() does you need to inspect just three lines of self-explicative code (after you renamed those functions with proper names from your domain).

\$\endgroup\$
1
\$\begingroup\$

I'm assuming that context is a ClientContext; please tell me that you properly dispose of it.


Why are you doing this:

foreach (ListItem item in serversToshutDown)
{
    context.Load(item);
    context.ExecuteQuery();

Aren't the properties you require already present when you retrieve serversToshutDown? I find these repeated calls to Load() and ExecuteQuery() really odd. I use a similar pattern in one of my applications, but only because I ran into an issue I couldn't solve otherwise.

\$\endgroup\$

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.