The Project: https://github.com/egnomerator/WinZipKata

This documentation is supplemental--its purpose is to provide some explanation of the work throughout the project.

• the purpose of this project was to practice writing clean code (keeping in mind the guiding principles in Uncle Bob's book Clean Code)
  • in spite of this purpose, there's still plenty further refactoring and improvements that could be made, but i had to stop somewhere
• one thing i noticed when reading through the book was the inconvenience of following the cross-references between refactoring notes and code exhibits
  • i thought how nice it would be if there were a GitHub-i-fied version of the clean code book where i could look at the code with convenient links to commits
  • this is NOT that GitHub-i-fied version lol, but it's my own tiny version inspired by the book, not only to practice, but also to have something that might be worth coming back to later
• something interesting and cool i noticed by the end of this project
  • presumably due to attempting to maintain fairly clean code, i noticed that the later commits were easier and quicker than the earlier commits even though the later commits were arguably introducing the most complexity
  • it felt like maintaining the fundamentals paid off when it was time to do the complicated work, which is of course the idea behind maintaining clean code

• A simple Windows Forms app that
  • let's a user provide a path, which is expected to contain sub-folders
  • the app detects and displays the sub-folders in a list
  • the user can then click a button to kick off processing which zips each sub-folder placing the resulting zip file into a new output folder
  • the output folder will contain a zip file for each sub-folder displayed in the app (each sub-folder the app detected within the user-provided path)
  • the user has the ability to abort this process of zipping all the folders
  • as the app finishes zipping each sub-folder, it highlights the sub-folder green/red to indicate success/failure
• My goals during writing this app
  • practice TDD, refactoring, other general clean-code practices like using quality names (of user-defined types, methods, variables, etc.), OOP, SOLID, DRY, YAGNI
  • touch on (emphasis on "touch") a few areas of programming disciplines
    • e.g. file I/O, async/await, parallelism, refactoring to patterns, integrating 3rd party libraries

• 2666a30 - Initial setup
• 0466930 & e401931 - in both of these commits i'm experimenting with Project "Renaming"
  • i initially created a "WinZipKata.Tests" project
  • i wanted to rename it to "WinZipKata.UnitTests"
  • solution: Removed existing project and added new project with the name i wanted
    • i was concerned that i would miss some detail(s) that i should update to accomplish the rename correctly
    • i didn't yet have anything in the existing project that i needed to preserve so i wasn't risking losing anything important by using the nuclear option
  • Renaming a project can be a pain because of all the places that are affected, many of which require manual work to update
    • this shows how complicated it can be: Solution And Project Renaming
• f5defb8 - Added first failing test
  • here the test calls a method that doesn't even exist yet, ValidateParentPath();
  • (also in this commit i added a directory structure with test data in the test project)
• e41f080 - i created the method simply returning true, just to make the test pass
• 201a9a0 - i added the second failing test
  • this test expects false, since the path doesn't exist
• 0bc6232 - now all tests pass: i updated the method to actually return wether the directory exists
• 18a22bd - added 3rd test which fails: this test expects false, because an "Output" child folder exists within the given folder path
  • in this commit i also did a little bit of clarifying variable renaming and test renaming
• a9c4a07 - made all tests pass
  • here i updated the method to ensure that BOTH
    • the given directory exists AND
    • the directory Doesn't contain a child folder called "Output"
  • the point of this test is to show that we won't process a given ParentPath if it contains an "Output" folder
    • this would indicate that processing has already been done on this path
    • (this is a "business rule" for this app)
• 6f6213f - in this commit i simply decided to add the OpenCover NuGet library for observing code coverage
  • I also installed the VS Extension AxoCover which integrates OpenCover into VS in a nice way
• 5bfacca - REFACTORING
  • this commit was about refactoring while maintaining my passing tests
  • I created a ParentPathValidator class to encapsulate the validation behavior I defined with my tests
  • I created a new test class file for this class (ParentPathValidatorTests)
  • I moved the existing tests (along with setup/teardown) from my placeholder/sandbox test class into ParentPathValidatorTests
    • I updated these tests to use ParentPathValidator as SUT
  • I removed the validation code that was in my placeholder static Zipper class, since it's now where it should be
• b3c1e73 - added new failing test
  • a lot happens in this commit, but it boils down to 2 things
    1. added a new failing test
       • this test was put in the placeholder test class to start defining behavior related to zipping
    2. refactoring/rearranging of test support code and test data
       • i created a test utilities project and referenced OpenCover from here (and removed OpenCover from the other project--only need one project to reference OpenCover)
       • i moved the setup/teardown code into a new Support class in the test utilities project
       • i also did a decent bit of test refactoring to utilize the Support class to hopefully make the tests a little cleaner
       • i moved the test data into the test utilities project
         • this was a mistake that i reversed in a later commit
         • i realized that moving the test data caused the existing tests not to know where the files were anymore
           • interesting note, i made it through one commit before noticing this
           • would have caught it immediately if i had ran all tests before my commit
• e402bac - here i implemented the method so the test could pass
• 0e23a6a - here i realized my mistake in moving the test data, so i put it back where it was
• ae3762c - i added a new test and it passed immediately
  • note: this showed that i didn't technically follow Uncle Bob's flavor of TDD to the letter when i wrote the method being tested
• a69f172 - REFACTORING
  • this commit was about refactoring while maintaining my passing tests
  • similar to the earlier refactoring commit (5bfacca9)
  • i created a new ZipValidator class to encapsulate the behavior i had defined in testing
  • i created a new test class and made this new ZipValidator class the SUT
  • added new helper methods in the test Support class
  • renamed the ValidateParentPath method to ParentPathIsValid which seemed to be more readable as intuitively returning a boolean
  • renamed the not helpful result variable names to isValid in the parent path validation tests
• b6f184b - added new failing test
  • i added the failing unit test--this time i created the method but just didn't implement it
    • earlier times, i would write the unit test and not even create the method so it wouldn't even compile but i would commit anyway
    • this way is probably preferable
    • although if you commit early and often, these slight differences may not be something to worry about
    • but it's definitely better to be in the habit of making the code compile before a commit
    • regardless, this whole commit history is focusing on showing a progression through the process of achieving some level of clean code
  • added a couple new support class methods
    • interesting notes on the new Support class method GetFolderName(string folderPath)
      • this method uses Path.GetFileName() passing in the folderPath
      • given the name GetFileName i think i'm perhaps shoehorning the method for an unintended use
      • i'm trying to make up for it with the clear method name and parameter name, but maybe this is still bad practice
      • but it was interesting to find out that the Path.GetFileName() works this way
• da6f68b - made the failing test pass
  • and another refactoring step:
    • after making the failing test pass, i saw that the method i had just implemented could be reused in an existing method, so i refactored that method to use it
• f13a544 - added new failing test for zip functionality
  • i'd been using the Zipper class as a placeholder so far while i drafted tests until i had a group of tests that felt like a class, then move these tests into that new class
  • at this point i was ready to actually use the Zipper class, so i moved it to the Core project
    • and to this class i added my new method (not implemented) that i wanted to test
  • i also created a dedicated ZipperTests class with my new failing test
  • i also did a couple other odds-and-ends like variable renaming and adding the zip NuGet DotNetZip
• 89029dd - made the failing test pass
  • implemented the method, and renamed the test for clarity (made it match the method being tested)
• f4bed1d - REFACTORING
  • this commit was about refactoring while maintaining my passing tests
  • the Zipper class was still static and i didn't want that
  • i updated the Zipper class to be instantiable and updated the test to instantiate it as SUT
• a481e1d & 5f58787 - created new ParentPath class and failing tests
  • also updated a method name for clarity
• b049495, 6dd9724, 2d28e98 - made ParentPath tests pass
  • also added a UI update method, but this was prematurely written, and I remove it in later commit
• a7e916a - REFACTORING
  • moved tests to a new test class that was made specifically for testing the ParentPath class as SUT
• 7846316
  • removed unnecessary code including a method for updating the UI that was prematurely written
• 41df884 - started settig up the GUI
• 0f55c97 - added 3 new tests planning the behavior i wanted for the new SubFolderTracker class
  • 3 tests: 1. find sub folders, 2. determine is a sub folder is processed 3. get stats on processed sub folders
  • also created 2 new classes as data structures, i intended for the tracker class to use
  • this turned out to just be premature design that didn't pan out
• b157688 - wrote a failing test for the tracker to find sub folders
  • added a new private _subFolders field and a public method to get the sub folders collection
    • i could have just used a public getter/private setter--maybe this would have been better for readability
  • added a custom comparer for NUnit to use for determining equality between 2 List<SubFolder>
• 2fc0030 - implemented the FindSubFolders method to make the failing test pass
  • also made a correction to the test--had setup expected result incorrectly
  • also see the last section of notes in this wiki page for more details (these ending notes are just scratched-out, first-draft quality but could still give more insight into what i was thinking about)
• d56b6ee - deleted the method for finding sub folders and updated the tracker class to require these instead
  • removed the test that covered this method, also removed the custom comparer which was no longer needed
  • this class's purpose was to track progress of processing the sub folders--it didn't seem like this class should also be responsible for determine what the list of sub folders would be
• b76fccf - removed the SubFolderTracker class and corresponding test class
  • at this point i decided to abandon tracking the count or percentage of processed folders, at least for now (and by the end of this small project, i decided not to bother with this)
• 02c5417
  • removed private path field from the ParentPathValidator and the constructor requiring the path string--instead updated the validation method to require the path string
    • this way, when a user needs to provide a different path, this class doesn't have to be created again--we just pass the path to the method
  • also began implementing the app workflow
  • note: whoops, i left a comment in this commit that i should have removed before committing--a todo comment that i no longer needed
• 01df255 - correction to "reset" behavior--when user clears the path field, the list of sub folders should clear
• 1262409
  • moved const string "Output" variable in the ParentPathValidator class from within a method to be a class member
  • added zipping button and functionality
• 6e664a5 - added highlighting of folder names to indicate if they succeeded or failed
• 6834ae9
  • not much here just added a quick check to prevent attempting to zip a folder that doesn't exist
  • side projects are nice--don't need to think about exception handling if i don't feel like it
• a25d90d, 11a684e, and c5ce0aa - refactoring to MVP
  • pushed behavior code from the Form/View class to the new Presenter class and enabled the Presenter to tell the Form/View what to do via the new interface
    • more specifically, i was going for the passive view approach
  • a note on a couple points of further refactoring that could be done
    • the WinZipUI.DisplaySubFolderNames() method first clears the items and then displays each item in the list argument
      • it could be better to have separate WinZipUI.Clear and WinZipUI.Display methods and then have the presenter be responsible for calling clear and then display
    • the WinZipUI.IndicateSubFolderProcessed() method decides a color based on the bool argument
      • it could be better to have separate WinZipUI.IndicateSubFolderSucceeded and WinZipUI.IndicateSubFolderFailed methods and have the presenter call the appropriate method
    • the WinZipUI.DisplayMessage() method applies a default string.Empty value if the provided caption argument is null
      • it could be better to have WinZipUI.DisplayMessage() just use the value and have the presenter handle protecting against null values
  • part of this refactoring was updating the ParentPath class to no longer have any reference to the Forms namespace
• 70dba85 - some readability-focused refactoring
• a387e38 - disabled editing a GUI field during processing; removed an unused class
• 99a70f4
  • the commit note says "... view async await" but should have read "... via async await"
  • implemented async await to free the UI thread during zipping
  • updated the method that applies highlighting to the zipped sub folder name to ensure that it executes on the UI thread
• 8a5b12f - processing was zipping each folder serially, updated this to zip them all in parallel
• f4a648a - implemented support for cooperative cancellation of the parallel zipping process
• 57b1e1d - implemented abort feature to signal a request for cooperative cancellation of zipping process
  • a note on updating the signature of the presenter's ZipEachSubFolder method to return a bool
    • in the method, i am catching the specific OperationCanceledException and returning true to indicate that processing was canceled
      • this is a pretty ugly way to let the method caller know if cancellation occurred
      • someone trying to learn the purpose of ZipEachSubFolder by its signature would have no idea what the return bool was for and would not only have to read through the method to find any return statements, they'd have to understand most of the method in order to have a context to figure out what the return value was supposed to indicate
    • by the time i got to this point reviewing my own code, i had decided that i was done with the coding phase of this side project and i'd just comment on things that could be improved; but i had to go back and do something about this, so my last commit shows what i did to address this issue
  • in addition to adding the abort feature, i rearranged some of the methods in the WinZipUI class for hopefully improved readability
• be899f7
  • after implementing the abort feature and running the app, i noticed that abort was responsive, but highlight ignored a cancellation request
  • i added a pre-condition to prevent highlighting if cancellation was requested
• c670d5d - just moved a class to another project
• 77c24cf
  • my notes from 3 commits ago complained about my choice to return a bool to indicate cancellation of parallel processing
  • this commit is how i addressed the issue
  • i created a new enum to indicate if cancellation had been not requested, not honored, or honored
  • and i return this new enum instead of the bool
  • by doing this, i enable a reader of this method's signature to immediately know there's something special about the return value and also to more easily understand what the return value is meant to indicate
  • also, at the call site of this method, it's more clear--since before, the only hint was the variable name

• my SubFolderTracker class doesn't have a way to pass in SubFolders
• i started writing a test like this

[Test]
public void ShouldDetermineIsSubFolderProcessed()
{
    // setup
    var parentPath = Support.Fixture.ParentPath;
    var tracker = new SubFolderTracker();
    tracker.FindSubFolders(parentPath);

    // run
    var SUT = tracker;
    var isProcessed = SUT.IsSubFolderProcessed(1);

    // assert
    Assert.That(isProcessed, Is.True);
}

• but this is bad because i'm calling an SUT method that's being tested in another test

  • some future change to FindSubFolders could cause the method meant to test it to fail (good), but it could also cause this test to fail which we would never want since this test is meant to test IsSubFolderProcessed

• in the end i decided that my SubFolderTracker class shouldn't be responsible for finding the sub folders in the first place, it should be given a list of sub folders to track

  • this meant deleting the test for finding sub folders and updating the class

• and then a few more things occurred to me

  • i didn't want to pass in a list of SubFolder objects to the tracker, i'd rather pass in a list of DirectoryInfo objects or paths and have the tracker convert those internally to its list of SubFolder objects
  • i couldn't setup a test for the IsSubFolderProcessed method, because i couldn't setup the class's internal data (SubFolder list)
  • i just hadn't given sufficient thought to this class design yet, and probably was BDUF'ing (Big Design Up Front) trying to do to much designing ahead of time rather than starting with dirty code and then refactoring to something that more naturally made sense

• so i removed the SubFolderTracker class and its test class altogether

• a not on using new-is-glue for zip validator and zipper

  • these are used only in one method in the presenter
  • could still make sense to inject into Presenter
    • could still make sense to create interface to inject into Presenter
  • the parent path validator could have also just been new-is-glued bc it's used only in one place
  • the used-only-in-one place argument isn't strong, i'd normally inject, and create an interface
    • but this project is a side project and i just didn't take the additional steps