38

I created an extension method for DataTable to convert them into a List<T> public static class Helper { /// <summary> /// Converts a DataTable to a list with generic objects /// </summary> /// <typeparam name="T">Generic object</typeparam> /// <param name="table">DataTable</param> /// <...


10

A few points tickle: Why is it a static method? None of the comments are helpful. Good comments say why, not what. I'd remove them all. Bracing is inconsistent. Consider: if (!(dr[propertyInfo.Name] is DBNull)) propertyInfo.SetValue(ob, dr[propertyInfo.Name]); And then: if (!(dr[primaryKey] is DBNull)) { propertyInfo.SetValue(ob, dr[primaryKey]....


8

There are a few things I notice right off the bat. Your variable names while not bad, could be better. For instance props -> properties. Stuff like this makes the code easier to read. You have the properties, why not use a foreach loop to fill the datatable (you did it in ToDataSet) the _ prefex should be used for class variables, not local variables. try ...


8

This: + "where s.Version = ms.CurrentVersion and scr.TargetType = 1 " + conditional + "order by svce.PeripheralNumber, ms.EnterpriseName, p.EnterpriseName"; Is poison. conditional came from a string concatenation that involves a parameter string that the method cannot trust: conditional = "and svce.PeripheralNumber= " + parameter.ToString() + " "; ..Uh, ...


7

If you're going to put a try catch in an else, you should probably wrap it in braces: else { try { ... } catch { ... } } You don't need to check for case "1" to case "9" and do the same thing in every single one. C# has fallthrough: case "1": case "2": case "3": //handle cases 1 to 3 _TextBox_Output.Text += NumVal; ...


7

If you use this method quit often by passing an Integer you will get performance problems because of the boxing and unboxing of the struct. My advice would be to write two methods one using an Integer and the other using a String as argument. Based on the naming guidelines you should name methods by using a verb or a verb phrase. DTableCellValue looks by ...


7

I wouldn't say, your major concern should be performance. I find the method rather error prone and it has a very limited use: 1) All columns have to be of type string - except the first which must be of type Guid 2) The order of the columns in the table must match the order of the properties of the object type. If they don't, a column could easily be ...


6

DataTables are quite powefull and offer lots of the real database functionality. Also as far as joins are concerned a few things are possible and I'm of the opinion that if someone uses DataTables he also should use the functionality they offer ;-) It this case using DataTable joins your example could look like this: DataSet ds = new DataSet(); DataTable ...


5

Two code-style issues are immediately obvious: Use braces for 1-liners only split lines where it overflows Code like: var item = result .SingleOrDefault(e => e == "Order"); if (item != null) baseTypes.Add(item); item = result .SingleOrDefault(e => e == "Motion"); Should be var item = result.SingleOrDefault(e => e == "Order"); if (item ...


5

First problem you have is that you have mixed your code logic with UI. But tackling that issue could be too much at this point. I would recommend some reading about MVC/MVP patterns and you can start with What are MVP and MVC and what is the difference? Another visible issue is repetition in your event handlers Button_Numeral_Click and Button_Hex_Click I ...


5

An optimization would be ... Do the object property to database column matching up front, and only once. Use that matching, to generate a list of functions which only essentially do the check for DBNull and the appropriate propertyInfo.SetValue. For each row retrieved, create an instance of the object and invoke all the generated functions, passing it ...


4

Seems like a reasonable approach. It's similar to one that I've used when building a query completely from an XML file, except I used parameters rather than adding in the string. selectString = string.Format("{0} WHERE {1}='{2}' {3};", selectString, dateColumn, dateSelected.ToString("yyyy-MM-dd"), ...


4

The only part that executes slowly would be the for and the nested for. Is there possibly a better way that I can code this? I take you ask us to optimize the nested loops: The most obvious problem, as @Antonio also noted, is the O(n^2) filtering in the nested fors. Also as he noted, you need some kind of data structure with O(1) Contains method. However ...


4

A couple of small points. Single letter identifiers and variable names are the devil incarnate. Replace r with something meaningful. There's not much point in using a variable for col.DataType.ToString. You only use the variable dataType once, so it would be perfectly acceptable to get rid of a line of a code and do this instead. Select Case col.DataType....


4

First - if you are working with DataTables, then you can use Linq to DataSet, which has nice extension methods to get field values. Now let's review logic of your code. You are not using any data from tblone and tbltwo in your result. Actually you only finding joined categories between these two tables, and for each matched rows pair you are adding all m ...


4

Like janos already stated it is ok to return out of an using block. Naming Based on the naming guidelines, method names should be named using PascalCase casing. So getData2 should become GetData2 which should be still renamed to a more meaningful name. Also input parameters should be named using camelCase casing, so connection_string_id should become ...


4

In addition to Heslacher's review, I'd also move the initial if-check to a separate function: private bool IsRowInvalid(DataRow row) { return row["total_daysclose"] == System.DBNull.Value || row["closed_in0to3_count"] == System.DBNull.Value ||row["closed_in4to7_count"] == System.DBNull.Value || row["closedGtr7_count"] =...


4

Useful property names: I find using names as theData bad practice. It doesn't give any info on the instance. Give it a useful name you, and others, easily understand. Casing of property names: Don't use PascalCase for local fields, use camelCase instead. Your MyModel will become myModel. You had already done this correctly for dataSetRow. Redundant ...


4

I'm a little late to the party so I will not repeat what @Heslacher said in his/her review. A method that doesn't use any members from a class instance should be static (shared). If the method reside in a module (not a class) then you're fine because the method is static (shared) by default. Now, since it looks like you're creating an extension method, we'...


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

I have two suggestions: Trim ColValue once rather than every iteration through the loop. Use a case-insensitive string compare rather than performing .ToLower() on two different strings on every loop iteration. Result: private string FindInT(DataTable dt, string ColName, string ColValue, string returnCol) { ColValue = ColValue.Trim(); foreach (...


3

Creation of the additional columns You should change the way you are adding the additional columns. By setting the DefaultValue property you simplify the for loop. dt.Columns.Add("Averagedays", typeof(string)); dt.Columns.Add("0to3", typeof(string)); dt.Columns.Add("4to7", typeof(string)); dt.Columns.Add("Gtrthan7", typeof(string)); should be ...


3

Yes, it's ok to return from the middle of a using block. See this and this related discussions. No need to release the DataSet, but you should wrap the SqlDataAdapter in using, something like this: using (SqlConnection con = new SqlConnection(connstring)) { con.Open(); using (SqlDataAdapter adapter = new SqlDataAdapter(sqlcmd, con)) { ...


3

There are a few optimizations that can be done in your code (such as using a break in the inner loop after a match is found - but reverting the order of the 2 loops) - but leaving the complexity order to O(n^2). A better approach instead is using a dictionary. Since you have a list of results, identified by a unique ID (Request ID), and a list of rows, ...


3

I assume it's your style to refer to class members using "this.". That's not the usual way to do things but it is acceptable. It also seems to be your style to capitalize the names of the UI elements. Again, not the usual style but not horrible (it does confuse the syntax coloration here though). You really need to separate the UI and the business logic. The ...


3

I have managed to run some tests (I was very curious about LINQ on objects vs. SQL processing performance): 1) Setup -- drop table t1 create table t1 (id INT PRIMARY KEY CLUSTERED, name NVARCHAR(1000)) go -- drop table t2 create table t2 (id INT PRIMARY KEY CLUSTERED, name NVARCHAR(1000)) go -- drop table t3 create table t3 (id INT PRIMARY KEY CLUSTERED, ...


3

Move code to methods. By the time you hit for (int j = counter; j < newTable.Rows.Count;) you're nearly ten levels deep. Not only are you losing valuable screen estate, it also doesn't help the clarity of your code. You could also use continue; to reduce indentation: if (!reader.HasRows) { continue; } And: if (!Convert.ToString(reader[i]).Contains("...


3

One simple trick here would be to take the column processing outside of the row processing (the column metadata should not change while the rows probably will): using (var metadaDataTable = GetMetaDataTable()) { var columns = metadaDataTable.Columns .Cast<DataColumn>() .Where(col => col.ColumnName != "path_variable") ....


3

params (version 1) You can use the same trick the Rows.Add method uses namely the params keyword By using the params keyword, you can specify a method parameter that takes a variable number of arguments. and pass columns names as the last parameters. Then you read the values form the reader with some LINQ and turn it into an array because it's just that,...


3

I'm going to try to avoid raging about the data table based data access layer because I doubt a random person on the internet is going to change the direction of your project. So, let's look at your SQL query: SELECT Date, Shift FROM ScheduleList WHERE ID = @ID AND MONTH(Date) = @Month AND YEAR(Date) = @Year I've removed the extra ...


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