Web API Interview Practical Test

A week ago, I applied for a 'Software Engineering Internship' at one of the company. Since I know C++, C#, Java well and have worked on couple of ASP.NET MVC projects with MS SQL server before, I knew I was well qualified for the job. Plus, I recently graduated from my Bachelor of Computer Science with High Distinction average so this opportunity was exactly what I was looking for.

I sent out my application and was shortlisted for a phone interview. I was really excited and couple of days after I got a phone call from HR and they told me that my skill sets were perfect and they want me to do a practical test before moving on to face to face interview. The lady told me that she was very confident that I would be able to smash the practical test and they would ask some database related questions on the test.

Before the practical test, I started studying SQL a lot, I did all the questions I could find on the internet and was fairly confident. On the day of the practical test, she sent me an email with PDF with the questions.

They told me I had 4 hours to complete those tasks. When I first looked at the questions, I was fairly surprised since I didn't expect the questions to be on this subject. I prepared for SQL. haha I had never programmed Web API before but I had done ASP.NET MVC web applications and knew that we can create API in the controller that can return JSON data which can be consumed by client using AJAX calls.

So, for the first task, I was thinking of using Entity Framework connecting to database file and mapping it to objects but in the second task they mentioned not to use Entity Framework so I had to use something else. At the end, I used XML format for data store because it was easy to hand craft an XML file for debugging purposes.

My Category class looks like this:

```
[Serializable]
public class Category
{
public string Name { get; set; }
public List Childs { get; set; }

public Category()
{
Childs = new List();
}

[Serializable]
public class Category
{
    public string Name { get; set; }
    public List Childs { get; set; }

    public Category()
    {
        Childs = new List();
    }

    public Category(string name)
    {
        Name = name;
        Childs = new List();
    }
}

Constructor chaining is your friend which helps you to reduce complexity and (usually) the amount of code.

[Serializable]
public class Category
{
    public string Name { get; set; }
    public List Childs { get; set; }

    public Category()
    {
        Childs = new List();
    }

    public Category(string name)
        : this()
    {
        Name = name;
    }
}

but moreover you should ask yourself, do I really want any outside caller to set the Childs property or would it be enough to access/change/delete the items of that List. If the latter is enough, make the setter private.

HomeController

//Creates root category from XML file
private Category getAllCategories()
{
    XmlSerializer xmlS = new XmlSerializer(typeof(Category));
    FileStream readFileStream = new FileStream(Server.MapPath("~/datamodel/model.xml"), FileMode.Open,  FileAccess.Read, FileShare.Read);
    Category category = (Category)xmlS.Deserialize(readFileStream);
    readFileStream.Close();
    return category;
}

-
The FileStream implements IDisposable hence it should be enclosed in a using block which takes care of disposing the FileStream object and therefor closing the underlying stream.

-
What happens if the file doesn't exist ? You should enclose it in a try..catch.

private void saveAllCategories(Category categories)
{
    XmlSerializer xmlS = new XmlSerializer(typeof(Category));
    TextWriter textWriter = new StreamWriter(Server.MapPath("~/datamodel/model.xml"));
    xmlS.Serialize(textWriter, categories);
    textWriter.Close();
}

The TextWriter implements the IDisposable interface as well.

//Navigates to particulate category
private Category navigateToCategory(IEnumerable path, Category root)
{
    Category destination = root;

    if (path.Count() == 1)  //It has got to be the root
    {
        return root;
    }
    else
    {
        for (int i = 1; i  c.Name == path.ElementAt(i));
            if (destination == null)    //Opps that is a problem, path doesn't exist
            {
                return null;
            }
        }
    }
    return destination;
}

The else is redundant, because it won't be reached if path.Count() == 1.

If you store the result of path.Count() into a variable it can be reused, right now you calculate this for each item in the IEnumerable. If that IEnumerable would be some type of ICollection that wouldn't do that much harm, just a cast and accessing the Count property, but for any other underlying type, it would need to iterate over the whole items and count them.

//Creates ouput that can be sent as Json. Categories name separated by ~
private string getDelimiterSeparatedCategories(Category category)
{
    string retVal = category.Name;
    foreach (var c in category.Childs)
    {
        retVal += "~" + c.Name;
    }
    return retVal;
}

Never ever use string concatenation within a loop, thats what a StringBuilder is for.

If you would override the ToString() method of the Category class like so

public override string ToString()
{
    return Name;
}

you could take advantage of the string.Join() method like so

private string getDelimiterSeparatedCategories(Category category)
{
    return category.Name + "~" + string.Join("~", category.Childs);
}

//API to delete category
[HttpPost]
public JsonResult DeleteCategory(IEnumerable path)
{
    if (path.Count() == 1)
    {
        return Json(new { Result = "ERROR" });
    }

    List pathParent = new List();

    for (int i = 0; i < path.Count() - 1; ++i)
    {
        pathParent.Add(path.ElementAt(i));
    }

    Category root = getAllCategories();
    Category parent = navigateToCategory(pathParent.AsEnumerable(), root);
    Category destination = navigateToCategory(path, root);

    if (destination == null)
    {
        return Json(new { Result = "ERROR" });
    }

    parent.Childs.Remove(destination);
    saveAllCategories(root);
    string retVal = getDelimiterSeparatedCategories(parent);
    return Json(new { Result = "OK", Data = retVal });
}

I don't get why you have this List pathParent in there. A much better way would be to use the Take() extension method. Another thing to mention is that you should retrieve the parent after you have checked the destination against null. If destination is null then there is no reason to retrieve the parent.

```
//API to delete category
[HttpPost]
public JsonResult DeleteCategory(IEnumerable path)
{
int pathCount = path.Count();

if (pathCount == 1)
{
return Json(new { Result = "ERROR" });
}

Category root = getAllCategories();

Catego

StackExchange Code Review Q#111075, answer score: 3