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