代碼細節重構：請對我的代碼指手划腳（二）

“請對我的代碼指手划腳”是我們群內搞的一個不定期的常規性活動，以代碼審閱和細節重構為主線，大家可以自由發表自己的意見和建議，也算得上是一種思維風暴。感覺到這個活動很有意義，有必要總結並記錄下來。

目標代碼

public static bool Serialize(Object obj, string fullname)
{
    FileStream filestream = new FileStream(fullname, FileMode.Create, FileAccess.Write);
    BinaryFormatter binaryformatter = new BinaryFormatter();

    binaryformatter.Serialize(filestream, obj);

    if (filestream == null)
    {
        return false;
    }
    else
    {
        filestream.Close();

        return true;
    }
}

看法一——@稻草人

1、沒有對傳入參數 obj 和 fullname 進行驗證。
2、在本段實例代碼中，不可能出現filestream 為空的情況，要么調用FileStream會直接拋出異常，中止代碼的執行。那么，根據filestream是否為空，來返回true或者false就顯得無意義。
3、BinaryFormatter 的 Serialize 倒是有可能會拋出異常，可視函數需要是否捕獲該異常。
4、對於實現了IDispose接口的對象，在不使用時，應該顯示調用Dispose，以釋放資源。

public static void Serialize(Object obj, string fullname)
{
    if (obj == null) throw new ArgumentNullException("obj");
    if (string.IsNullOrEmpty(fullname)) throw new ArgumentNullException("fullname");

    using (FileStream filestream = new FileStream(fullname, FileMode.Create, FileAccess.Write))
    {
        BinaryFormatter binaryformatter = new BinaryFormatter();

        binaryformatter.Serialize(filestream, obj);
    }
}

看法二——@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，它在項目中需要或已經被他人引用，我們不能在改變這個邏輯的前提下進行重構，因此@稻草人的做法就不妥了。

重構后代碼如下：

public static bool Serialize(Object obj, string fileName)
{
    if (obj == null) throw new ArgumentNullException("obj");

    // if (fileName == null) throw new ArgumentNullException("fileName");
    if (String.IsNullOrWhiteSpace(fileName)) throw new ArgumentOutOfRangeException("fileName");

    FileStream filestream = null;

    try
    {
        filestream = new FileStream(fileName, FileMode.Create, FileAccess.Write);

        new BinaryFormatter().Serialize(filestream, obj);
    }
    catch
    {
        return false;
    }
    finally
    {
        if (filestream != null) filestream.Close();
    }

    return true;
}

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