Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring c# projects #544

Merged
merged 11 commits into from
Jun 6, 2024
Merged

Conversation

StefH
Copy link
Contributor

@StefH StefH commented May 31, 2024

  • I acknowledge that this PR is not a solution to the Gilded Rose Kata, but an improvement to the template.

Please provide your PR description below this line

  • Use file-scoped namespaces everywhere
  • Changed csproj files : added RootNamespace
  • Removed NUnit/GildedRoseTests/TextTestFixture.cs because the same logic is defined in csharp.NUnit/GildedRose/Program.cs
  • Program.cs is the same for both (⭐: actually I think it's even better to only have 1 main GildedRose project which is defined in the NUnit project and referenced in the other xUnit project)
  • small changes from GildedRose(IList<Item> Items) to GildedRose(IList<Item> items) and IList<Item> Items; to private readonly IList<Item> _items; to be more conformant with C# .NET coding guidelines.

Copy link
Collaborator

@codecop codecop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some in line comments. Also please keep formatting changes minimal if possible (unless missing formatting). Thank you

csharp.NUnit/GildedRose/Program.cs Outdated Show resolved Hide resolved
csharp.xUnit/GildedRose/GildedRose.cs Outdated Show resolved Hide resolved
csharp.NUnit/GildedRose/Program.cs Outdated Show resolved Hide resolved
csharp.xUnit/GildedRose/Program.cs Outdated Show resolved Hide resolved
@StefH
Copy link
Contributor Author

StefH commented Jun 2, 2024

@codecop
Please check the files again if it's ok.

@codecop
Copy link
Collaborator

codecop commented Jun 2, 2024

Thank you. Now I see, the original code was already inconsistent. That is a pity. So as background for you, the code should look the same as much as possible for both projects. From what I see here I think project files, GildedRose and Item are identical. Both Program are almost identical besides the missing dynamic days in the second one. While it has not been there if you can please add it to improve the consistency between the versions. Also both Approvaltests should look as similar as possible. Because you have the project open, maybe you can run a diff between both folders to verify my assumption. If all is good, then please squash your changes to one commit. We are nearly there ;-) Thank you for your patience.

@StefH
Copy link
Contributor Author

StefH commented Jun 2, 2024

@codecop

  • I've fixed README.md file
  • I've added a README.md file
  • removed obsolete global.json file
  • I've fixed the dynamic days and the two main projects are now identical. So only 1 main GildedRose project is needed. I can make that change in this PR also if you approve.
  • Both Approvaltests look as similar as possible
  • I did a diff between the 2 folders
  • squash your changes to one commit --> Normally I don't do this, I do this when I merge the PR to master, in that case I use Squash commit, this makes the Git Timeline even cleaner.

My project
image

This project
image

Removed global.json
@emilybache
Copy link
Owner

Hi, thanks to both of you for doing this, I'm happy to see these changes. I think I will ask @codecop to take another look and do the merge as he sees fit.

@codecop codecop merged commit 99f2fcb into emilybache:main Jun 6, 2024
@StefH StefH deleted the stef-refactor-csharp-projects branch June 6, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants