Summary of my code reviews

Driven by the strong efforts of the leaders, the Department has recently begun to review code.

I had the honor to be a member of the department Code Review Committee, responsible for reviewing the Code of many colleagues. While reviewing others' code, it is also a great improvement for myself.

Now the coding feeling is quite different from that of half a year ago.

 

Then the Review Board selects a short piece of my code as a "Model Code" for the company's chief scientist "God" to review it through its careful review, it even gave me a sigh of relief for the code that I once thought was good. It seems that the code is really not the best, only better.

 

My recent code review summarizes the following suggestions:

 

1. To throw an exception, you must be decisive.

2. The relationship between thread locks needs to be carefully designed.

3. Unnecessary bool is returned.

4. More application language features, such as C # using

5. More careful design is required for exception handling mechanisms

6. Class member instances should not be exposed as much as possible. If necessary, a copy is provided.

7. The comments are not refined enough.

 

A section of my code, written in C # (Manager Part of a tree task system)

 

Using system; <br/> using system. collections. generic; <br/> using system. LINQ; <br/> using system. text; <br/> using netvideo. tasktrace. model; <br/> using netvideo. tasktrace. dal; <br/> using netvideo. tasktrace. common. dbhelper; <br/> namespace netvideo. tasktrace. bll <br/> {<br/> /// <summary> <br/> // Task Tree manager <br/> /// </Summary> <br/> public class treemanager <br/>{< br/> # region Singleton <br/> static PR Ivate treemanager _ instance = NULL; <br/> static private object _ mutex = new object (); <br/> static public treemanager getinstance () <br/>{< br/> If (_ instance = NULL) <br/>{< br/> lock (_ mutex) <br/>{< br/> If (_ instance = NULL) <br/> _ instance = new treemanager (); <br/>}< br/> return _ instance; <br/>}< br/> private treemanager () <br/> {<br/> specialviewmanager = new specialviewman Ager (); <br/> If (! Specialviewmanager. Load () <br/> throw new exception ("Special Permission manager initialization failed! "); <Br/> virtualroot = new tasktreenode (0," virtual root node ", 0, 0, true ); <br/>}< br/> # endregion <br/> # region attributes <br/> private tasktreenode virtualroot = NULL; // virtual root node <br/> private specialviewmanager = NULL; // special permission manager <br/> # endregion <br/> # Region Public method <br/> /// <summary> <br/> // obtain the virtual Root Node <br/> // </Summary> <br/> // <returns> </returns> <br/> Public tasktreenode getvirtualroot () <BR/>{< br/> return virtualroot; <br/>}< br/> /// <summary> <br/> // obtain the special permission manager <br/> /// </Summary> <br/ >/// <returns> </returns> <br/> Public specialviewmanager getspecialviewmanager () <br/>{< br/> return specialviewmanager; <br/>}< br/> /// <summary> <br/> // Add a Task Tree <br/> /// </Summary> <br/> /// <Param name = "root"> root task node </param> <br/> Public void addroottree (tasktreenode root) <br/> {<br/> Lock (this) <br/>{< br/> virtualroot. children. add (Root); <br/> root. parent = virtualroot; <br/>}< br/> This. updatenode (root, true ); <br/>}< br/> /// <summary> <br/> // obtain a node from the Task Tree <br/> /// </Summary> <br/> // <Param name = "ID"> </param> <br/> // <returns> </returns> <br/> Public tasktreenode getnodebyid (int ID) <br/> {<br/> lock (this) <br/> {<br/> // search for breadth first <br/> List <tasktreenode> todolist = New List <tasktreenode> (); <br/> todolist. add (virtualroot); <br/> while (todolist. count> 0) <br/>{< br/> tasktreenode currnode = todolist [0]; <br/> If (currnode. id = ID) <br/> return currnode; <br/> else <br/>{< br/> todolist. addrange (currnode. children); <br/> todolist. remove (currnode); <br/>}< br/> return NULL; <br/>}< br/> /// <summary> <br/> // Add a child <br/> /// </Summary> <B R/> // <Param name = "parentid"> parent node id </param> <br/> // <Param name = "son"> child node </ param> <br/> /// <returns> whether the instance is successfully added </returns> <br/> Public bool addson (INT parentid, tasktreenode son) <br/>{< br/> tasktreenode parent = This. getnodebyid (parentid); <br/> return this. addson (parent, son ); <br/>}< br/> /// <summary> <br/> // Add a child <br/> /// </Summary> <br/>/ // <Param name = "parent"> parent node </param> <br/> /// <Param name = "son"> child node </param> <br/> // <returns> whether the node is successfully added </returns> <br/> Public bool addson (tasktreenode parent, tasktreenode son) <br/>{< br/> If (parent = NULL | son = NULL | parent = virtualroot | parent. children. contains (son) <br/> return false; <br/> lock (this) <br/>{< br/> parent. children. add (son); <br/> son. parent = parent; <br/> This. updatenode (parent); // write persistence layer <br/> This. updateno De (son); <br/>}< br/> return true; <br/>}< br/> /// <summary> <br/> // change the parent node <br/> /// </Summary> <br/> /// <Param name = "sontaskid"> child node id </param> <br/> /// <Param name = "parenttaskid"> parent node id </param> <br/> // <returns> </returns> <br/> Public bool changeparent (INT sontaskid, int parenttaskid) <br/>{< br/> return changeparent (getnodebyid (sontaskid), getnodebyid (parenttaskid); <br/>}< br/>/// <Summary> <br/> // change parent node <br/> /// </Summary> <br/> /// <Param name = "son"> child node </param> <br/> /// <Param name = "parent"> parent node </param> <br/> /// <returns> </returns> <br/> Public bool changeparent (tasktreenode son, tasktreenode parent) <br/>{< br/> If (son = NULL | parent = NULL | son = parent | son. equals (virtualroot) | parent. children. contains (son) <br/> return false; <br/> lock (this) <br/>{< br/> Pare NT. children. add (son); <br/> son. parent. children. remove (son); <br/> son. parent = parent; <br/> This. updatenode (parent, true); <br/> This. updatenode (son, true); <br/>}< br/> return true; <br/>}< br/> /// <summary> <br/> // delete a node (the persistent layer will be updated) <br/> /// </Summary> <br/> /// <Param name = "ID"> </param> <br/> /// <returns> </returns> <br/> Public bool deletenodebyid (int id) <br/>{< br/> return this. deletenod E (this. getnodebyid (ID); <br/>}< br/> // <summary> <br/> // delete a node (the persistent layer will be updated) <br/> // </Summary> <br/> // <Param name = "Node"> </param> <br/> // <returns> </returns> <br/> Public bool deletenode (tasktreenode node) <br/>{< br/> If (node = NULL | node = virtualroot | node. parent = NULL) <br/> return false; <br/> lock (this) <br/>{< br/> If (node. parent. children. remove (node) <br/>{< br/> This. updaten Ode (node, true); <br/> return true; <br/>}< br/> else <br/> return false; <br/>}< br/> // <summary> <br/> // change node status <br/> /// </Summary> <br/> /// <Param name = "Node"> </param> <br/> /// <Param name = "status"> </param> <br/> Public void updatenodestatus (tasktreenode node, netvideo. tasktrace. enum. taskstatus status) <br/>{< br/> lock (this) <br/>{< br/> node. status = status; <br/>}< br/> th Is. updatenode (node, false); <br/>}< br/> static object _ dalmutex = new object (); <br/> // <summary> <br/> // load the task tree from the persistent layer <br/> /// </Summary> <br/> // /<returns> </returns> <br/> Public bool load () <br/>{< br/> lock (_ dalmutex) <br/>{< br/> try <br/>{< br/> virtualroot = new tasktreenode (0, "Virtual root node", 0, 0, true); // create a virtual root node <br/> List <tasktreenode> nodes = dataaccess. createtasknode (). getall (); // load any <Br/> // generate the Task Tree <br/> foreach (tasktreenode node in nodes) <br/>{< br/> If (node. parentid = 0) // root task <br/>{< br/> treemanager. getinstance (). addroottree (node); // set to virtual root node child <br/>}< br/> foreach (tasktreenode OBJ in nodes) // find the child of the node <br/>{< br/> If (obj. parentid = node. ID) <br/>{< br/> obj. parent = node; <br/> If (! Node. children. contains (OBJ) <br/> node. children. add (OBJ); <br/>}< br/> return true; <br/>}< br/> catch (exception e) <br/>{< br/> return false; <br/>}< br/> // <summary> <br/> // write the task tree to the persistent layer <br //> // note: free nodes will not be written into <br/> /// </Summary> <br/> /// <returns> </returns> <br/> Public bool save () <br/>{< br/> lock (_ dalmutex) <br/>{< br/> mysqltransactionhelper DB = N EW mysqltransactionhelper (); <br/> dB. begintransaction (); // start a transaction <br/> try <br/> {<br/> // breadth first, traverse nodes by layer <br/> List <tasktreenode> todolist = new list <tasktreenode> (); <br/> todolist. add (virtualroot); <br/> while (todolist. count> 0) <br/>{< br/> tasktreenode currnode = todolist [0]; <br/> If (currnode! = Virtualroot) // do not write to the root node <br/>{< br/> If (dataaccess. createtasknode (). insertorupdate (currnode, DB) = false) // Transaction Failed <br/>{< br/> return false; <br/>}< br/> todolist. addrange (currnode. children); <br/> todolist. remove (currnode); <br/>}< br/> dB. commit (); <br/> return true; <br/>}< br/> catch (exception e) <br/>{< br/> return false; <br/>}< br/> // <summary> <br/> // forward Layer-long update node <br/> /// </Summary> <br/> /// <Param name = "Node"> </param> <br/> // /<returns> Successful </returns> <br/> Public bool updatenode (tasktreenode node) <br/>{< br/> lock (_ dalmutex) <br/>{< br/> return dataaccess. createtasknode (). insertorupdate (node ); <br/>}< br/> // <summary> <br/> // update a node to the persistent layer <br/> // </Summary> <br/> /// <Param name = "Node"> </param> <br/> /// <Param name = "sonupdate"> whether or not Update the child node of the modified node </param> <br/> // <returns> </returns> <br/> Public bool updatenode (tasktreenode node, bool sonupdate) <br/>{< br/> If (sonupdate) <br/>{< br/> lock (_ dalmutex) <br/> {<br/> foreach (tasktreenode son in node. children) <br/> {<br/> dataaccess. createtasknode (). insertorupdate (son); <br/>}< br/> return dataaccess. createtasknode (). insertorupdate (node); <br/>}< br/> else <br/> {< Br/> return this. updatenode (node ); <br/>}< br/> // <summary> <br/> // obtain the root node of the tree where the node is located <br/> // /</Summary> <br/> /// <Param name = "Node"> </param> <br/> /// <returns> </returns> <br /> Public tasktreenode gettreerootnode (tasktreenode node) <br/>{< br/> If (node = NULL) <br/> return NULL; <br/> lock (this) <br/>{< br/> // trace back to the exposed parent node at the top level <br/> tasktreenode currnode = node; <br/> while (currnod E. parent! = NULL & currnode. parent! = Virtualroot) <br/>{< br/> currnode = currnode. parent; <br/>}< br/> return currnode; <br/>}< br/> // <summary> <br/> // obtain the root node of the tree where the node is located <br/> // /</Summary> <br/> /// <Param name = "taskid"> </param> <br/> /// <returns> </returns> <br /> Public tasktreenode gettreerootnode (INT taskid) <br/>{< br/> return this. gettreerootnode (this. getnodebyid (taskid); <br/>}< br/> # endregion <br/>}< br/>