Added configuration methods and updated DataAccess with DbProviderFactory.#1
Open
phusband wants to merge 1 commit intoTimCorey:masterfrom
Open
Added configuration methods and updated DataAccess with DbProviderFactory.#1phusband wants to merge 1 commit intoTimCorey:masterfrom
phusband wants to merge 1 commit intoTimCorey:masterfrom
Conversation
Updated DataAccess to be database agnostic.
Owner
|
Thanks for the submission. I'll try to review it soon (sorry, pretty backlogged right now). The one initial thought I have is that I wouldn't put a production connection string in a config. Two issues with that. First, it exposes public credentials to developers. Second, it means that production code runs a bit differently than development code. This change, however minor, means that the application doesn't get tested fully. Instead what I do is replace the connection string on build with the appropriate one depending on location. That way the connection string itself changes but how you call it and what you call stays the same. I'll review this and get back to you with my full thoughts soon. Thanks again. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A few quick edits I made to the ProcessorLibrary which may be personal preference more than best-practices, but I think they do highlight an approach that can be useful in some cases.
DbProviderFactories is a nice to have for avoiding concrete classes like SqlConnection/SqlCommand/etc. hard-coded into the project. As long as the providers are configured somewhere in the chain of machine.config -> app.config/web.config, adding multiple connectionstrings with varying providers can require zero code changes as long as the System.Data.Common pattern has been implemented correctly (which in some cases requires some massaging behind the scenes, but the major providers are covered).
Also with the connectionKey as an appSetting, the idea would be to have multiple named connections that can be swapped out as desired:
So any code keyed to use 'DbConnectionToUse' will use whichever connection the setting is pointed to.