Skip to main content
deleted 4 characters in body
Source Link
BCdotWEB
  • 11.3k
  • 2
  • 27
  • 45
  • The FileStream implements IDisposable hence it should be enclosed in a using block which takes care of disposing the FileStream object and therfortherefor closing the underlayingunderlying stream.

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

The TextWriter implements the IDisposable interface tooas well.

  • I don't like the camleCasecamelCase casing for naming private methods. Why would you want to distinguish between private and public by the casing style  ? If you later decide to change a private method to public you will likely forget to rename the method.

  • The comments above the methods like //Navigates to particulate category doesn'tdon't add any value. If you would have proper xml documentation this would alosalso show up in intellisense. That being said, for public methods which are part of an API proper xml documentation is a have tomust-have.

  • you are using braces {} although they might be optional which is good.

  • you should consider to change some methodnames likemethod names; e.g

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

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

The TextWriter implements the IDisposable interface too.

  • I don't like the camleCase casing for naming private methods. Why would you want to distinguish between private and public by the casing style  ? If you later decide to change a private method to public you will likely forget to rename the method.

  • The comments above the methods like //Navigates to particulate category doesn't add any value. If you would have proper xml documentation this would alos show up in intellisense. That being said, for public methods which are part of an API proper xml documentation is a have to.

  • you are using braces {} although they might be optional which is good.

  • you should consider to change some methodnames like e.g

  • 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.

The TextWriter implements the IDisposable interface as well.

  • I don't like the camelCase casing for naming private methods. Why would you want to distinguish between private and public by the casing style? If you later decide to change a private method to public you will likely forget to rename the method.

  • The comments above the methods like //Navigates to particulate category don't add any value. If you would have proper xml documentation this would also show up in intellisense. That being said, for public methods which are part of an API proper xml documentation is a must-have.

  • you are using braces {} although they might be optional which is good.

  • you should consider to change some method names; e.g

Source Link
Heslacher
  • 50.4k
  • 5
  • 81
  • 175

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

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

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

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<Category> Childs { get; set; }

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

    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<T>. 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 therfor closing the underlaying 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 too.


//Navigates to particulate category
private Category navigateToCategory(IEnumerable<string> 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 < path.Count(); ++i)
        {
            destination = destination.Childs.FirstOrDefault(c => 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<T>. If that IEnumerable<T> would be some type of ICollection<T> 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<string> path)
{
    if (path.Count() == 1)
    {
        return Json(new { Result = "ERROR" });
    }

    List<string> pathParent = new List<string>();

    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<string> 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<string> path)
{
    int pathCount = path.Count();

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

    Category root = getAllCategories();

    Category destination = navigateToCategory(path, root);

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

    Category parent = navigateToCategory(path.Take(pathCount - 1), root);
    
    parent.Childs.Remove(destination);

    saveAllCategories(root);
    string retVal = getDelimiterSeparatedCategories(parent);

    return Json(new { Result = "OK", Data = retVal });
}

As a side note, because List<T> implements IEnumerable<T> there is no need to call AsEnumerable() on such a List<T> if you need an IEnumerable<T>.


Style

  • I don't like the camleCase casing for naming private methods. Why would you want to distinguish between private and public by the casing style ? If you later decide to change a private method to public you will likely forget to rename the method.

  • The comments above the methods like //Navigates to particulate category doesn't add any value. If you would have proper xml documentation this would alos show up in intellisense. That being said, for public methods which are part of an API proper xml documentation is a have to.

  • you are using braces {} although they might be optional which is good.

  • you should consider to change some methodnames like e.g

Category root = getAllCategories();  

is just misleading. Maybe GetRootCategory would be a better name.