Code details refactoring: Please point out my code (2)

"PleaseCodeIt is an irregular routine activity in our group. With code review and detail refactoring as the main line, you can freely express your opinions and suggestions, it is also a storm of thinking. It makes sense to summarize and record this activity.

Target code

 1   Public   Static   Bool Serialize (Object OBJ, String  Fullname)  2   {  3 Filestream = New  Filestream (fullname, filemode. Create, fileaccess. Write );  4 Binaryformatter = New  Binaryformatter ();  5   6   Binaryformatter. serialize (filestream, OBJ );  7   8       If (Filestream = Null  )  9   {  10           Return   False ;  11   }  12       Else  13   {  14   Filestream. Close ();  15   16           Return   True  ;  17   }  18 }

Opinion 1 -- @ scarecrow

1. The input parameters OBJ and fullname are not verified.
2. In this section of the Instance code, it is impossible for filestream to be empty, or calling filestream directly throws an exception and aborts code execution. It is meaningless to return true or false based on whether filestream is null.
3. The serialize of binaryformatter may throw an exception. The visual function needs to capture the exception.
4. For objects that implement the idispose interface, call dispose to release resources should be displayed when not in use.

 1   Public   Static   Void Serialize (Object OBJ, String  Fullname)  2   {  3       If (OBJ = Null )Throw   New Argumentnullexception ( "  OBJ  "  );  4       If ( String . Isnullorempty (fullname )) Throw   New Argumentnullexception ( "  Fullname  "  );  5  6       Using (Filestream = New  Filestream (fullname, filemode. Create, fileaccess. Write ))  7   {  8 Binaryformatter = New  Binaryformatter ();  9   10   Binaryformatter. serialize (filestream, OBJ );  11   } 12 }

Opinion 2 -- @ freeworklife

Problems:

1: slaveProgramIn the execution sequence, the judgment of the 6th rows should be determined after the filestream is initialized, so that no Initialization is required even if the initialization fails.

• Binaryformatter = new binaryformatter ();
• Binaryformatter. serialize (filestream, OBJ );

They are.

2: binaryformatter. serialize (filestream, OBJ );

This is whether the serialization is successful or not. I don't know whether there should be a judgment.

3: No judgment is made on the passed variables.

Cause: the unexpected situation is not taken into account. If you want to write a function well, you must consider the various possible situations and give corresponding processing, in this way, functions are the most efficient, secure, and practical. My personal suggestion is: when writing a function, you must have a clear understanding of the logic before and after the function. Each statement has its functions and possible bugs, A comprehensive consideration is required to make the function more robust.

Opinion 3-@ game spirit

1. There are a lot of operations that are prone to exceptions and are not handled. For example, when new filestream is used, binaryformatter. serialize is prone to exceptions. (Filestream = NULL)
This judgment should be determined after the filestream is created. Otherwise, the serialization data is written to the empty filestream object without any reason, and an exception is required.
2. The fullname does not check whether it is a valid path name, so it is easy to make an error. In addition, the access permission exception is not judged. In the case of binaryformatter. serialize, it does not take into account whether the imported instance object can be serialized. If an object that cannot be serialized is imported, an exception is inevitable.

Chen's opinion

There are four major defects:
1. filestream should be placed into the using statement;
2. "filestream = NULL" is never possible! Or throw an exception. It is not null;
3. Based on the second "Return false;" will never be executed!
4. Check the input parameters. Other parameters are relatively unimportant;

However, one of the above three ideas is ignored or distorted, that is, the return value of the method is bool, which needs or has been referenced by others in the project, we cannot reconstruct the logic on the premise of changing this logic, so @ Scarecrow's approach is inappropriate.

The code after reconstruction is as follows:

 1   Public   Static   Bool Serialize (Object OBJ, String Filename)  2   {  3       If (OBJ = Null ) Throw   New Argumentnullexception ( "  OBJ  "  );  4   5       // If (filename = NULL) throw new argumentnullexception ("FILENAME "); 6       If (String. isnullorwhitespace (filename )) Throw   New Argumentoutofrangeexception ( "  Filename  "  );  7   8 Filestream = Null  ;  9   10       Try 11   {  12 Filestream = New  Filestream (filename, filemode. Create, fileaccess. Write );  13   14           New  Binaryformatter (). serialize (filestream, OBJ );  15   }  16       Catch  17   {  18          Return   False  ;  19   }  20       Finally  21   {  22           If (Filestream! = Null  ) Filestream. Close ();  23   }  24   25      Return   True  ;  26 }

Original post address: http://newsql.cn/thread-81-1-1.html