patterncsharpMinor
Excel-to-JSON parser 2
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)
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 =>
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
This clearly shows that it won't fail whatsoever.
GetFiledValue & GetActionDatesArray etc
They should all be private members of the
SettingsController
CreateSettingsFile
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
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
and let it create a default model.
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.