ShareXのソースを眺める④～適当リファクタ～

③
のつづき

前回やったことから・・・

新しくErrorクラスを切り出したのはいいけど、テスト書いてない。
元々のテストをそのまま流用できるだろうからひとまずテストで保護をする。

対象クラス。

なんてことないね。

    public class Errors
    {
        /// <remarks>keep compatibility public</remarks>
        public List<string> _value { get; set; }

        public Errors()
        {
            _value = new List<string>();
        }

        public override string ToString()
        {
            return string.Join(Environment.NewLine + Environment.NewLine, _value.ToArray());
        }

        public bool IsErrorsHasLeastOneError
        {
            get
            {
                if (_value == null)
                {
                    return false;
                }
                if (!_value.Any())
                {
                    return false;
                }
                return true;
            }
        }
    }

テスト書いている最中にErrorsのコンストラクタが足りてないことにきづく
public Errors(List values)
{
_value = values;
}
最低限これはないとおかしいかな。
public Errors(List values)
{
Make(values);
}
private void Make(List values)
{
_value = values;
}
「レガシーコード改善ガイド」によると、コンストラクタにそのまま書くと面倒なことになるらしい。
Factoryをつくるほどでもないけど、分離しておく。

もう一つきづいた。
public bool IsErrorsHasLeastOneError
{
get
{
if (_value == null)
{
return false;
}
if (!_value.Any())
{
return false;
}
return true;
}
}
これメソッド名がおかしい。Errorsにあるんだから、IsErrors。いらない。

ということで、Errorsクラスのテスト

    [TestClass]
    public class ErrorsTest
    {
        Errors obj { get; set; }
        [TestInitialize]
        public void Initialize()
        {
            // none
        }

        [TestMethod]
        public void Errors_ToString_01()
        {
            List<string> expectedList = new List<string>() { "dummy_value" };
            obj = new Errors(expectedList);

            string expected = string.Join(Environment.NewLine + Environment.NewLine, expectedList.ToArray());
            string actual = obj.ToString();

            Assert.AreEqual(expected, actual);
        }

        [TestMethod]
        public void Errors_IsErrorsHasLeastOneError_01()
        {
            obj = new Errors(null);
            Assert.AreEqual(false, obj.HasLeastOneError);

            obj = new Errors(new List<string>());
            Assert.AreEqual(false, obj.HasLeastOneError);

            obj = new Errors(new List<string>() { "dummy_value" });
            Assert.AreEqual(true, obj.HasLeastOneError);
        }
    }

やっぱりメソッドの役割が小さいとテスト書くのもらくだな。

Errorsも終わったのでUploadResultクラスに戻って「IsSuccess」を見てみる。



        private bool isSuccess;
        public bool IsSuccess

        {

            get

            {

                return isSuccess && !string.IsNullOrEmpty(Response);

            }

            set

            {

                isSuccess = value;

            }

        }

なんだか絶妙にいやらしい感じのプロパティ。
なんでこんなところでResponseのNull判定がいるんだろうか。とりあえずテストを書いて現状を保存しよう。

        [TestMethod]
        public void UploadResult_IsSuccess()
        {
            Assert.AreEqual(false, Invoke_IsSuccess(false, null));
            Assert.AreEqual(false, Invoke_IsSuccess(true, null));
            Assert.AreEqual(false, Invoke_IsSuccess(false, string.Empty));
            Assert.AreEqual(false, Invoke_IsSuccess(true, string.Empty));
            Assert.AreEqual(false, Invoke_IsSuccess(false, "dummy_response"));
            Assert.AreEqual(true, Invoke_IsSuccess(true, "dummy_response"));
        }

        private bool Invoke_IsSuccess(bool isSuccess, string response)
        {
            obj.IsSuccess = isSuccess;
            obj.Response = response;
            return obj.IsSuccess;
        }

連続的にパラメータを切り替えるときはInvokeメソッドを作ったほうが見やすいと思う。
TestCase、TestCaseSource使うのもありだけど、なるべく余計なものをインポートしなくてもいい方法を選択する。
テストで保護もできたので軽くリファクタする。

        private bool _isSuccess;
        public bool IsSuccess
        {
            get
            {
                if(!_isSuccess)
                {
                    return false;
                }

                if(string.IsNullOrEmpty(Response))
                {
                    return false;
                }

                return true;
            }
            set
            {
                _isSuccess = value;
            }
        }

男はだまってガード節。お互いの条件が密接な場合はわざとつなげたりするんだけど、
それでもそれならメソッド切るし、末端はガード節「&& ||」を書かない。（書いたほうが見やすい場合もある気がするんだけど、今のところほとんど遭遇してない）

Responseの文字列が影響してるなら、そもそもResponse自体がIsSuccessを返せばいいような気がするんじゃが。
いまの状態を利用してどうにかしている場所があるんだろうなぁ。手が出せず。
見る限り、SendRequestFileがらみのResult（まーそうよね。クラス名的に）
として使われているが・・・この辺の依存を外そうと思ったら使われているところ全部テスト組んでいかないといけない・・・

なんだか嫌な空気はでてたけど、案の定参照どうなってるのか使う側にゆだねられてて、
入れ物になりかけてるなぁ。そんな感じ。

今回はこの辺で・・・