HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

SharePoint and JSON output project

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
sharepointjsonoutputprojectand

Problem

I'm working on a project that reads a series of SharePoint lists and outputs them into JSON file(s) which will later be uploaded via FTP to use on a website.

I'm aware that there's a lot I could be doing better, but I need some direction on what these areas are and what I can be doing to improve upon my development skills. Any feedback, good or bad, would be greatly appreciated.

Full project can be downloaded here

Program.cs

```
using System;
using System.Configuration;
using System.IO;
using System.Reflection;
using code.runner.intranet.Events;
using code.runner.intranet.JSON;
using NLog;

namespace code.runner
{
class Program
{
// create our NLOG references
private static readonly Logger Logger = LogManager.GetCurrentClassLogger();

// create our NLOG type
private enum LogType
{
Debug,
Info,
Error
};

static void Main(string[] args)
{
CLog(LogType.Info, "Code Runner started with version " + Assembly.GetAssembly(typeof(Program)).GetName().Version);

ProcessVolunteerOpportunities( ConfigurationManager.AppSettings["VCECurrentOpportunitiesOutputFile"],
ConfigurationManager.AppSettings["VCECurrentOpportunitiesSPSite"],
ConfigurationManager.AppSettings["VCECurrentOpportunitiesSPList"]);
}

private static void ProcessVolunteerOpportunities(string outputFile, string sharePointSite,
string sharepointList)
{
try
{
var r = new VCECurrentOpportunities();

// Attach VCE event handler
r.OnEventHandler += SharePointEventHandler;

try
{
// retrieve list and write to a new JSON file
using (var jsonFile = new StreamWriter(outputFile))
{
jso

Solution

public enum ExceptionLevel
{
    Debug,
    Info,
    Error
}


And

private enum LogType
{
    Debug,
    Info,
    Error
};


Both are enums, and both have the same values. This tells me you should probably merge these into one enum.

while (true)
{
    switch (type)
    {
        case LogType.Debug:
            Console.WriteLine("[{0}] {1}", DateTime.Now.ToShortTimeString(), message);
            Logger.Debug(String.Format("[{0}] {1}", DateTime.Now.ToShortTimeString(), message));
            break;


case LogType.Info:
            Console.WriteLine("[{0}] {1}", DateTime.Now.ToShortTimeString(), message);
            Logger.Info(String.Format("[{0}] {1}", DateTime.Now.ToShortTimeString(), message));
            break;

        case LogType.Error:
            Console.WriteLine("[{0}] {1}", DateTime.Now.ToShortTimeString(), message);
            Console.WriteLine("[{0}] Exception details: {1}", DateTime.Now.ToShortTimeString(), e);
            Logger.Error(String.Format("[{0}] {1}\n", DateTime.Now.ToShortTimeString(), message));
            Logger.Error(String.Format("[{0}] Exception details: {1}", DateTime.Now.ToShortTimeString(), e));
            break;

        default:
            type = LogType.Error;
            e = new Exception("Unknown logging type.");
            continue;
    }
    break;
}


If I read that correctly, it enters the loop, logs the error, and immediately exits the loop, unless the LogType is not entered there, upon which it sets the LogType to Error and loops again. I would probably do this with a guard clause rather than a loop which under all normal circumstances should never actually loop. Also, I expect R# would tell you the default case is never hit, and may be removed unless you have plans to add future values to LogType.

This is how I would do this:

var currentTime = DateTime.Now.ToShortTimeString();

if (type != LogType.Debug &&
    type != LogType.Info &&
    type != LogType.Error)
{
        type = LogType.Error;
        e = new Exception("Unknown logging type.");
}

switch (type)
{
    case LogType.Debug:
        Console.WriteLine("[{0}] {1}", currentTime, message);
        Logger.Debug(String.Format("[{0}] {1}", currentTime, message));
        break;

    case LogType.Info:
        Console.WriteLine("[{0}] {1}", currentTime, message);
        Logger.Info(String.Format("[{0}] {1}", currentTime, message));
        break;

    case LogType.Error:
        Console.WriteLine("[{0}] {1}", currentTime, message);
        Console.WriteLine("[{0}] Exception details: {1}", currentTime, e);
        Logger.Error(String.Format("[{0}] {1}\n", currentTime, message));
        Logger.Error(String.Format("[{0}] Exception details: {1}", currentTime, e));
        break;
}


Braces help prevent bugs. I would use braces here:

if (_onEvent == null) return;


Finally, since you are writing all the values into the log file, and are not removing any of them programmatically, I would probably not log to the screen. If you really want a notification, just set a Boolean value and write to the screen once after everything is done and logged.

Code Snippets

public enum ExceptionLevel
{
    Debug,
    Info,
    Error
}
private enum LogType
{
    Debug,
    Info,
    Error
};
while (true)
{
    switch (type)
    {
        case LogType.Debug:
            Console.WriteLine("[{0}] {1}", DateTime.Now.ToShortTimeString(), message);
            Logger.Debug(String.Format("[{0}] {1}", DateTime.Now.ToShortTimeString(), message));
            break;
case LogType.Info:
            Console.WriteLine("[{0}] {1}", DateTime.Now.ToShortTimeString(), message);
            Logger.Info(String.Format("[{0}] {1}", DateTime.Now.ToShortTimeString(), message));
            break;

        case LogType.Error:
            Console.WriteLine("[{0}] {1}", DateTime.Now.ToShortTimeString(), message);
            Console.WriteLine("[{0}] Exception details: {1}", DateTime.Now.ToShortTimeString(), e);
            Logger.Error(String.Format("[{0}] {1}\n", DateTime.Now.ToShortTimeString(), message));
            Logger.Error(String.Format("[{0}] Exception details: {1}", DateTime.Now.ToShortTimeString(), e));
            break;

        default:
            type = LogType.Error;
            e = new Exception("Unknown logging type.");
            continue;
    }
    break;
}
var currentTime = DateTime.Now.ToShortTimeString();

if (type != LogType.Debug &&
    type != LogType.Info &&
    type != LogType.Error)
{
        type = LogType.Error;
        e = new Exception("Unknown logging type.");
}

switch (type)
{
    case LogType.Debug:
        Console.WriteLine("[{0}] {1}", currentTime, message);
        Logger.Debug(String.Format("[{0}] {1}", currentTime, message));
        break;

    case LogType.Info:
        Console.WriteLine("[{0}] {1}", currentTime, message);
        Logger.Info(String.Format("[{0}] {1}", currentTime, message));
        break;

    case LogType.Error:
        Console.WriteLine("[{0}] {1}", currentTime, message);
        Console.WriteLine("[{0}] Exception details: {1}", currentTime, e);
        Logger.Error(String.Format("[{0}] {1}\n", currentTime, message));
        Logger.Error(String.Format("[{0}] Exception details: {1}", currentTime, e));
        break;
}

Context

StackExchange Code Review Q#71371, answer score: 3

Revisions (0)

No revisions yet.