From 6633b6a1f316dfe3572220208ea51a713697ecc6 Mon Sep 17 00:00:00 2001 From: Twisha Desai Date: Thu, 2 May 2019 10:19:43 -0400 Subject: [PATCH 1/2] Added suggested changes --- .gitignore | 7 ++++++- dot-net/WhatWouldYouChange/INSTRUCTIONS.txt | 23 ++++++++++++++++++++- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index 6c00c877..28f79d32 100644 --- a/.gitignore +++ b/.gitignore @@ -6,4 +6,9 @@ build/ # Package Files # *.jar -*.log \ No newline at end of file +*.log +/.vs +/dot-net/UnitTesting +/dot-net/WhatWouldYouChange/obj +/dot-net/WhatWouldYouChange/bin/Debug +/dot-net/WhatWouldYouChange/.vs/ExampleProgram/v15 diff --git a/dot-net/WhatWouldYouChange/INSTRUCTIONS.txt b/dot-net/WhatWouldYouChange/INSTRUCTIONS.txt index be2263d6..122adf70 100644 --- a/dot-net/WhatWouldYouChange/INSTRUCTIONS.txt +++ b/dot-net/WhatWouldYouChange/INSTRUCTIONS.txt @@ -3,6 +3,27 @@ you think should be made in the section below. Don't spend more than 30 minutes on this entire assignment. Feel free to describe why you made particular design decisions. - Suggested Changes: +1. In the entry point of the app - Program -> Main, + 1.1 The file name is hardcoded and has a typo. + 1.2 Instead of hardcoding, FileName can be passed in as argument (string[] args) +2. In the method ExampleClass.ExampleMethod, + 2.1 There is no check for whether the FileExists. I understand there is try-catch to handle any exception. However, if we add the check and raise or log errors, the code will be much cleaner and easy to maintain. + 2.2. StreamReader is used to read the contents of the file. However there is no use of using syntax. Thus it look like resource will not be disposed properly. Moreover I dont see the File.Close method call to close the file. + 2.3 Instead of StreamReader, File.ReadAllText can be used. The code is less verbose this way and it will take care of disposing resources automatically. It will not be developer's headache to worry about that anymore. + 2.4 The code assumes the file with provide filename is will exists in current run-time working directory (say bin folder) which is not a good assumption. There can be various approaches take to fix this - either have fully qualified path or UNC path or a config param (app.config appSettings) + to store the source directory. Then use Path.Combine to get full path and use it to read the contents of the file. + 2.5 In the catch block the default text string is built using concatenation approach. Instead, we can use verbatim string i.e. var s = @"This is line 1 + This is line 2 + This is line 3 + . + . + . + This is line n"; + We can also use StringBuilder instead. +3. Design Patterns + 3.1 Repository Pattern & Dependency Injection -We can define interface instead of using concrete implementation (ExampleClass). This way we can easy swap underlying way of getting the file contents. Say down the lane we get the contents from say api stream instead of file stream. + This approach will also make our code unit testable. We can easily mock against the interface and test our method or unit of code. + 3.2 Factory Pattern & Inversion of Control Container - Instead of instantiating concrete class instance, we can have IoC container inject/spin-up the concreate objects into our code. + \ No newline at end of file From 0186e5ebc383547738a3ceb6b6a759dff32869b8 Mon Sep 17 00:00:00 2001 From: Twisha Desai Date: Thu, 2 May 2019 10:55:16 -0400 Subject: [PATCH 2/2] added unit tests --- .gitignore | 1 + .../Services/LessonServiceUnitTests.cs | 95 ++++++++++++++++++- .../WriteUnitTest.UnitTests.csproj | 37 ++++++++ dot-net/UnitTesting/WriteUnitTest/Program.cs | 14 ++- .../Repositories/LessonRepository.cs | 3 +- .../Repositories/ModuleRepository.cs | 3 +- .../WriteUnitTest/Services/LessonService.cs | 20 ++-- .../WriteUnitTest/WriteUnitTest.csproj | 2 + .../WriteUnitTest.csproj.FileListAbsolute.txt | 6 ++ 9 files changed, 163 insertions(+), 18 deletions(-) diff --git a/.gitignore b/.gitignore index 28f79d32..330144c7 100644 --- a/.gitignore +++ b/.gitignore @@ -12,3 +12,4 @@ build/ /dot-net/WhatWouldYouChange/obj /dot-net/WhatWouldYouChange/bin/Debug /dot-net/WhatWouldYouChange/.vs/ExampleProgram/v15 +/dot-net/.vs diff --git a/dot-net/UnitTesting/WriteUnitTest.UnitTests/Services/LessonServiceUnitTests.cs b/dot-net/UnitTesting/WriteUnitTest.UnitTests/Services/LessonServiceUnitTests.cs index c016f412..f99d26e9 100644 --- a/dot-net/UnitTesting/WriteUnitTest.UnitTests/Services/LessonServiceUnitTests.cs +++ b/dot-net/UnitTesting/WriteUnitTest.UnitTests/Services/LessonServiceUnitTests.cs @@ -1,13 +1,100 @@ -using Microsoft.VisualStudio.TestTools.UnitTesting; +using FizzWare.NBuilder; +using Moq; +using NUnit.Framework; +using WriteUnitTest.Entities; +using WriteUnitTest.Interfaces; +using WriteUnitTest.Services; namespace WriteUnitTest.UnitTests.Services { - [TestClass] + [TestFixture] public class LessonServiceUnitTests { - [TestMethod] - public void UpdateLessonGrade_Test() + [Test] + public void UpdateLessonGrade_LessonReturnedIsPassed_ShouldSetLessonIsPassedToTrue() { + //Arrange + var mockLessonRepository = new Mock(); + var mockModuleRepository = new Mock(); + var service = new LessonService(mockLessonRepository.Object, mockModuleRepository.Object); + //var testLesson = Builder.CreateNew().Build(); + Lesson testLesson = new Lesson { + LessonId = 1, + Grade = 70.5, + IsPassed = true + }; + mockLessonRepository.Setup(q => q.GetLesson(It.IsAny())).Returns(testLesson); + + //Act + var lesson = service.UpdateLessonGrade(testLesson.LessonId, 80.5); + + //Assert + NUnit.Framework.Assert.That(lesson, Is.Not.Null); + Assert.That(lesson.IsPassed, Is.True); + mockLessonRepository.Verify(x => x.GetLesson(It.Is(q=>q==testLesson.LessonId)), Times.Once, "Missing mandatory call to lesson repository to get lesson."); + mockModuleRepository.Verify(x => x.GetModule(It.IsAny()), Times.Never, "Invalid call made to module repository to get module."); + } + + [Test] + public void UpdateLessonGrade_LessonReturnedIsFailed_ShouldSetLessonIsPassedToFalse() + { + //Arrange + var mockLessonRepository = new Mock(); + var mockModuleRepository = new Mock(); + var service = new LessonService(mockLessonRepository.Object, mockModuleRepository.Object); + //var testLesson = Builder.CreateNew().Build(); + Lesson testLesson = new Lesson + { + LessonId = 1, + Grade = 70.5, + IsPassed = false + }; + mockLessonRepository.Setup(q => q.GetLesson(It.IsAny())).Returns(testLesson); + Module testModule = new Module + { + MinimumPassingGrade = 75 + }; + mockModuleRepository.Setup(q => q.GetModule(It.IsAny())).Returns(testModule); + + //Act + var lesson = service.UpdateLessonGrade(testLesson.LessonId, 74); + + //Assert + NUnit.Framework.Assert.That(lesson, Is.Not.Null); + Assert.That(lesson.IsPassed, Is.False); + mockLessonRepository.Verify(x => x.GetLesson(It.Is(q => q == testLesson.LessonId)), Times.Once, "Missing mandatory call to lesson repository to get lesson."); + mockModuleRepository.Verify(x => x.GetModule(It.IsAny()), Times.Once, "Missing mandatory call to module repository to get module."); + } + + [Test] + public void UpdateLessonGrade_LessonReturnedIsFailed_ShouldSetLessonIsPassedToTrue() + { + //Arrange + var mockLessonRepository = new Mock(); + var mockModuleRepository = new Mock(); + var service = new LessonService(mockLessonRepository.Object, mockModuleRepository.Object); + //var testLesson = Builder.CreateNew().Build(); + Lesson testLesson = new Lesson + { + LessonId = 1, + Grade = 70.5, + IsPassed = false + }; + mockLessonRepository.Setup(q => q.GetLesson(It.IsAny())).Returns(testLesson); + Module testModule = new Module + { + MinimumPassingGrade = 70 + }; + mockModuleRepository.Setup(q => q.GetModule(It.IsAny())).Returns(testModule); + + //Act + var lesson = service.UpdateLessonGrade(testLesson.LessonId, 74); + + //Assert + NUnit.Framework.Assert.That(lesson, Is.Not.Null); + Assert.That(lesson.IsPassed, Is.True); + mockLessonRepository.Verify(x => x.GetLesson(It.Is(q => q == testLesson.LessonId)), Times.Once, "Missing mandatory call to lesson repository to get lesson."); + mockModuleRepository.Verify(x => x.GetModule(It.IsAny()), Times.Once, "Missing mandatory call to module repository to get module."); } } } \ No newline at end of file diff --git a/dot-net/UnitTesting/WriteUnitTest.UnitTests/WriteUnitTest.UnitTests.csproj b/dot-net/UnitTesting/WriteUnitTest.UnitTests/WriteUnitTest.UnitTests.csproj index f3d83f84..76128198 100644 --- a/dot-net/UnitTesting/WriteUnitTest.UnitTests/WriteUnitTest.UnitTests.csproj +++ b/dot-net/UnitTesting/WriteUnitTest.UnitTests/WriteUnitTest.UnitTests.csproj @@ -1,5 +1,6 @@  + Debug AnyCPU @@ -16,6 +17,8 @@ $(ProgramFiles)\Common Files\microsoft shared\VSTT\$(VisualStudioVersion)\UITestExtensionPackages False UnitTest + + true @@ -35,7 +38,26 @@ 4 + + ..\packages\Castle.Core.4.3.1\lib\net45\Castle.Core.dll + + + ..\packages\NBuilder.6.0.0\lib\net40\FizzWare.NBuilder.dll + + + ..\packages\Moq.4.10.1\lib\net45\Moq.dll + + + ..\packages\NUnit.3.11.0\lib\net45\nunit.framework.dll + + + + ..\packages\System.Runtime.CompilerServices.Unsafe.4.5.0\lib\netstandard2.0\System.Runtime.CompilerServices.Unsafe.dll + + + ..\packages\System.Threading.Tasks.Extensions.4.5.1\lib\netstandard2.0\System.Threading.Tasks.Extensions.dll + @@ -55,6 +77,15 @@ + + + + + + {00a40a05-8314-4f25-a444-46ddeac3497e} + WriteUnitTest + + @@ -75,6 +106,12 @@ + + + This project references NuGet package(s) that are missing on this computer. Use NuGet Package Restore to download them. For more information, see http://go.microsoft.com/fwlink/?LinkID=322105. The missing file is {0}. + + +