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

Excel-to-JSON parser 2

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

Problem

After first code review I have fixed the found issues. Could you please review my new refactored application?

Main class (entry point)

static class MainClass
{
    private static readonly ILog Log = LogManager.GetLogger(typeof(MainClass));

    static void Main(string[] args)
    {
        log4net.Config.XmlConfigurator.Configure();

        Log.Info("this app converts XLS data to export.json");

        var settings = SettingsController.LoadSettings(@"settings.json");
        var result = Converter.ConvertExcelToJson(settings);
        File.WriteAllText(@"export.json", result);

        Log.Info("converting finished.");
    }
}


Converter.cs

```
static class Converter
{
private static GoogleGeocoder GoogleGeocoder { get; set; }

private static readonly ILog Log = LogManager.GetLogger(typeof(Converter));

public static string ConvertExcelToJson(SettingsModel settings)
{
try
{
GoogleGeocoder = new GoogleGeocoder(); // default api key used

var nameField = GetSourceFieldNumber(settings, "Name");
var datesField = GetSourceFieldNumber(settings, "Dates");
var timesField = GetSourceFieldNumber(settings, "Times");
var zipField = GetSourceFieldNumber(settings, "PostalCode");
var cityField = GetSourceFieldNumber(settings, "City");
var streetField = GetSourceFieldNumber(settings, "Street");

var book = new LinqToExcel.ExcelQueryFactory(settings.FileName);

var worksheetName = book.GetWorksheetNames().FirstOrDefault();
if (worksheetName == null)
throw new Exception("Cannot find any worksheet");

Log.Info("start converting, please wait...");

ConsoleSpiner consoleSpiner = new ConsoleSpiner();

var test = book
.WorksheetRange(settings.CellToStart, settings.CellToEnd, worksheetName)
.AsEnumerable()
.Select(item =>

Solution

This doesn't look bad now but I've spotted a few things that I still would change.

Converter

.ConvertExcelToJson

This method is expected to return a string but it doesn't do it if an exception occurs. This is rather an unexpected behaviour. From a method like this I would expect it to throw an exception if something went wrong or have a different signature like this one:

public static bool TryConvertExcelToJson(SettingsModel settings, out string result)


This clearly shows that it won't fail whatsoever.

GetFiledValue & GetActionDatesArray etc

They should all be private members of the ExportsModel whose constructor should be passed the item.

SettingsController

CreateSettingsFile

Environment.Exit(0);


Putting this in that method is a very bad idea. I'd be really surprised if my application exited when I called this API.

If you still want to keep it there then consider naming the method CreateSettingsFileAndExit which would prevent such surprises.

ExtensionMethods

SetEndOfTheDay

This method does not set-end-for-the-day. It adds hours and minutes. What if the current time isn't exactly midnight? The end of they would be invalid. You should calculate the offsets or create an entirely new instance.

SettingsModel

Instead of having a constructor with a parameter I suggest keeping it clear and creating a new property

public static Default => new SettingsModel { ... }


and let it create a default model.

Code Snippets

public static bool TryConvertExcelToJson(SettingsModel settings, out string result)
Environment.Exit(0);
public static Default => new SettingsModel { ... }

Context

StackExchange Code Review Q#122479, answer score: 2

Revisions (0)

No revisions yet.