0
\$\begingroup\$

I have this piece of code I developed but it doesn't look efficient to me, can anyone improve it please.

 private void AddToDataBase(SPItemEventProperties properties)
    {
        DataAccess DA = new DataAccess(ConnectionString());

        DetailsRow result = DA.GetDetails(properties.WebUrl);

        if (result != null && string.IsNullOrEmpty(result.URL))
        {
            DA.DeleteDetails(properties.WebUrl);
        }

        if (bool.Parse(properties.ListItem.Properties["Regular Customer"].ToString()) == true)
        {
            DA.InsertDetails(properties.Name, properties.WebUrl, DateTime.Now);
        }
        else
        {
            if (NumberOfInvoices(properties) > 0)
            {
                DA.InsertDetails(properties.Name, properties.WebUrl, DateTime.Now);
            }

            else
            {
                DeleteInvoiceDocument(properties);
            }
        }
    }
\$\endgroup\$

closed as off-topic by Toby Speight, yuri, Graipher, Stephen Rauch, Malachi Sep 11 '18 at 17:01

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – yuri, Graipher, Malachi
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ Hello TimeToThine. How do you suppose anyone can answer that without knowing what your conditions are? Your question is at risk of being closed as "not a real question" if you don't include this critical information. By the way, your assumption is probably wrong - even if the code is different, it can have a big influence on the possible refactorings, so you should show that too. \$\endgroup\$ – Adam Sep 6 '12 at 9:08
  • \$\begingroup\$ basically my conditions are just checking few results returned by a DataTable and it checks if they are null or not + few other validations, I want this code to be reviewed based on structure of it as I got 1 empty "else" statement in there which doesn't look great to me to be honest :) \$\endgroup\$ – Muhammad Raja Sep 6 '12 at 9:13
  • 2
    \$\begingroup\$ This question is incomplete. To help reviewers give you better answers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. The current title states your concerns about the code; it needs an edit to simply state the task; see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Toby Speight Sep 11 '18 at 14:36
12
\$\begingroup\$

Since I don't have the SharePoint libraries I wasn't able to test my solution, but it should work.

  1. Use proper indentation and formatting.
  2. == true is redundant and can be removed.
  3. Instead of the following:

    bool.Parse(properties.ListItem.Properties["Regular Customer"].ToString())
    

    just convert to bool (casting won't work because of SharePoint):

    Convert.ToBoolean(properties.ListItem.Properties["Regular Customer"])
    
  4. Reduce nesting by using else if instead of else { if (...) }

  5. Use || instead of two seperate conditions with the same associated code to avoid redundancy.

Result of refactoring

private void AddToDataBase(SPItemEventProperties properties)
{
    DataAccess dataAccess = new DataAccess(ConnectionString());
    DetailsRow result = dataAccess.GetDetails(properties.WebUrl);
    bool isRegular = Convert.ToBoolean(properties.ListItem.Properties["Regular Customer"])

    if (result != null && string.IsNullOrEmpty(result.URL))
    {
        dataAccess.DeleteDetails(properties.WebUrl);
    }

    if (isRegular || NumberOfInvoices(properties) > 0)
    {
        dataAccess.InsertDetails(properties.Name, properties.WebUrl, DateTime.Now);
    }
    else
    {
        DeleteInvoiceDocument(properties);
    }
}
\$\endgroup\$
  • \$\begingroup\$ my code is ignoring rest of code after this line "bool isRegularCustomer = (bool)properties.ListItem.Properties["Regular Customer"];" tried debugging but when it comes to this line it simply refreshes SharePoint page and there's nothing left to process \$\endgroup\$ – Muhammad Raja Sep 6 '12 at 11:13
  • \$\begingroup\$ the cast to bool seems to be throwing an exception. try using the old version isRegularCustomer = bool.Parse(...ToString()) instead. \$\endgroup\$ – Adam Sep 6 '12 at 11:14
  • \$\begingroup\$ its something related to sharepoint I guess, I tried it so many times before as you suggested but never worked, so used to prase it \$\endgroup\$ – Muhammad Raja Sep 6 '12 at 11:19
  • \$\begingroup\$ hm... according to this comment on a related StackOverflow post, Convert.ToBoolean should also work. \$\endgroup\$ – Adam Sep 6 '12 at 11:23
  • 1
    \$\begingroup\$ I prefer it because we can leave out the call to ToString(). There are some subtle differences, but the main reason for me is that Convert.ToBoolean will work with any compatible type, not just with string. \$\endgroup\$ – Adam Sep 6 '12 at 11:28

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