“请对我的代码指手划脚”是我们群内搞的一个不定期的常规性活动,以代码审阅和细节重构为主线,大家可以自由发表自己的意见和建议,也算得上是一种思维风暴。感觉到这个活动很有意义,有必要总结并记录下来。
目标代码
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 else13 {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 try11 {12 filestream = new FileStream(fileName, FileMode.Create, FileAccess.Write);13 14 new BinaryFormatter().Serialize(filestream, obj);15 }16 catch17 {18 return false;19 }20 finally21 {22 if (filestream != null) filestream.Close();23 }24 25 return true;26 }
原贴地址: