4
\$\begingroup\$

I am developing a C# Windows forms application. In that, I am reading data from XML which is coming from a web API and storing that XML data into SQL database. I am able to achieve this. But my only concern is, In Production environment, this application will be in service and there will a lot of data to synchronize from XML to SQL Database. So, can anyone review my code and tell me whether it is an efficient way or not. Any sugggestions are welcome.

Because it's the first time I am working on the API and even in coding, I am a fresher.

I have done all to achieve my requirement. I would like a code review from experts.

public void save_vendor_info()
    {
    SqlConnection con = new SqlConnection(@"server=localhost;Database=TEST;integrated security=true");
    con.Open();
    DataTable dt = new DataTable();
    XmlDocument doc = new XmlDocument();
    doc.Load("https://testdata/api/vendordata");

    try
    {
        XmlNode node = doc.DocumentElement.ChildNodes.Cast<XmlNode>().ToList()[0];
        foreach (XmlNode column in node.ChildNodes)
        {
            dt.Columns.Add(column.Name, typeof(String));
        }

        XmlNode Filas = doc.DocumentElement;
        foreach (XmlNode Fila in Filas.ChildNodes)
        {
            List<string> Valores = Fila.ChildNodes.Cast<XmlNode>().ToList().Select(x => x.InnerText).ToList();

           SqlCommand cmd = new SqlCommand(("IF NOT EXISTS (Select Vendorcode From Vendors where Vendorcode = @Vendorcode) INSERT INTO Vendors VALUES (@Vendorcode, @STCD3, @Name1, @Name2, @Name3, @street1, @street2, @street3, @city1, @city2, @city3, @state1, @state2, @state3, @zip1, @zip2, @zip3, @countrycode, @BussinessUnitId, @VATID, @nationalVATID, @IBAN, @BankAccount, @BankCode)"), con);
            cmd.CommandType = CommandType.Text;
            cmd.Parameters.AddWithValue("@Vendorcode", Valores[0]);
            cmd.Parameters.AddWithValue("@STCD3", Valores[1]);
            cmd.Parameters.AddWithValue("@Name1", Valores[2]);
            cmd.Parameters.AddWithValue("@Name2", Valores[3]);
            cmd.Parameters.AddWithValue("@Name3", Valores[4]);
            cmd.Parameters.AddWithValue("@street1", Valores[5]);
            cmd.Parameters.AddWithValue("@street2", Valores[6]);
            cmd.Parameters.AddWithValue("@street3", Valores[7]);
            cmd.Parameters.AddWithValue("@city1", Valores[8]);
            cmd.Parameters.AddWithValue("@city2", Valores[9]);
            cmd.Parameters.AddWithValue("@city3", Valores[10]);
            cmd.Parameters.AddWithValue("@state1", Valores[11]);
            cmd.Parameters.AddWithValue("@state2", Valores[12]);
            cmd.Parameters.AddWithValue("@state3", Valores[13]);
            cmd.Parameters.AddWithValue("@zip1", Valores[14]);
            cmd.Parameters.AddWithValue("@zip2", Valores[15]);
            cmd.Parameters.AddWithValue("@zip3", Valores[16]);
            cmd.Parameters.AddWithValue("@countrycode", Valores[17]);
            cmd.Parameters.AddWithValue("@BussinessUnitId", Valores[18]);
            cmd.Parameters.AddWithValue("@VATID", Valores[19]);
            cmd.Parameters.AddWithValue("@nationalVATID", Valores[20]);
            cmd.Parameters.AddWithValue("@IBAN", Valores[21]);
            cmd.Parameters.AddWithValue("@BankAccount", Valores[22]);
            cmd.Parameters.AddWithValue("@BankCode", Valores[23]);

            cmd.ExecuteNonQuery();
        }
    }
    catch (Exception ex)
    {

    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Tell us more about the production environment. Is the XML being refreshed regularly? Is concurrent access to the XML file allowed? How is this code used? \$\endgroup\$ – dfhwze Aug 10 at 7:51
  • \$\begingroup\$ Actually, we are calling a web API to read the XML data. We will call this Web API twice a day. \$\endgroup\$ – Sunil Edupuganti Aug 10 at 9:02
  • 1
    \$\begingroup\$ @SunilEdupuganti it helps if you could put these details in the question body. Comments have a habit of disappearing without notice, and not everyone will read them. \$\endgroup\$ – VisualMelon Aug 10 at 9:07
5
\$\begingroup\$

As dfhwze says, you should probably pull in the connection string/information from an external source: the production database hopefully won't be called 'TEST', which means you can't use this code in production, which means you will be adding time, effort, and the opportunity for things to go wrong to deployment. Similarly for the web API URL: it might change overnight (if you don't control it), or you might want to use a mock API (e.g. a file on disk) for testing purposes. These could be parameters, or they could be from some configuration system (which could for example read them from disk).


Expanding on dfhwze's comments about your empty catch block... this is rarely a good sign. save_vendor_info is lacking inline documentation (maybe it has some somewhere else?), but presumably its job is to save the vendor information; at the moment it will fail silently if it fails to do so.

What do you anticipate going wrong? If you expect something to go wrong, you should handle it specifically (and probably log it). For anything you can't anticipate, you need to decide whether this method should throw violently (often a good idea) or should fail in a known and consistent state and report this to the caller somehow. Throwing has two big advantages: it's easy (hard to get wrong), and it forces the caller to acknowledge the fact. Whatever you do, you should document it, so that if a failure occurs the caller can handle it appropriately (e.g. retry in an hour, try some other data-source, throw violently itself...)


Your code is a bit of jumble of XML and SQL operations. I would consider trying to pull all the XML out into another method so that this method can focus on the SQL. This method could be as simple as something that returns an IEnumerable<List<string>>, but if that is the case, you need to acknowledge and document the fact that this list of strings has a well-defined order. There would be merit in taking the data and stuffing it into a statically typed DTO, but the added code may not be worth it if this method can be hidden well enough.


I don't think you need the first ToList here: Fila.ChildNodes.Cast<XmlNode>().ToList().Select(x => x.InnerText).ToList();. Generally, making things simple and lazy when you are just going to enumerate them immediately is a good idea, because it doesn't frighten the maintainer ("why is this being cached?") and can improve performance and memory characteristics.


You may see some performance improvement by re-using the same SqlCommand and only changing the parameter values each run of the loop.


What is the use of dt? Looks like you were previously going to load the data into a table but gave up on that plan: remove old code.


Are the parameter names defined somewhere else? If not, I would consider normalising them or finding some way to define each parameter name only once, since currently they are mix of many case conventions, and thought hopefully they won't change much, this will make it more tedious to review the code. I would consider putting them in a list, and using the list to build the query and population the parameters: it's a little dodgy building queries, but it will reduce the repetition of the name, and provide a canonical reference for the index -> name mapping which is trivially modified if the API does change (means that the code can be more easily re-used for other APIs as well).

I would probably want the code to look at the Schema, and in real-time verify that it matches what I'm expecting, because simply indexing into a list of XML nodes is going to go wrong if something is added to the API in an unhelpful position. Honestly, I'm not sure I'd trust any XML source to be consistent in the order of nodes, and would instead explicitly extract all values by name. This would add overhead, but it would make the code robust to non-silly API changes (e.g. it would keep working if a new field was added, and it would fail loudly if an old field was removed or renamed (which is a good thing)).


You should use a consistent naming convention for local variables: I would expect Valores and Filas to be camelCase like everything else.

\$\endgroup\$
5
\$\begingroup\$

Review

Single responsibility principle
Method save_vendor_info (which we'll rename later) does data retrieval from an external source, xml to db mapping, and SQL command execution on a database. I would expect a method with the name Save* to only store data, in this case in the database. I would also suggest to split up loading the input data and saving to the database in separate methods. A mediator method could then call both methods and ensures the data is mapped correctly.


Resource management
You don't dispose of resources that you create. SqlConnection and SqlCommand implement the IDisposable interface. You should make sure to dispose these instances (SQL Disposable Pattern Example). You already open the connection before retrieving the web data. This is too soon. Keep the scope of a connection lifetime to a minimum.


Exception Handling
Definining an empty catch block is really bad practice. You don't know whether something went wrong. At least, log exceptions. Think about which specific exceptions you want to handle and which ones need to be propagated up the stack. Even in sand-box mode, where you don't want exceptions to reach the caller, you should return a boolean to the caller whether the call succeeded.


Naming & Style Conventions

  • Try to avoid underscores in method names and use PascalCase. Prefer SaveVenderInfo over save_vendor_info. As indicated earlier on, I would use this method name only for the part where you save the data to the database.
  • Use var when the type is inferred and don't use abbreviations for variable names: Prefer var dataTable = new DataTable(); over DataTable dt = new DataTable();.
  • Read SQL connection string information from a config file rather than hardcoding it. This gives better maintainability for different environments.
  • Be consistent in placement of curly braces. Either append an opening curly brace after the method with a white space between, or place the opening curly brace on the next line at the same indentation of the member declaration. Don't indent a curly brace on the next line, this leads to deeply indented code.
\$\endgroup\$
  • 1
    \$\begingroup\$ Drat, was just doing a final proof-read! \$\endgroup\$ – VisualMelon Aug 10 at 9:21
  • \$\begingroup\$ Feel free to add your answer as well. \$\endgroup\$ – dfhwze Aug 10 at 9:21
  • 1
    \$\begingroup\$ Wasn't too much overlap in the end, but I left my paragraph about how much I love exceptions in... \$\endgroup\$ – VisualMelon Aug 10 at 9:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.