-1
\$\begingroup\$

This class returns a cached contact by ID. The two cached functions here are doing the same thing. The only difference being an early return statement or using else and having only one return statement. CacheHelper class is a static class that is a wrapper around .Net's MemoryCache class. It's a dll reference so I don't have the source code for it.

I personally think the function with else is more readable. However, when I'm reading on the web, it says I need to avoid using else whenever possible.

Also, there is a reason I'm checking if cache exists first instead of directly getting it. In some cases, null Contact objects are added to the cache, so if I do a get directly, the logic won't be right. Get would return an empty object if the cache didn't exist or even if a null object is stored in the cache. I understand that storing null objects does not make much sense. I'm the in process of refactoring it, so for now please focus only on the question about whether or not to use else.

public class ContactHelper
{

    public Contact GetContact(int ID)
    {
        return ContactService.GetContact(ID);
    }

    public Contact GetCachedContact(int ID)
    {

        string Key = ID;
        Contact Con = default(Contact);

        if (CacheHelper.Exists(Key)) {
            Con = (Contact)CacheHelper.Get(Key);
        } else {
            Con = GetContact(ID);
            CacheHelper.Add(Key, Con);
        }

        return Con;
    }

    public Contact GetCachedContact_2(int ID)
    {

        String Key = ID

        if (CacheHelper.Exists(Key)){
            return (Contact)CacheHelper.Get(Key);
        }

        Con = GetContact(ID);
        CacheHelper.Add(Key, Con);

        return Con;
    }

}
\$\endgroup\$

closed as off-topic by Heslacher, forsvarir, Vogel612, t3chb0t, Graipher Oct 11 '16 at 14:14

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

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – Heslacher, t3chb0t, Graipher
  • "Questions must involve real code that you own or maintain. Pseudocode, hypothetical code, or stub code should be replaced by a concrete implementation. Questions seeking an explanation of someone else's code are also off-topic." – forsvarir, Vogel612
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ What's underneath CacheHelper? A dictionary? If so, there are more methods than just ContainsKey that can be useful, say for example, TryGetValue. Also, do make sure you aren't reinventing the wheel by wrapping a dictionary's methods when the dictionary class itself has all the functionality you need. \$\endgroup\$ – moarboilerplate Oct 11 '16 at 3:51
  • \$\begingroup\$ @moarboilerplate CacheHelper is a third party dll. All I know is it uses MemoryCache class of .Net. I don't really know the implementation details of it. It works without any issues though. \$\endgroup\$ – user3587180 Oct 11 '16 at 3:53
  • \$\begingroup\$ @mdfst13 I added more information about CacheHelper class in my question. \$\endgroup\$ – user3587180 Oct 11 '16 at 3:53
  • 3
    \$\begingroup\$ Does not compile for me. Cannot implicitly cast int to string. String Key = ID. On the second you are missing a ; \$\endgroup\$ – paparazzo Oct 11 '16 at 4:10
1
\$\begingroup\$

It's debatable on which is more readable. I've worked in places where the common opinion is that an explicit else is more readable, and I've worked in places that consider the terser the code, the better.

If I had to choose, I'd personally go for the one without the else, because it's less lines of code to maintain, but my general advice is that within the overall picture of developing your app, it probably doesn't matter that much.

If you decide that the way with the else is more readable, I wouldn't initialize Con at all, because it gets overwritten in either case.

\$\endgroup\$

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