2
\$\begingroup\$

I think here is something wrong with code. I use a class with methods to get tenants from DB:

    public List<CrmTenant> GetAllTenants()
    {
        List<CrmTenant> tenantsList = new List<CrmTenant>();

        try
        {
            var crmTenants =
                 from tenant in context.CrmTenant
                 select tenant;

            if (crmTenants != null)
            {
                foreach (var tenant in crmTenants)
                {
                    tenantsList.Add(tenant);
                }
            }

            return tenantsList;
        }
        catch (Exception ex)
        {
            this.logger.Write("Failed to get all tenants from database.", "Exceptions", TraceEventType.Error, ex);
            return null;
        }
    }

    /// <summary>
    /// Gets Tenant by name.
    /// </summary>
    /// <returns></returns>
    public CrmTenant GetTenantByName(string tenantName)
    {
        CrmTenant crmTenant = null;

        try
        {
            crmTenant =
                 (from customerCrm in context.CrmTenant
                  where customerCrm.TenantName == tenantName
                  select customerCrm).Single();
            return crmTenant;
        }
        catch (Exception ex)
        {
            this.logger.Write(String.Format("Failed to get tenant '{0}' from database.", tenantName), "Exceptions", TraceEventType.Error, ex);
            return null;
        }
    }

    /// <summary>
    /// Gets tenant by Username.
    /// </summary>
    /// <returns></returns>
    public CrmTenant GetTenantByUserName(string userName)
    {
        CrmTenant crmTenant = null;

        try
        {
            crmTenant =
                 (from customerCrm in context.CrmTenant
                  where customerCrm.Username == userName
                  select customerCrm).Single();
            return crmTenant;
        }
        catch (Exception ex)
        {
            this.logger.Write(String.Format("Failed to get tenant by username '{0}' from database.", userName), "Exceptions", TraceEventType.Error, ex);
            return null;
        }
    }

Here is call for methods

// get tenant
var crmTenant = CrmServiceHandlerFactory.GetInstance().Resolve<TenantManager>().GetTenantByUserName(userName);

I am new in programming, could you please help me to improve this code

\$\endgroup\$
  • \$\begingroup\$ Why do you think there is something wrong with your code? \$\endgroup\$ – svick Jul 25 '12 at 9:11
  • \$\begingroup\$ Because I have ten methods like GetTenantByName GetTenantByUserName GetTenantBySize GetTenantByIce and so on... I doubt that it's a good style... Or i'm wrong? \$\endgroup\$ – Igor Jul 27 '12 at 10:22
3
\$\begingroup\$

I can see some room for improvement:

  1. Most importantly, you shouldn't hide exceptions like this. You should most likely let the exceptions bubble up and let the called decide what to do when an exception happens. And if you return null, it means you will either have to add null checks everywhere, or you will get NullReferenceException instead of the exception that actually caused the problem, which makes it harder to debug.
  2. You don't need to add items into the list one by one, you can do it all at once using AddRange(). Or even better, just use ToList():

    return crmTenants.ToList();
    
  3. This one is really minor, but the query from tenant in context.CrmTenant select tenant can be simplified to just context.CrmTenant, the LINQ doesn't do anything useful there.

#1 is by far the biggest issue, I think, because it shows an error in your design. #2 and #3 could just simplify your code a bit.

\$\endgroup\$

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.