-2
\$\begingroup\$

I have this class

export class CurrentLanguageService {
  currentLanguage(loggedUser: User): string {
    if (sessionStorage.getItem(LanguageStorageKey) !== null) {
      if (Object.keys(Languages).includes(sessionStorage.getItem(LanguageStorageKey))) {
        return sessionStorage.getItem(LanguageStorageKey);
      } else {
        return Languages[Languages.nl];
      }
    } else {
      if (loggedUser.language) {
        return Languages[loggedUser.language];
      } else {
        return Languages[Languages.nl];
      }
    }
  }
}

You will see that I have so many conditions, is there a way that some one knows is this is proper coding, thanks

\$\endgroup\$
2
  • 5
    \$\begingroup\$ The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?, as well as How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ Aug 17, 2021 at 16:05
  • 5
    \$\begingroup\$ Does the c# tag really apply, or is it just typescript? Also, it would benefit reviewers to have a bit more information about the code in the description. From the help center page How to ask: "You will get more insightful reviews if you not only provide your code, but also give an explanation of what it does. The more detail, the better." \$\endgroup\$ Aug 17, 2021 at 16:10

2 Answers 2

3
\$\begingroup\$

I don't think the logic is correct. Why return the default language if there is a language corresponding to the language storage key but not contained in Object.keys? I'd think the language of the logged user to be the better alternative.

var language = sessionStorage.getItem(LanguageStorageKey);
if (language !== null && Object.keys(Languages).includes(language)) {
    return language;
} else if (loggedUser.language) {
    return Languages[loggedUser.language];
} else {
    return Languages[Languages.nl];
}

This simplification is possible due to these points:

  1. Combining two conditions into one if-statement with &&. Note that Java Script (and thus TypeScript) uses short-circuit-evaluation. I.e., the second condition is only evaluated if the first is true (in the case of &&).
  2. Testing the conditions in a sequential way instead of using nested if-statements.
  3. Returning the default language (nl) at one place due to a better logic.
  4. Storing the result of the call sessionStorage.getItem(LanguageStorageKey) into a temporary variable instead of performing the call three times. This leads not only to a small performance gain but also makes the code more readable.
\$\endgroup\$
1
\$\begingroup\$

Beautify

First thing that you need to do: apply refactoring Extract variable. This will make code cleaner & faster.

const storedLanguage = sessionStorage.getItem(LanguageStorageKey);
if (storedLanguage !== null) {
    if (Object.keys(Languages).includes(storedLanguage)) {
        return storedLanguage;
    } else {
        return Languages[Languages.nl];
    }
} else {
    …
}

After that let's get rid of if-s by applying Replace Nested Conditional with Guard Clauses

const storedLanguage = sessionStorage.getItem(LanguageStorageKey);
if (storedLanguage !== null && Object.keys(Languages).includes(storedLanguage)) {
    return storedLanguage;
}

if (loggedUser.language) {
    return Languages[loggedUser.language];
}

return Languages[Languages.nl];

And now we can use Nullish Coalescing operator to simplify our code

const storedLanguage = sessionStorage.getItem(LanguageStorageKey);
if (storedLanguage !== null && Object.keys(Languages).includes(storedLanguage)) {
    return storedLanguage;
}

return Languages[loggedUser.language ?? Languages.nl];

Now, I would suggest to leverage in-syntax to make things shorter:

if (storedLanguage !== null && storedLanguage in Languages) {
    …

Next trick can be considered bad in some code styles. Typically, typescript will deny you to perform check like null in smth, because of typing. But he is wrong here. We can use non-null assertion operator

const storedLanguage = sessionStorage.getItem(LanguageStorageKey);
if (storedLanguage! in Languages) {
    return storedLanguage;
}

return Languages[loggedUser.language ?? Languages.nl];

Same level of abstraction

Let's make things more clear. You are checking that storedLanguage is in enum. Let's build utility function for it

function keepIfInEnum<T>(
    value: string,
    enumObject: { [key: string]: T }
) {
    if (Object.values(enumObject).includes((value as unknown) as T)) {
        return (value as unknown) as T;
    } else {
        return undefined;
    }
}

Now we can write:

return keepIfInEnum(sessionStorage.getItem(LanguageStorageKey)) 
    ?? Languages[loggedUser.language ?? Languages.nl];
\$\endgroup\$

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