41

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

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, ...


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 ...


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

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 ...


5

As pointed out by @iSR5's comment, A DataSet is basically a wrapper around a collection or DataTable objects. It has some other helpful utility methods, but from a performance point of view the only performance issue with your logic is that in the DataSet solution you need to access the table to read from through the indexer that has assumed that there is a ...


5

Review Welcome to Code Review. There are few suggestions as below. Layout and formatting Not sure why many newlines between the curly brackets and the method definition, if statements and for statements. For improving readability, those unnecessary newlines can be removed. Magic numbers and List<DataTable> I have no idea about why the initial value of ...


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

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

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

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

You can lose some of the reflection badness in Gaui's answer with a little bit of refactoring and a little bit of caching as such: public static class Helper { private static readonly IDictionary<Type, ICollection<PropertyInfo>> _Properties = new Dictionary<Type, ICollection<PropertyInfo>>(); /// <summary> /...


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

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

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

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

Scope If all you need is to calculate some values from a set of data tables, it would suffice to use LINQ. However, if you prefer to provide an API with common functionality concerning orders and order details, you should really create your own domain classes and map the data tables to your domain classes. For the sake of this question, I would opt for ...


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

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, ...


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