Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
Community Bot
Community Bot
  • 1

Instead, use constructor chaining:

Instead, use constructor chaining:

thought a bit about OP's request
Source Link
BCdotWEB
BCdotWEB
  • 11.4k
  • 2
  • 28
  • 45

Now, looking at your "problem": it is inevitable that there has to be some place where you need to actually do something specific. In this case there's ReadAsset where you get the ReadAssetRequest, convert that to sqlParams and dbCommandInfo, use these to try to get a dataTable from the databaseHelper, convert that to a ReadAssetResponse.

I'm sure some of that can be moved to a Helper class or a base class, and if you're really hardcore you can:

  • build a mapper that can take any Request class and convert it to a list of SqlParameters etc. via reflection.
  • Perhaps the names of the SqlParameters are the same as the name of the properties, and if they aren't there's an Attribute on top of the property.
  • The name of the stored proc can be in a dictionary somewhere, with the type of the Request as the key.
  • And another mapper can convert the datatable to the Response object.
  • Even that DataTableIsNotPopulated check could be configured somewhere (because now you require a single result, but another response might require multiple results), etc.

Which means you perhaps wouldn't need a ReadAsset class anymore, since all of the actions inside it are actually a bunch of configurations used by helper classes. And thus instead of adding a single Asset class you now need to remember to add various configurations at various places.

Is that an improvement? I've worked with code like that, and while I did admire it, at times I did feel like Alice falling down the rabbit hole, ending up with dozens of code pages open in Visual Studio trying to figure out what needed to be altered where in order to get all of the components to play nice. And three months later I again needed to re-acquaint myself with the system before I could add another configuration.

Now, I can't say I'm a big fan of the code you show us here. The usage of arrays instead of IEnumerable<>, the apparent mixing of UI (request, response) and db (SqlParameter, DataTable),... I'd expect the db-related code to be in a separate layer and that you'd work with business entities. Why not use Entity Framework instead of mapping datatables to your custom classes? And do you really need to catch SqlExceptions?

Now, looking at your "problem": it is inevitable that there has to be some place where you need to actually do something specific. In this case there's ReadAsset where you get the ReadAssetRequest, convert that to sqlParams and dbCommandInfo, use these to try to get a dataTable from the databaseHelper, convert that to a ReadAssetResponse.

I'm sure some of that can be moved to a Helper class or a base class, and if you're really hardcore you can:

  • build a mapper that can take any Request class and convert it to a list of SqlParameters etc. via reflection.
  • Perhaps the names of the SqlParameters are the same as the name of the properties, and if they aren't there's an Attribute on top of the property.
  • The name of the stored proc can be in a dictionary somewhere, with the type of the Request as the key.
  • And another mapper can convert the datatable to the Response object.
  • Even that DataTableIsNotPopulated check could be configured somewhere (because now you require a single result, but another response might require multiple results), etc.

Which means you perhaps wouldn't need a ReadAsset class anymore, since all of the actions inside it are actually a bunch of configurations used by helper classes. And thus instead of adding a single Asset class you now need to remember to add various configurations at various places.

Is that an improvement? I've worked with code like that, and while I did admire it, at times I did feel like Alice falling down the rabbit hole, ending up with dozens of code pages open in Visual Studio trying to figure out what needed to be altered where in order to get all of the components to play nice. And three months later I again needed to re-acquaint myself with the system before I could add another configuration.

Now, I can't say I'm a big fan of the code you show us here. The usage of arrays instead of IEnumerable<>, the apparent mixing of UI (request, response) and db (SqlParameter, DataTable),... I'd expect the db-related code to be in a separate layer and that you'd work with business entities. Why not use Entity Framework instead of mapping datatables to your custom classes? And do you really need to catch SqlExceptions?

Source Link
BCdotWEB
BCdotWEB
  • 11.4k
  • 2
  • 28
  • 45

Don't do this:

    public DatabaseCommandInfo(string storeProcName, SqlParameter[] spParams)
    {
      StoredProcName = storeProcName;
      Parameters = spParams;
      CommandType = CommandType.StoredProcedure;
    }


    public DatabaseCommandInfo(string storeProcName, SqlParameter[] spParams, string[] tableNames)
    {
      StoredProcName = storeProcName;
      Parameters = spParams;
      TableNames = tableNames;
      Option = LoadOption.OverwriteChanges;
      CommandType = CommandType.StoredProcedure;
    }

Instead, use constructor chaining:

public DatabaseCommandInfo(string storeProcName, SqlParameter[] spParams)
    : this(storeProcName, spParams, new string[])
{
}

The same is true for ReadAsset:

public ReadAsset()
   : this(new DatabaseHelper("Data Source=.; Initial Catalog=Assets; integrated security=true;"))
{
}

The code isn't consistent: row[columnName] != DBNull.Value vs DBNull.Value.Equals(row[columnName]).

Why is this checked: row.Table.Columns.Count > 0 ?

In both GetValue and GetNullableValue you repeatedly call row[columnName]. Call it once and store the value in a variable and work with that variable.

In IdsToXml the element name "Ids" is used twice, so ideally it should be a const.

Your list of SqlParameter is missing the SqlDbType. I'd prefer this:

  var sqlParams = new[]
  {
    new SqlParameter("@TypeId", SqlDbType.Int).Value = request.TypeId,