{"id":153,"date":"2010-11-26T12:06:00","date_gmt":"2010-11-26T12:06:00","guid":{"rendered":"https:\/\/pagan-gerbil.net\/?p=153"},"modified":"2010-11-26T12:06:00","modified_gmt":"2010-11-26T12:06:00","slug":"learning-unit-testing-v-refactoring-mistakes","status":"publish","type":"post","link":"https:\/\/www.pagan-gerbil.net\/?p=153","title":{"rendered":"Learning Unit Testing V &#8211; Refactoring Mistakes"},"content":{"rendered":"<p>I implied in the last post that I had made a mistake when I moved the MoveModel code into the Model class from the MovementManager class. Now, tests on the MovementManager class are actually testing functionality in the Model class. I also discovered that I am not properly testing the \u2018GetDistanceFrom\u2019 method with known data at all. This will have to be corrected.<\/p>\n<p>Since I already have Model implementing an IModel interface, it\u2019ll be easy enough to mock the Model part in the MovementManager. To see what tests are reaching into Model.MoveModel, I can just alter that method to throw an exception and see which tests fail.<\/p>\n<p><img loading=\"lazy\" decoding=\"async\" title=\"image\" style=\"border-top: 0px; border-right: 0px; background-image: none; border-bottom: 0px; padding-top: 0px; padding-left: 0px; border-left: 0px; display: inline; padding-right: 0px\" border=\"0\" alt=\"image\" src=\"https:\/\/pagan-gerbil.net\/wp-content\/uploads\/2017\/01\/image-1.png\" width=\"600\" height=\"83\"><\/p>\n<p>Since this functionality has been moved to the Model class, these tests should actually be made against the Model class itself. Simple? Yes, but incorrect, as I found out next \u2013 it still fails on one condition and checking further, I don\u2019t believe it\u2019s checking the other one correctly. The validation of whether or not a model may make a move (ie, the distance is still under it\u2019s Movement rate) is done in the MovementManager. My two options at this point are: move the validation back to the MovementManager and do a lot of mocking around the Model, or to move that validation into the Model as a ValidateMove method. Then the movement manager really becomes a mere progress tracker for the Movement phase&#8230; but I think that\u2019s probably the best idea in this situation. Just one more test to move over (MoveModel_CannotMoveFurtherThanModelsMovement), then the GetDistanceFrom method needs to be tested. I think the following cases will need to be covered:<\/p>\n<ul>\n<li>Full three-dimensional movement (all values change)\n<li>Two-dimensional movement (one value doesn\u2019t change)\n<li>One-dimensional movement (only one value <strong>does<\/strong> change)\n<li>Cover reverse cases of each of the above (so the direction is backwards) <\/li>\n<\/ul>\n<p>This test (worked on with a calculator and doodles to prove my working), however:<\/p>\n<pre class=\"code\">[<span style=\"color: #2b91af\">TestMethod<\/span>]\n<span style=\"color: blue\">public void <\/span>GetDistanceFrom_CorrectAnswerWhenAllValuesChange()\n{\n<span style=\"color: #2b91af\">Model <\/span>testModel = <span style=\"color: blue\">new <\/span><span style=\"color: #2b91af\">Model<\/span>();\ntestModel.PositionX = 1;\ntestModel.PositionY = 3;\ntestModel.PositionZ = 4;\n<span style=\"color: blue\">double <\/span>result = testModel.GetDistanceFrom(2, 5, 7);\n<span style=\"color: #2b91af\">Assert<\/span>.AreEqual(3.74, <span style=\"color: #2b91af\">Math<\/span>.Round(result, 2));\n}<\/pre>\n<p>Shows me that either my test calculation is wrong or my original working is wrong. In the GetDistanceFrom method, this line<\/p>\n<pre class=\"code\">distance = <span style=\"color: #2b91af\">Math<\/span>.Sqrt(<span style=\"color: #2b91af\">Math<\/span>.Sqrt((differenceX * differenceX) + (differenceY * differenceY)) + (differenceZ * differenceZ));<\/pre>\n<p>Should actually be<\/p>\n<pre class=\"code\">distance = <span style=\"color: #2b91af\">Math<\/span>.Sqrt((differenceX * differenceX) + (differenceY * differenceY) + (differenceZ * differenceZ));<\/pre>\n<p>So I got the calculation wrong. Now the test passes, but another one breaks.<\/p>\n<p><img loading=\"lazy\" decoding=\"async\" title=\"image_3\" style=\"border-top: 0px; border-right: 0px; background-image: none; border-bottom: 0px; padding-top: 0px; padding-left: 0px; border-left: 0px; display: inline; padding-right: 0px\" border=\"0\" alt=\"image_3\" src=\"https:\/\/pagan-gerbil.net\/wp-content\/uploads\/2017\/01\/image_3-1.png\" width=\"374\" height=\"53\"><\/p>\n<p>This is because that movement calculation now requires the model to move further than it\u2019s allowed distance. Fixing that test\u2019s values (from 3, 4, 5 to 2, 2, 2) and adding in the following tests:<\/p>\n<pre class=\"code\">[<span style=\"color: #2b91af\">TestMethod<\/span>]\n<span style=\"color: blue\">public void <\/span>GetDistanceFrom_CorrectAnswerWhenTwoValuesChange()\n{\n<span style=\"color: #2b91af\">Model <\/span>testModel = <span style=\"color: blue\">new <\/span><span style=\"color: #2b91af\">Model<\/span>();\ntestModel.PositionX = 1;\ntestModel.PositionY = 3;\ntestModel.PositionZ = 4;\n<span style=\"color: blue\">double <\/span>result = testModel.GetDistanceFrom(1, 5, 7);\n<span style=\"color: #2b91af\">Assert<\/span>.AreEqual(3.61, <span style=\"color: #2b91af\">Math<\/span>.Round(result, 2));\n}\n[<span style=\"color: #2b91af\">TestMethod<\/span>]\n<span style=\"color: blue\">public void <\/span>GetDistanceFrom_CorrectAnswerWhenOneValueChanges()\n{\n<span style=\"color: #2b91af\">Model <\/span>testModel = <span style=\"color: blue\">new <\/span><span style=\"color: #2b91af\">Model<\/span>();\ntestModel.PositionX = 1;\ntestModel.PositionY = 3;\ntestModel.PositionZ = 4;\n<span style=\"color: blue\">double <\/span>result = testModel.GetDistanceFrom(1, 3, 7);\n<span style=\"color: #2b91af\">Assert<\/span>.AreEqual(3, <span style=\"color: #2b91af\">Math<\/span>.Round(result, 2));\n}\n[<span style=\"color: #2b91af\">TestMethod<\/span>]\n<span style=\"color: blue\">public void <\/span>GetDistanceFrom_CorrectAnswerWhenAllValuesChange_Reversed()\n{\n<span style=\"color: #2b91af\">Model <\/span>testModel = <span style=\"color: blue\">new <\/span><span style=\"color: #2b91af\">Model<\/span>();\ntestModel.PositionX = 2;\ntestModel.PositionY = 5;\ntestModel.PositionZ = 7;\n<span style=\"color: blue\">double <\/span>result = testModel.GetDistanceFrom(1, 3, 4);\n<span style=\"color: #2b91af\">Assert<\/span>.AreEqual(3.74, <span style=\"color: #2b91af\">Math<\/span>.Round(result, 2));\n}\n[<span style=\"color: #2b91af\">TestMethod<\/span>]\n<span style=\"color: blue\">public void <\/span>GetDistanceFrom_CorrectAnswerWhenTwoValuesChange_Reversed()\n{\n<span style=\"color: #2b91af\">Model <\/span>testModel = <span style=\"color: blue\">new <\/span><span style=\"color: #2b91af\">Model<\/span>();\ntestModel.PositionX = 1;\ntestModel.PositionY = 5;\ntestModel.PositionZ = 7;\n<span style=\"color: blue\">double <\/span>result = testModel.GetDistanceFrom(1, 3, 4);\n<span style=\"color: #2b91af\">Assert<\/span>.AreEqual(3.61, <span style=\"color: #2b91af\">Math<\/span>.Round(result, 2));\n}\n[<span style=\"color: #2b91af\">TestMethod<\/span>]\n<span style=\"color: blue\">public void <\/span>GetDistanceFrom_CorrectAnswerWhenOneValueChanges_Reversed()\n{\n<span style=\"color: #2b91af\">Model <\/span>testModel = <span style=\"color: blue\">new <\/span><span style=\"color: #2b91af\">Model<\/span>();\ntestModel.PositionX = 1;\ntestModel.PositionY = 3;\ntestModel.PositionZ = 7;\n<span style=\"color: blue\">double <\/span>result = testModel.GetDistanceFrom(1, 3, 4);\n<span style=\"color: #2b91af\">Assert<\/span>.AreEqual(3, <span style=\"color: #2b91af\">Math<\/span>.Round(result, 2));\n}<\/pre>\n<p>Gives me 19 passes out of 19 tests. Excellent \u2013 I was expecting some of the 0-distance values being squared and rooted to throw some sort of calculation exception in .Net, but I can get along with this result.<\/p>\n<p>This leaves me with my refactoring mess cleaned up, a calculation error solved, a method I somehow missed tests for covered properly, and all ready to tackle the last part of page 10 \u2013 a much longer page than I had initially expected. \u201cOoh look\u201d, said the me at the beginning of the week, \u201chalf the page is picture and prose, and the other half is mostly descriptive. One or two posts to talk about design ideas and I\u2019ll be on page 20 in a fortnight!\u201d Isn\u2019t hindsight awesome.<\/p>\n<p><em>This is a learning project I am documenting in order to teach myself TDD and unit testing \u2013 as I publish this, I have already written many parts in advance but I want to know what I\u2019m doing wrong! If you see improvements or corrections, either to the code or the process, please leave a comment and let me know! Introduction to this project can be found <a href=\"https:\/\/pagan-gerbil.net\/?p=106\">here<\/a>.<\/em><\/p>\n","protected":false},"excerpt":{"rendered":"<p>I implied in the last post that I had made a mistake when I moved the MoveModel code into the Model class from the MovementManager class. Now, tests on the MovementManager class are actually testing functionality in the Model class. I also discovered that I am not properly testing the \u2018GetDistanceFrom\u2019 method with known data &hellip; <\/p>\n<p class=\"link-more\"><a href=\"https:\/\/www.pagan-gerbil.net\/?p=153\" class=\"more-link\">Continue reading<span class=\"screen-reader-text\"> &#8220;Learning Unit Testing V &#8211; Refactoring Mistakes&#8221;<\/span><\/a><\/p>\n","protected":false},"author":1,"featured_media":0,"comment_status":"open","ping_status":"closed","sticky":false,"template":"","format":"standard","meta":{"footnotes":""},"categories":[14],"tags":[43,62,63,64],"series":[73],"_links":{"self":[{"href":"https:\/\/www.pagan-gerbil.net\/index.php?rest_route=\/wp\/v2\/posts\/153"}],"collection":[{"href":"https:\/\/www.pagan-gerbil.net\/index.php?rest_route=\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/www.pagan-gerbil.net\/index.php?rest_route=\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/www.pagan-gerbil.net\/index.php?rest_route=\/wp\/v2\/users\/1"}],"replies":[{"embeddable":true,"href":"https:\/\/www.pagan-gerbil.net\/index.php?rest_route=%2Fwp%2Fv2%2Fcomments&post=153"}],"version-history":[{"count":0,"href":"https:\/\/www.pagan-gerbil.net\/index.php?rest_route=\/wp\/v2\/posts\/153\/revisions"}],"wp:attachment":[{"href":"https:\/\/www.pagan-gerbil.net\/index.php?rest_route=%2Fwp%2Fv2%2Fmedia&parent=153"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/www.pagan-gerbil.net\/index.php?rest_route=%2Fwp%2Fv2%2Fcategories&post=153"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/www.pagan-gerbil.net\/index.php?rest_route=%2Fwp%2Fv2%2Ftags&post=153"},{"taxonomy":"series","embeddable":true,"href":"https:\/\/www.pagan-gerbil.net\/index.php?rest_route=%2Fwp%2Fv2%2Fseries&post=153"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}