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

Paulvancoller techtest submission #13

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Paulvancoller techtest submission #13

wants to merge 11 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 28, 2020

This PR submits Paul van Coller's (Cyberpro Consulting) Tech Test results.

It refactors the code into some better practices and implements Customer listing (with optional orders), as well as customer linking to orders.

Refactoring updates:

  • Object reorganization for clarity.
  • Hardcoded settings moved to settings file (connection strings and VAT settings).
  • Parameterized all queries for sanitation.
  • Class comments to simplify intention.

Issues / refactoring not done:

Description Reason
Inversion of Control Due to this being a class library, dependency injection would not be set up here, but rather on the parent project utilizing this library. One might want to use interfaces on sub-objects to allow these dependencies to be handed to the OrderService, but the static class on the CustomerRepository does not allow for this.

Additionally, since the Order Repository is marked as internal, one might guess that the original intention was for only the AnyCompany assembly to know about these objects, rendering Inversion of Control on these objects irrelevant.
Unit tests Unit tests are difficult to create here due to the lack of Inversion of Control, as discussed above. One would preferably mock any connection or repository objects in order to test only the specific unit of code.
Integration tests Despite the absence of unit tests, one could still implement integration tests, either via Postman (using a web API parent project and an on-the-spot database instance), or CLI-Unit (using a CLI parent project and an on-the-spot database instance).
Country Code Enum A choice was made not to change the internal country code properties to an Enum, but rather leave it as a string. This is due to the fact that the data will be passed from an external project, and that multiple databases are used.
Key Guids Since customers and orders live in different databases, it would be recommended that their primary keys be changed from ints to GUIDs, in order to ensure uniqueness of the keys across multiple databases or servers.

@paulvancoller
Copy link

@seanogt, please review when you get a chance?

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.

1 participant