2

I hope this fits more here than SO because it is mostly about code design than making it work. Also, this is not tied to RxJs, but more to the reactive paradigm. (TypeScript tag is only added to make the code highlighted.)

So this is what the code looks like, though this is just an overview. Actual function names are different than in the real world:

// Observable returns true, if the action has been successfully executed
const tryLogin = (id: number): Observable<boolean> => {
    const result = new ReplaySubject<boolean>();

    // Operation 1
    loadUserFromDB(id)
        .subscribe(
            user => {
                // Early stop 1 - true
                if (user.name === 'Sarah' || user.id === 0) {
                    result.next(true);
                    result.complete();
                    return;
                }

                // Operation 2, dependent on Operation 1
                checkMasterData(user)
                    .subscribe(
                        loginInformation => {
                            const {userId, lastLogin} = loginInformation;

                            // Early stop 2 - false
                            if (lastLogin === 'LAST_YEAR') {
                                result.next(false);
                                result.complete();
                                return;
                            }

                            // Operation 3, dependent on Operation 1 and Operation 2 's success
                            createNewUserId({attemptedUserId: userId})
                                .subscribe(
                                    // Operation N...
                                    // createAccountFromLogin function also has a similar structure to `tryLogin`
                                    newId => createAccountFromLogin(newId, result),
                                    error => result.error(error)
                                );
                        },
                        error => {
                            result.error(error);

                            // Sidenote: No idea why complete is called
                            result.complete();
                        }
                    );
            },
            error => result.error(error)
        );

    return result;
};

Problems with the current setup:

  • a result object is passed around into functions, rather than the function to return a true/false Observable
  • the deeply nested code makes it hard to read
  • since there is no single observable chain, the code is incredibly difficult to debug (can not use tap at a single place)
  • Observables are used as an ES5 Promise, but they are much more than that

Observables would be needed to be connected somehow by flatMap, but the chain should be finished up once the login process is terminally decided.

(Caching the single output is not a problem, observable will be simply cached.)

So can this code actually be refactored into something more readable that is more authentic to the libraries it is currently using?

Any suggestion is appreciated.

Full code that also includes function declarations.

  • Can you explain the decision process that is modeled here? As those inner function don't take any parameters, it's difficult to see how the are working together. – Yoshi Mar 14 at 13:29
  • @Yoshi : edited it to pass user as input to the next function. Some of the results of the subscriptions are used in the next function parameters, just like checkLastWorkingLogin(user, result). - The decision process itself is totally different - I am sorry that my made-up example does not make much sense. The code flow is what is important. I didn't dare to post the actual function names from the company's code, hope you understand that. – andras Mar 14 at 13:35
  • andras Thank you for the edit, it's getting clearer. Is it correct to say that: you have a hierarchy of checks, and you want to use them in succession until one gives a definite answer. At which point you don't want to ask any of the other checks. – Yoshi Mar 14 at 13:40
  • @Yoshi Yes, that is exactly what is going on (built with a reactive framework so that obscures a couple aspects). Very well formulated. – andras Mar 14 at 13:45
0

Yes, you should definitely refactor this code to use regular return values. The use of Rx simply doesn't make any sense in this context.

Assuming the whole application is developed in this style, you have quite a task before you. You certainly want to do this one step at a time and make sure you always have a working application.

There are several ways by which you could approach this. The safest, though not necessarily quickest way I can see is the following:

  • Each function gets a wrapper function that does nothing but reduce the Observable<T> returned by the original function into the actual T return value you want to have.

    So you'll have a reducedTryLogin, reducedCheckIfUserIsAuthorizedFromDatabase, etc.

  • You replace the implementation of the reducedXXX functions. Instead of simply reducing the result of XXX, you'll use the other wrapper functions to implement the actual functionality.

Let's take tryLogin as an example. Originally, your reducedTryLogin function will simply be something like

public reducedTryLogin(userId: number): boolean {
    return tryLogin(number).reduce((acc, val) => acc || val, false);
}

But once you have reducedTryCheckIfUserIsAuthorizedFromDatabase and reducedCheckIfUserHasAccess, etc. you can replace it with

public reducedTryLogin(userId: number): boolean {
    var user = reducedTryCheckIfUserIsAuthorizedFromDatabase();
    if (user.Id === userId)
    {
        return true;
    }

    var access = reducedCheckIfUserHasAccess();
    if (access.banned)
    {
        return false;
    }

    // etc.
}

At this point, you can flip things around and make the original tryLogin function simply call reducedTryLogin and push the result through a Subject. Finally, you can remove it altogether when you've updated all callers.

Observables can make the code much clearer when you actually want to apply an operation to a stream of things. Outside of that, they just obscure the code and I expect the readability of your code to improve significantly once you've removed the use of Rx where it isn't appropriate.

  • I am not aware of a reduce function that converts an Observable<T> to a T. Is that even possible? – andras Mar 14 at 13:29

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.