对我的代码评审总结

在领导的极力推动下，最近部门开始大力进行代码评审。

有幸成为部门代码评审委员会一员，负责评审了许多同事的代码。在评审他人代码的同时，也对我自己是一个极大的进步。

现在coding的感觉，已经和半年前有了极大的差别。

然后评审委员会选出我的一小段代码作为“模范代码”让公司的首席科学家“老天”评审，通过其细致的评审，甚至让我对这段我曾经认为还不错的代码生出了愧疚之情，看来代码真的没有最好，只有更好。

最近对我的代码评审，总结为以下几点建议：

1。抛出异常需果断，绝不能对调用者手软

2。线程锁的关系需更仔细的设计

3。返回值不给不必要的bool

4。应该更加多应用语言特性，比如c#的using

5。异常处理机制需更仔细的设计

6。类成员实例尽量不要暴露，如需要，给出拷贝

7。注释给的不够精炼

附一段我的代码，C#写的（一个树状任务系统的Manager部分）

using System; using System.Collections.Generic; using System.Linq; using System.Text; using NetVideo.TaskTrace.Model; using NetVideo.TaskTrace.DAL; using NetVideo.TaskTrace.Common.DBHelper; namespace NetVideo.TaskTrace.BLL { /// <summary> /// 任务树管理器 /// </summary> public class TreeManager { #region singleton static private TreeManager _instance = null; static private Object _mutex = new Object(); static public TreeManager getInstance() { if( _instance == null) { lock(_mutex) { if(_instance == null ) _instance = new TreeManager(); } } return _instance; } private TreeManager() { specialViewManager = new SpecialViewManager(); if (!specialViewManager.Load()) throw new Exception("特殊查看权限管理器初始化失败!"); virtualRoot = new TaskTreeNode(0, "虚拟根节点", 0, 0, true); } #endregion #region 属性 private TaskTreeNode virtualRoot = null; //虚拟根节点 private SpecialViewManager specialViewManager = null; //特殊权限管理器 #endregion #region 公有方法 /// <summary> /// 获取虚拟根节点 /// </summary> /// <returns></returns> public TaskTreeNode getVirtualRoot() { return virtualRoot; } /// <summary> /// 获取特殊权限管理器 /// </summary> /// <returns></returns> public SpecialViewManager getSpecialViewManager() { return specialViewManager; } /// <summary> /// 增加任务树 /// </summary> /// <param name="root">根任务节点</param> public void AddRootTree(TaskTreeNode root) { lock (this) { virtualRoot.Children.Add(root); root.Parent = virtualRoot; } this.UpdateNode(root, true); } /// <summary> /// 从任务树中获取节点 /// </summary> /// <param name="id"></param> /// <returns></returns> public TaskTreeNode getNodeById(int id) { lock (this) { //广度优先搜索 List<TaskTreeNode> todoList = new List<TaskTreeNode>(); todoList.Add(virtualRoot); while (todoList.Count > 0) { TaskTreeNode currNode = todoList[0]; if (currNode.ID == id) return currNode; else { todoList.AddRange(currNode.Children); todoList.Remove(currNode); } } } return null; } /// <summary> /// 增加孩子 /// </summary> /// <param name="parentId">父节点id</param> /// <param name="son">孩子节点</param> /// <returns>是否添加成功</returns> public bool AddSon(int parentId, TaskTreeNode son) { TaskTreeNode parent = this.getNodeById(parentId); return this.AddSon(parent, son); } /// <summary> /// 增加孩子 /// </summary> /// <param name="parent">父节点</param> /// <param name="son">孩子节点</param> /// <returns>是否添加成功</returns> public bool AddSon(TaskTreeNode parent, TaskTreeNode son) { if( parent == null || son == null || parent == virtualRoot || parent.Children.Contains(son)) return false; lock (this) { parent.Children.Add(son); son.Parent = parent; this.UpdateNode(parent); //写持久层 this.UpdateNode(son); } return true; } /// <summary> /// 改变父节点 /// </summary> /// <param name="sonTaskId">孩子节点ID</param> /// <param name="parentTaskId">父节点ID</param> /// <returns></returns> public bool ChangeParent(int sonTaskId, int parentTaskId) { return ChangeParent(getNodeById(sonTaskId), getNodeById(parentTaskId)); } /// <summary> /// 改变父节点 /// </summary> /// <param name="son">孩子节点</param> /// <param name="parent">父节点</param> /// <returns></returns> public bool ChangeParent(TaskTreeNode son, TaskTreeNode parent) { if (son==null||parent==null||son == parent||son.Equals(virtualRoot)||parent.Children.Contains(son)) return false; lock (this) { parent.Children.Add(son); son.Parent.Children.Remove(son); son.Parent = parent; this.UpdateNode(parent, true); this.UpdateNode(son, true); } return true; } /// <summary> /// 删除节点(将更新持久层) /// </summary> /// <param name="id"></param> /// <returns></returns> public bool deleteNodeById(int id) { return this.deleteNode(this.getNodeById(id)); } /// <summary> /// 删除节点(将更新持久层) /// </summary> /// <param name="node"></param> /// <returns></returns> public bool deleteNode(TaskTreeNode node) { if (node==null||node == virtualRoot||node.Parent == null) return false; lock(this) { if(node.Parent.Children.Remove(node)) { this.UpdateNode(node,true); return true; } else return false; } } /// <summary> /// 改变节点状态 /// </summary> /// <param name="node"></param> /// <param name="status"></param> public void updateNodeStatus(TaskTreeNode node,NetVideo.TaskTrace.Enum.TaskStatus status) { lock (this) { node.Status = status; } this.UpdateNode(node,false); } static Object _dalMutex = new object(); /// <summary> /// 从持久层载入任务树 /// </summary> /// <returns></returns> public bool Load() { lock (_dalMutex) { try { virtualRoot = new TaskTreeNode(0, "虚拟根节点", 0, 0, true);//新建虚拟根节点 List<TaskTreeNode> nodes = DataAccess.CreateTaskNode().getAll();//载入任务 //生成任务树 foreach (TaskTreeNode node in nodes) { if (node.ParentId == 0) //根任务 { TreeManager.getInstance().AddRootTree(node); //设置成虚拟根节点孩子 } foreach (TaskTreeNode obj in nodes)//寻找该节点的孩子 { if (obj.ParentId == node.ID) { obj.Parent = node; if (!node.Children.Contains(obj)) node.Children.Add(obj); } } } return true; } catch (Exception e) { return false; } } } /// <summary> /// 向持久层写入任务树 /// 注：游离节点将不会被写入 /// </summary> /// <returns></returns> public bool Save() { lock (_dalMutex) { MysqlTransactionHelper db = new MysqlTransactionHelper(); db.BeginTransaction();//事务启动 try { //广度优先,按层遍历节点 List<TaskTreeNode> todoList = new List<TaskTreeNode>(); todoList.Add(virtualRoot); while (todoList.Count > 0) { TaskTreeNode currNode = todoList[0]; if (currNode != virtualRoot) //不写入根节点 { if (DataAccess.CreateTaskNode().insertOrUpdate(currNode, db) == false) //事务失败 { return false; } } todoList.AddRange(currNode.Children); todoList.Remove(currNode); } db.Commit(); return true; } catch (Exception e) { return false; } } } /// <summary> /// 向持久层更新节点 /// </summary> /// <param name="node"></param> /// <returns>是否成功</returns> public bool UpdateNode(TaskTreeNode node) { lock (_dalMutex) { return DataAccess.CreateTaskNode().insertOrUpdate(node); } } /// <summary> /// 向持久层更新节点 /// </summary> /// <param name="node"></param> /// <param name="sonUpdate">是否更新改节点的孩子节点</param> /// <returns></returns> public bool UpdateNode(TaskTreeNode node, bool sonUpdate) { if (sonUpdate) { lock (_dalMutex) { foreach (TaskTreeNode son in node.Children) { DataAccess.CreateTaskNode().insertOrUpdate(son); } return DataAccess.CreateTaskNode().insertOrUpdate(node); } } else { return this.UpdateNode(node); } } /// <summary> /// 获取节点所在树的根节点 /// </summary> /// <param name="node"></param> /// <returns></returns> public TaskTreeNode getTreeRootNode(TaskTreeNode node) { if (node == null) return null; lock (this) { //向上追溯到最顶层的公开父节点 TaskTreeNode currNode = node; while (currNode.Parent != null && currNode.Parent != virtualRoot) { currNode = currNode.Parent; } return currNode; } } /// <summary> /// 获取节点所在树的根节点 /// </summary> /// <param name="taskId"></param> /// <returns></returns> public TaskTreeNode getTreeRootNode(int taskId) { return this.getTreeRootNode(this.getNodeById(taskId)); } #endregion } }