ASP MVC Code Review anyone?

RealistSpy

Hi all,

I am learning ASP MVC 4/5 and I set my self a simple project.
Is it possible for someone to help review the code, what would be the general approach in a real life work place situation.

Dropbox

Phase 1: Data file input and results displayed in on a chart. [Current]
Phase 2: Same as phase 1 but data will show in a table with pagination.
Phase 3: Test Unit using NUnit or what would companies use?
Phase 4: I don't know yet

Aswerty

Spent a bit longer on this post than I meant to. Just as a bit of background; I've been working with MVC for about 5 years and mainly on MVC 3 and 4. I wouldn't be much help with MVC 5 since it takes a whole new approach to a lot of things.

First of all, it's a lot easier to just use github or bitbucket even for learning projects like this. Then other developers will be able to access it a lot more easily.

Also the dropbox project does not have a solution file (.sln). This file is an important part of every MVC application and should have been included. Secondly I had to sort out dependency issues since the references System.Net.Http and System.Web.Http were not found but this might have just been an issue with NuGet not installing the dependencies properly on my end as opposed to the build file not being correct.

The application structure itself is fine since you use the default MVC folders and namespaces. One thing I might mention is that since the application doesn't make use of a database the Model aspect of your application is overly trivial and is therefore not representative of most MVC applications used in the wild. Typically you'd be using ADO.NET or Entity Framework at the data access layer. And the data layer (Model) is typically a more complex/important/time-consuming aspect than the controllers and views.

The parking-system.js file is nice and modular. I'm assuming you have a reasonable amount of experience with JavaScript so there's not much to say there. One thing I'm not a fan of and consider a code smell is having inline JavaScript. So for example in Table.cshtml remove:

onclick="UploadFile(this, $('#parkingSystemUploadedFileInputButton'))" 

And in parking-system.js register the events when the page loads. For example:

$(document).ready(function() {
	RegisterEvents();
});

function RegisterEvents() {
	$('#parkingSystemUploadedFileInputButton').onclick = UploadFile;
};

So now we don't have to rely on inline code to trigger our client side events. This is particularly important since when the application complexity increases we don't have numerous cshtml files hiding away the entry points to our code.

I'd have a few quibbles with how you've approached the ParkingSystem class. Since there is only two methods I'll deal with each separately.

The first method...

public int InsertNewTimeString(string input)
{
    try
    {
	string[] splittedInput = input.Split(new[] {","}, StringSplitOptions.RemoveEmptyEntries);
	if(splittedInput.Length != 2) return -1;

	if (TransactionList == null) TransactionList = new List<ParkingSystemTimeStructure>();

	DateTime arrival;
	DateTime departure;

	if(!DateTime.TryParseExact(splittedInput[0].Trim(), "HH:mm", CultureInfo.InvariantCulture, DateTimeStyles.None, out arrival))return -1;
	if (!DateTime.TryParseExact(splittedInput[1].Trim(), "HH:mm", CultureInfo.InvariantCulture, DateTimeStyles.None, out departure)) return -1;
	TransactionList.Add(new ParkingSystemTimeStructure { Arrival = arrival, Departure = departure });
	return TransactionList.Count-1;
    }
    catch (Exception ex)
    {
	Debug.WriteLine(ex.StackTrace);
	return -1;
    }
}

In this method the approach taken to exception handling doesn't make sense. You've just wrapped the entire method with a try statement which isn't just not useful and bad practice, it's potentially harmful. Currently if there is a bug in this method (opposed to a user input issue) the application will respond that there is an issue with the file the user submitted. Exception handling should only be used for events that you know might happen and want to handle. Exceptions should not be used for flow control or handling bugs in your own code. Typically one uses exception handling when communication with an external component/system might fail such as a database or file system. So if there is a bug in your method let the exception bubble up to the MVC framework which will send a HTTP 500 Error to the client. Handling the logging of such an error should be done at the MVC framework (via HTTP Modules) level or IIS level. Even if this exception handling is just being done for debugging purposes it doesn't provide any value. Just let the application fail and allow the MVC framework do it's job. You should probably add a handler into the JavaScript Ajax query that handles a 500 error and informs the user that there is a server side issue and the problem is not on their side or due to their actions.

Also it might be an idea that instead of responding with a -1 when the user input isn't valid just create a method that performs validation separately and returns an error message if the data isn't valid. Then chuck that error message into the JsonResult and fire it back to the client. At the moment you're just highjacking the return value using a pretty naive approach.

Now onto the next method...

public List<ParkingSystemTimeFrameStructure> FindListTimeFrameWithMostCars()
{
    try
    {
	if (TransactionList == null || TransactionList.Count <= 0)
	{
	    TransactionList = new List<ParkingSystemTimeStructure>();
	    return null;
	}

	List<DateTime> listOfAllTimes = TransactionList.Select(x => x.Arrival)
						       .Concat(TransactionList.Select(x => x.Departure)).Distinct().ToList();
	listOfAllTimes.Sort();

	return (from start in listOfAllTimes
		let result = TransactionList.Count(r => r.Arrival <= start && r.Departure >= start)
		select new ParkingSystemTimeFrameStructure
		    {
			Count = result, TimeFrame = start
		    }).ToList();
    }
    catch (Exception ex)
    {
	Debug.WriteLine(ex.StackTrace);
	return null;
    }
}

This method is pretty messy from a readability point of view. It might be a good idea to separate the two LINQ queries into separate private methods within the class so it easier to understand each operation without having to delve into the logic. Also with regards to the LINQ queries - I'm not a fan of embedding one query inside another. Just declare it separately and then use the result in the following query. This should make it a lot more readable. Also jumping between the LINQ query syntax and method chaining makes the code more difficult to understand. It's easier to read if you just keep to one or the other.

In conclusion. The project is fine but some of the code isn't very well implemented from a readability point of view and is in need of a refactoring. The catch-all exception handling shows a lack of understanding as to how exceptions in code should be dealt with. There is very rarely a reason to catch every exception unless you are performing the very top level error handling and you just want to log before the application/process/thread crashes. Also the lack of a moderately complex Model means many of the difficulties one deals with when writing an MVC application aren't touched upon.

A very good start though, especially since most of my criticism is around more general C# application development. There is nothing wrong with how you approach this solution although not many applications don't use a database these days. You definitely understand the MVC pattern which can sometimes trip people up particularly if they come from WebForms. I hope you continue using MVC since it's a very nice platform and Microsoft seem to be developing it in the right direction. If you're just starting on it it's probably worth sticking with MVC 5 since that is the future of the platform and it deviates significantly from previous versions.

As to your phases. Phase 2 shouldn't be much work on top of whats already there. I'm a fan of https://www.nuget.org/packages/MvcPaging for paging in MVC 4 although it won't be supported for MVC 5. For Phase 3 NUnit is as popular as anything else, personally I use the Microsoft Unit Testing Framework but not sure how well that is supported on free to use Visual Studio plans. Though you're better off testing while developing (TDD and all that) rather than doing up unit tests after your code has been written. For Phase 4 I'd advise you to put in a database and interface with it using Entity Framework. You could even use MSSQL Compact or SQLite which are in memory databases if you don't want to go for something heavyweight like MSSQL or MySQL. And maybe try deploying it on Azure using a free dev/test plan and using the free database plan (or if using Complact/SQLite you just put the database in App_Data in your app).

And finally my rambling ends. Must get back to doing some work now...

RealistSpy

Thank you so so so so much.
I actually added the project to my GitHub account few days ago.
Again thank you so so so so so much.

I will edit the code based on your feedback.

I cannot edit the OP with the new link so here it is: Github
Any changes will be live on the github link.

Hopefully the MODs would be able to edit the OP for us.

dazberry

I was looking at this a while back but didn’t get to post a reply – so kudos to Aswerty for the in-depth reply.

Similar to Aswerty I don’t have much experience in MVC5, in my case mostly web api. As your controller at this point is really a web service as it’s returning json – I thought I'd play around with it a bit. I guess I could argue that that bit should be in web api, but prior to MVC6, MVC and Web API were separate libraries – and that would just add a layer of complexity to what you’re trying to do for very little benefit – so I think you’re right to have it the way it is.

Like Aswerty I found your ParkingSystem a little confusing – it has a number of responsibilities and I think it would be better if we tried to split it up a bit and reorganise things slightly.

So the first thing is that I changed your structures to classes and renamed them. While there could be arguments for and against, I think we all love POCOs now (plain old C# objects) so I did the following:

    public class ParkingTime
    {
        public DateTime Arrival { get; set; }
        public DateTime Departure { get; set; }
    }

    public class ParkingTimeFrameCount
    {
        public DateTime TimeFrame { get; set;}
        public int Count { get; set;}
    }

So the next thing is to move the InsertNewTimeString somewhere more appropriate. I guess we could put it in the ParkingTime class – it seems fair to consider the loading of the NewTimeString as being part of that responsibility. So we’d end up with something like this:

public class ParkingTime
    {
        public DateTime Arrival { get; set; }
        public DateTime Departure { get; set; }

        private DateTime ParseTimeSegment(string segment)
        {
            return DateTime.ParseExact(segment.Trim(), "HH:mm", CultureInfo.InvariantCulture, DateTimeStyles.None);
        }

        public void LoadFromCommaDelimitedString(string value)
        {
            string[] splittedInput = value.Split(new[] { "," }, StringSplitOptions.RemoveEmptyEntries);
            if (splittedInput.Length != 2)
            {
                throw new ArgumentException();
            }

            this.Arrival = ParseTimeSegment(splittedInput[0]);
            this.Departure = ParseTimeSegment(splittedInput[1]);
        }
    }

Now you could suggest to me that down the line you might need to support other formats to load data, so perhaps this won’t be too good in the future. Perhaps we could consider some sort of IoC (Inversion of Control) and inject some sort of parser into the object instance, something like:

public void Load(IParkingTimeParser parser, string[] value)
{
	parser.Load(value, out Arrival, out Departure);
}

Or use some sort of inheritance and polymorphic behaviour if you’re more that way inclined, but for the time being let’s keep it simple as is.

Ok, the next bit is the FindListTimeFrameWithMostCars() method, we need to do something with that too. Lets put it into it’s own class too.

public class ParkingFrameCountService
    {
        private IEnumerable<DateTime> GetDistinctParkingTimes(ICollection<ParkingTime> parkingTimes)
        {
            return parkingTimes.Select(x => x.Arrival)
                               .Concat(parkingTimes.Select(x => x.Departure))
                               .Distinct()
                               .OrderBy(x => x);
                               
        }

        public IEnumerable<ParkingTimeFrameCount> CalculateParkingFrameCounts(ICollection<ParkingTime> parkingTimes)
        {
            var distinctTimes = GetDistinctParkingTimes(parkingTimes);

            return distinctTimes.Select(x => new ParkingTimeFrameCount() 
            {
                TimeFrame = x,
                Count = parkingTimes.Count(y => y.Arrival <= x && y.Departure >= x),
            });

        }
    }

Couple of things to note. I guess first, we could have made the Linq into one single method – personally I prefer it as two as it’s easier to read – yvvm. Secondly, this class could be static as it contains no state, but again if we were using IOC it could be injected into the controller so we could mock/fake it in the future as part of our unit tests, but again to keep it simple lets just leave it as is.

So this again has one responsibility, take a list of parkingTimes and calculate the TimeFrame count thingy.

So that leaves us with the controller itself. Your uploadFile controller does a bit of the heavy lifting too by iterating through the fileLines and trying to insert each one. My personal preference is to keep the controllers as thin as possible – in work they stuff linq and a lot of crap into controllers and this annoys me no end – I much prefer a very thin controller calling some sort of service – where the responsibility of the controller is simply looking after perhaps automapping DTOs to models and vice versa, and handling specific HTTP return states.

So what I’m going to do here, is to pull the loop out, and again put it in a separate class. In this case I’ll make it static, again it has one responsibility

public static class ParkingTimesLoader
    {
        public static ICollection<ParkingTime> Load(string[] values)
        {
            var parkingTimes = new List<ParkingTime>();
            foreach (var value in values)
            {
                var parkingTime = new ParkingTime();
                parkingTime.LoadFromCommaDelimitedString(value);
                parkingTimes.Add(parkingTime);
            }
            return parkingTimes;
        }
    }

Ok, may not be the best name, and again we’re tied into the LoadFromCommaDelimitedString method, if we have to support different formats or whatever we’d need to rethink this – but again we can revisit that if we need to.

We don’t need the loop now, so our controller ends up like this…

        [HttpPost]
        public ActionResult UploadFile(string[] fileLines)
        {
            var parkingService = new ParkingFrameCountService();
            var result = parkingService.CalculateParkingFrameCounts(ParkingTimesLoader.Load(fileLines));
            return Json(new {status = "Success", message = "No new message", data = result});
        }

So this is our happy case, assuming it will always work. Ideally we’d return a HTTP status of 200 instead of the status = “Success” flag, but that’s for another day.

So we’ve no exception handling anywhere now, but if we throw some exception, we may not want a 500 Internal Server Error, when perhaps an Invalid request 400 would be more appropriate. The only exception was are purposely throwing is the ArgumentException, however the ParseExact can additionally throw ArgumentNull and FormatExceptions.

Unfortunately that leaves us in a scenario of having to do something like this which isn’t to good in this instance:

     try
            {
            }
            catch (ArgumentException ex)
            {

            }
            catch (ArgumentNullException ex)
            {

            }
            catch (FormatException ex)
            {
                
            } 

While we know that these errors could be returned from the ParseExact method, other things might return them too for different reasons – so ideally what I would do here is return some sort of custom exception, catching the ParseExact exception at the source and returning my custom exception.

catch (ParkingTimesLoadException ex)
{
}

But for the time being, lets just assume that we know what we’re doing, and it’s ok to catch these specifically – we could end up with something like this:

  [HttpPost]
        public ActionResult UploadFile(string[] fileLines)
        {
            try
            {
                var parkingService = new ParkingFrameCountService();
                var result = parkingService.CalculateParkingFrameCounts(ParkingTimesLoader.Load(fileLines));
                return Json(new {status = "Success", message = "No new message", data = result});
            }
            catch (Exception ex)
            {
                if ((ex is ArgumentException) || (ex is ArgumentNullException) || (ex is FormatException))
                    return Json(new
                    {
                        status = "Error",
                        message = "Invalid data file, please make sure you have valid 24hrs time. <br>" +
                        "<b> Example <br> 11:15, 14:20 <br> 11:15, 14:00",
                        data = ""
                    });
                throw;
            }
}

Basically we’ll handle those exceptions we expect, if it’s another one we'll let the server throw the 500.

Anyway, RealistSpy any questions, or expressions of disgust just fire them on!

D.

TLDR:
I’ve tried to move a little towards something more SOLID, just by trying to separate the responsibilities – S(ingle responsibility) without digging into the likes of D(ependency injection), as I think at this point that would confuse things.

RealistSpy

Thank you so so so much dazberry.
I will be make some changes to the code today based on all the feedbacks.
I want to learn web services as well so later on I might add it to the project.

The github link will have all the latest changes as I make them.

Thank you