Hot answers tagged

14

SqlConnection implements the IDisposable interface, and so does the SqlDataAdapter class - any type that implements IDisposable should have its Dispose method called; by disposing the connection you don't need to explicitly close it. The best way to ensure Dispose gets called is to wrap the object in a using block, which is basically compiler magic for a ...


13

There are two problems with this that are immediately apparent: You are executing arbitrary SQL. This is a SQL Injection flaw. You'll want to use parameterized queries. You are catching an exception and doing nothing with it. Malformed SQL, integrity constraint violations, or database connectivity issues will be swallowed, causing failures in your ...


8

First of all, kudos for using a Parameter and not concatenating the value into your T-SQL string. You're not disposing all IDisposable objects. SqlDataReader should be disposed as well. Now this makes it quite a bunch of nested using scopes, which you could rework like this: using (var connection = new SqlConnection(Settings.ConnectionString)) ...


8

Nesting. You can reduce nesting by stacking the using blocks, like this: using (var connection = new SqlConnection(connectionString)) using (var command = new SqlCommand(cmdText, connection)) { try { command.CommandType = cmdType; command.Parameters.AddRange(commandParameters); connection.Open();...


7

In terms of the raw/basic functionality, what you have is fine. The transactional logic is good. Readability is the only concern I have, and would rewrite your code as (note, there are some spaces I added around some = conditions): BEGIN TRANSACTION UPDATE SalaryTrans SET carried_forward_amount = @carriedForwardAmount, net_wage = @netWage,...


7

As this method is quite large, let's start digging through the code. insertStatement should be a const on class level The creation of the SqlParameters can be extracted to a method private SqlParameter[] GetInsertEarningsParameters() { //I use a List<T> because it is easier to read. List<SqlParameter> sqlParameters = new List<...


7

The only thing I'd say about your repository, is that FindAll shouldn't return an IList<T>, but an IEnumerable<T>. And you're probably missing a method like GetById that returns only a single record - right now the only way to fetch a single student is... to fetch them all and then filter in memory. The database backend is better at this, you ...


6

As @chrfin mentioned, this type of question is best suited for CodeReview. This is the wrong way to use connections. Keeping a connection open indefinetely is bad because it accumulates locks, transactions remain open,it wastes server resources due to blocking and forces you to use more connections than you actually need. ADO.NET (and ADO before it, way ...


6

You don't want to pull the entire Employee table unless necessary. public IEnumerable<Employee> GetNewEmployees() { string selectStatement = "SELECT Employee.Emp_ID, Employee.Initials + ' ' + Employee.Surname AS Name,..."; using (SqlConnection sqlConnection = new SqlConnection(db.GetConnectionString)) { using (SqlCommand ...


6

I suggest using a light-weight ORM called Dapper. Hides all the dirty work by providing extension methods over ADO.NET. Take a look at the examples here: http://dapper-tutorial.net/execute


5

Aside from Mat's comments there are a few minor things: Parameters are lowerCamelCase so ConnectionString becomes connectionString. In your SQL query you use studentID and in the reader you use StudentID. Local variables are lowerCamelCase so Students becomes students. Aside from that there is not much code to review.


5

Here is your code refactored - since all animals have id, name and age, you can remove duplication (reading and setting these properties for each type of animal): if (reader.HasRows) { while (reader.Read()) { string animalType = reader.GetString(3); Animal animal = CreateAnimal(animalType); animal.AnimalID = (int)reader["...


5

So I can reuse the InvokeSql and do not have to bother about the disposing. Sure. Except InvokeSql is private, so reuse is rather limited to that class. And if that class has a GetUsers method, I dare expect only User-related stuff in that class... which InvokeSql isn't. In all likelihood, your model is going to involve more than just User entities; I'd ...


5

If you do it this way, how will you do an audit trail? Of course I'm assuming you want that. It's just that accounting type problems tend to want that. Have a think about a future statement from your system. Typically, you'd have a monthly statement with a "Carried Forward" at the bottom of each month. If you delete that, you won't be able to find it. ...


5

I can suggest a number of improvements you can make to the code: You're calling GetSchemaTable to get column names. To simply fetch column names you don't need, there is a faster alternative: var columns = Enumerable.Range(0, reader.FieldCount).Select(reader.GetName).ToList(); You're not computing the result of your select query to get column names. ...


4

<clippy>It appears you are writing an Object/Relational Mapper! Would you like some help?</clippy> Yes, a lot of reflection in loops can be bad thing since reflection is known for its slowness. In a situation like this, it's best to cache the results as your types will not be changing during the lifetime of the program. Simple fix: private ...


4

Everything looks good over here, however, I would like to mention some points which cover some of your questions. I can see your methods are dependent on the global variable connectionString. Global variables creates confusion when code grows. It harder to maintain. The code which uses global variables gets tightly coupled. I suggest you to create a ...


4

Having written this type of class before, I would suggest an additional overload with the following signature: public static async Task<IEnumerable<T>> ExecuteReaderAsync( string connectionString, CommandType cmdType, string cmdText, Func<IDataReader, T> transform), params SqlParameter[] commandParameters) With this ...


4

Your Question System.Data.DataTable lives under the System.Data namespace; one good way to start mixing concerns in your BLL is to reference System.Data, and then you don't even need a DAL any more and can have your BLL directly access the database. Or worse, have some of your data access code code in the DAL, and then some more data access code in the BLL. ...


4

using (cnn = NewConnection()) {} This is a very dangerous design. Especially the private TConnection cnn field that is shared by each method. If you ever use it in paralell then those methods will overwrite each other connections. You should use them locally only: using (var cnn = NewConnection()) {} using (cmd) This is also a no go. I'd be really ...


4

(See also comments in code) GetRefNo Create case-insensitive dictionaries right away as you ignore the case later anyway: public SubmissionHeaderDTO GetRefNo() { // tell the dictionaries how to compare the keys var inPrms = new Dictionary<string, object>(StringComparer.InvariantCultureIgnoreCase) { // ... }; var outPrms =...


4

Disclaimer: my first post here and I'm not C# programmer If the input is different, the function will of course call itself again (that's why it is a recursive one, right?) This is dangerous, as user can keep entering wrong inputs, which will increment scoping stack (eventually resulting in StackOverflowException being thrown). Even doubly so, your ...


3

You're making life harder for yourself by not allowing the delegate to have a return type. You could create an overload that accepted a Func instead. private T InvokeSql<T>(string sql, Func<SqlCommand, T> f) { // same as before but return f(command); } Now you can simplify: public IEnumerable<User> GetUsers() { string sql = "...


3

There is no need to have a Action<SqlConnection, SqlCommand> parameter if all you need is a SqlCommand object. Selecting all columns for just using the Name column of the returned reader is slowing down the whole stuff. You should always only select the columns you need. You are using C# 6 so you should take advantage of the nameof expression like ...


3

Firstly and unfortunately, there isn't an immediate plan to replace the entity specific ADO.NET with an ORM. That's fine. This is more questionable: private int GetSqlOutputParam() { return 1; } It would be much clearer to declare this as a descriptively named constant near the top of the class (being class-level scoped): private const int ...


3

I think your question would be better if you fleshed it up with your actual working code, there's a lot of "placeholder" code in your post, which makes it border the line of off-topicness. You may be able to find more information and high-level design guidance about MVP on Programmers.SE. There isn't a lot to review here, but this is jumping at me:...


3

I don't find having to repeat the connection string with each command simpler. Why not make SqlHelper a non-static class, pass the connection string into a constructor and then use it from your methods? Similarly, I believe most commands are going to use CommandType.Text, so that should be the default. You can't use optional parameters together with params, ...


3

you could collapse this piece of code into a single line public IDomainTableRepository DomainTables { get { if (_domainTables == null) { _domainTables = new DomainTableRepository(_context); } return _domainTables; } } by using a Null Coalescing operator (??) ...


3

Introduction: no one reviewed your code then let me do it, even if after few days. The problem is that you're in the middle of the learning process then many things need to be changed/updated. I won't rewrite your code (it won't help you to learn) but I will try to go through it leaving the fixes as exercise for the reader. Let's start from your domain ...


Only top voted, non community-wiki answers of a minimum length are eligible