代码细节重构：请对我的代码指手划脚（二）

“请对我的代码指手划脚”是我们群内搞的一个不定期的常规性活动，以代码审阅和细节重构为主线，大家可以自由发表自己的意见和建议，也算得上是一种思维风暴。感觉到这个活动很有意义，有必要总结并记录下来。

目标代码

 1 public static bool Serialize(Object obj, string fullname)
 2 {
 3     FileStream filestream = new FileStream(fullname, FileMode.Create, FileAccess.Write);
 4     BinaryFormatter 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 }

看法一——@稻草人

1、没有对传入参数 obj 和 fullname 进行验证。
2、在本段实例代码中，不可能出现filestream 为空的情况，要么调用FileStream会直接抛出异常，中止代码的执行。那么，根据filestream是否为空，来返回true或者false就显得无意义。
3、BinaryFormatter 的 Serialize 倒是有可能会抛出异常，可视函数需要是否捕获该异常。
4、对于实现了IDispose接口的对象，在不使用时，应该显示调用Dispose，以释放资源。

 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 filestream = new FileStream(fullname, FileMode.Create, FileAccess.Write))
 7     {
 8         BinaryFormatter binaryformatter = new BinaryFormatter();
 9 
10         binaryformatter.Serialize(filestream, obj);
11     }
12 }

看法二——@freeworklife

存在的问题：

1：从程序执行的顺序上说：第6行的判断应该在初始化filestream之后就要判断，这样即使失败了也不用再初始化

• BinaryFormatter binaryformatter = new BinaryFormatter();
• binaryformatter.Serialize(filestream, obj);

他们了。

2：binaryformatter.Serialize(filestream, obj);

这个是序列化是否成功不知道应该有个判断。

3：没有对传过来的变量进行判断处理。

原因： 没有考虑到意外的情况的发生，考虑不周全，要想把一个函数写好，就要考虑到它可能出现的各种情况，并给与相应的处理，这样函数才是最高效安全实用的。 我个人的建议是： 在写函数时要有个整体的把握前后逻辑要清楚，每条语句都有它功能和可能出现的bug，要全面的考虑才能让函数更健壮。

看法三——@游戏风

1、操作存在大量容易出异常的地方，没有对异常进行处理，比如new FileStream的时候，binaryformatter.Serialize的时候都容易出异常。(filestream == null)
这个判断应该在创建FileStream之后就判断，不然无端的对着空FileStream对象写入序列化数据，铁定要出异常。
2、fullname没有检查是否为合法的路径名称，所以很容易出错，再就是没有进行access权限异常的判断。binaryformatter.Serialize的情况也是没有考虑传入的实例对象是否允许序列化，如果传入一个不能序列化的对象，必然异常。

老陈的看法

缺陷主要有4个：
1、FileStream应当置入using语句；
2、"filestream == null"永远不可能成立！要么抛出异常，反正不会是null；
3、基于第二点"return false;" 永远不会执行！
4、输入参数的检查，至于其他的相对来说不算重要了；

不过，上述三种看法中，有一个问题被忽略或被扭曲了，即方法的返回值是bool，它在项目中需要或已经被他人引用，我们不能在改变这个逻辑的前提下进行重构，因此@稻草人的做法就不妥了。

重构后代码如下：

 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 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 }

原贴地址：http://newsql.cn/thread-81-1-1.html

【IT技术综合群，纯粹.NET技术讨论请入本群】
网鸟-刺客巅峰：47700865

---

【NoSQL群】NoSQL系列技术QQ群：
一群：23152359（满员）
二群：193713524（接近满员）
三群：79377097（强烈推荐）
四群：191845335（新建）
入群写明理由，否则一律拉黑！

---

关于老陈：一只不小心踏入互联网十二年的老猴子，喜欢在群里跟大家伙儿研究和分享各种V。天造的懒汉，十足的吃货！

分类: C#, 架构设计