1
\$\begingroup\$

This is my event handler for a videochat client that takes all the events that the server throws at us and routes them to different functions. All of the functions are imported and both settings and session are global objects. My main priority on this project is readability and style and this big switch statement doesn't achieve that goal.

const eventHandler = (event: backendEvent) => {
    const { name, data } = event;
    if (!session.started) {
        return;
    }
    switch (name) {
        case "rtccall":
        case "rtcpeerdescription":
        case "icecandidate":
            eventHandlerRTC(event);
            break;
        case "gotMessage":
            setTyping(false);
            addMessage(data, "stranger");
            break;
        case "typing":
            setTyping(true);
            break;
        case "stoppedTyping":
            setTyping(false);
            break;
        case "commonLikes":
            addStatus(getLikeString(data));
            break;
        case "connected":
            connected();
            break;
        case "strangerDisconnected":
            userDisconect("Stranger");
            break;
        case "nullRequest":
            if (session.connected) {
                userDisconect("Stranger");
            }
            break;
        case "waiting":
            clearAdd("Waiting");
            break;
        case "identDigests":
            return twiceSkipping(data);
        case "serverMessage":
            addStatus(data);
            break;
        case "error":
            if (data.includes("banned")) {
                settings.autoskip = false;
            }
            console.log(event);
            break;
        default:
            console.log(event);
            break;
    }
};

This function is located at src/ts/network/events.ts:11.

Compiling and running the project is very simple, just run this commands:

git clone https://github.com/pwall2222/OmClient
cd OmClient
npm i --include=dev
npm start

(Windows users might get errors because browserSync needs to compile c++)

\$\endgroup\$
2
  • \$\begingroup\$ One thing to increase readability, would be to extract those strings into an enum something like this: tsplay.dev/N5eXPN - another thing I'd do, is to move the if statements for example if (session.connected) into the userDisconnect function. Also other if statements can be pulled into their respective function. \$\endgroup\$
    – r3dst0rm
    May 23 at 7:29
  • \$\begingroup\$ @r3dst0rm You have basically just written the beginning of a good answer. Perhaps an answer would be better than a comment. \$\endgroup\$
    – pacmaninbw
    May 23 at 12:17

1 Answer 1

1
\$\begingroup\$

As pointed out in the comments, I'm now writing a complete answer.

Extract 'case' strings into an enum

I'd start extracting the event strings into an enum. This reduces the risk for typos and harmonizes the appearance.

enum Events {
  CONNECTED = "connected",
  RTC_CALL = "rtccall",
  RTC_PEER_DESCRIPTION = "rtcpeerdescription",
  // ... and so on
}

Note: If you can change the type of the name attribute of the backendEvent to the enum you could also construct some sort of check (exhaustive check), that all enum values are handled inside the switch case block. An example implementation can be found here: https://tsplay.dev/wjZx7w

Reduce nesting

Furthermore, reducing the nesting inside a switch block, can also help to understand and parse what is happening. Compare the following to listings:

if (session.connected) {
  userDisconnect("Stranger");
}
case Events.NULL_REQUEST:
  userDisconnect("Stranger");
  break;

// --- somewhere else
const userDisconnect = (name: string) => {
  if (!session.connected) {
    return;
  }
  // .. do actual disconnecting
}

Extract methods whenever possible

Another thing I noticed is the switch block for the error handling. There are two options I see to improve readability here:

// Original
if (data.includes("banned")) {
  settings.autoskip = false;
}

// Option 1
settings.autoskip = data.includes("banned");

// Option 2
const handleError = (event: backendEvent) => {
  settings.autoskip = event.data.includes("banned");
  // Notice how I also pulled the console.log statement in here!
  console.log(event);
}

Harmonize method parameters and method naming

So in this section I group various things, to harmonize and adjust method naming, method parameters and so on. My point here is, to point out, that it is important to harmonize how things are called and how things operate. Your naming might differ, but the following is how I'd approach it. So your mileage might vary.

setTyping

The method gets called with either true or false. I assume this method, should indicate whether or not a user is typing, hence the boolean parameter. When using this function somewhere in the code, it is not necessarily important to the caller to know, that there must be a boolean parameter, as it could be hidden very easily.

So I'd consider those methods somewhere: startTyping, stopTyping.

export const startTyping = () => setTyping(true);
export const stopTyping = () => setTyping(false);
Indicate methods are there for handling certain events

When responding to an event it might be necessary to execute various action which are not primarily linked to each other. For example: setTyping(false) and addMessage(...). At first this looks confusing when looking at the case block - to avoid this confusion, it might be helpful to create specific handler methods. Something like: handleGotMessage(event) where everything necessary for this event is called and executed. This can be applied to each other case as well. Leaving you with a bunch of handle* methods.

twiceSkipping

The method twiceSkipping is the only method which is supposed to return something. This seems odd - mainly because the whole code block can exit early, when there is no session connected, without any return value. As well as the whole function signature does not inform me, which kind of data is allowed to be returned. I would try to avoid such things, as it can make it pretty difficult to understand. In terms of this review I adapt the above code to match the same pattern, although I'd like to get rid of it. As it suppose to return a boolean, it would be good to add it to the signature of the function as well as the event handler.

Summary

I've elaborated on various things, you could do and use to improve readability of the above mentioned code. To put things together, I will leave you with the code, I'd come up with.

enum Events {
  RTC_CALL = "rtccall",
  RTC_PEER_DESCRIPTION = "rtcpeerdescription",
  ICE_CANDIDATE = "icecandidate",
  GOT_MESSAGE = "gotmessage",
  TYPING = "typing",
  STOPPED_TYPING = "stoppedTyping",
  COMMON_LIKES = "commonLikes",
  CONNECTED = "connected",
  STRANGER_DISCONNECTED = "strangerDisconnected",
  NULL_REQUEST = "nullRequest",
  WAITING = "waiting",
  IDENT_DIGESTS = "identDigests",
  SERVER_MESSAGE = "serverMessage",
  ERROR = "error"
}

const eventHandler = (event: backendEvent): boolean | void => {
    if (!session.started) {
        return;
    }

    switch (event.name) {
        case Events.RTC_CALL:
        case Events.RTC_PEER_DESCRIPTION:
        case Events.ICE_CANDIDATE:
            // Renamed the original eventHandlerRTC
            handleRTC(event);
            break;
        case Events.GOT_MESSAGE:
            handleGotMessage(event);
            break;
        case Events.TYPING:
            handleTyping();
            break;
        case Events.STOPPED_TYPING:
            handleStoppedTyping();
            break;
        case Events.COMMON_LIKES:
            handleCommonLikes(event);
            break;
        case Events.CONNECTED:
            handleConnected();
            break;
        case Events.STRANGER_DISCONNECTED:
            handleStrangerDisconnected();
            break;
        case Events.NULL_REQUEST:
            handleNullRequest();
            break;
        case Events.WAITING:
            handleWaiting();
            break;
        case Events.IDENT_DIGESTS:
            return handleIdentDigests(event);
        case Events.SERVER_MESSAGE:
            handleServerMessage(event);
            break;
        case Events.ERROR:
            handleError(event);
            break;
        default:
            console.log(event);
            break;
    }
};

const handleIdentDigests = (event: backendEvent): boolean => twiceSkipping(event.data);

const handleGotMessage = (event: backendEvent) => {
  stopTyping();
  addMessage(event.data, "stranger");
}

const handleTyping = () => {
  startTyping();
}

const handleStoppedTyping = () => {
  stopTyping();
}

const handleCommonLikes = (event: backendEvent) => {
  addStatus(getLikeString(event.data));
}

const handleConnected = () => {
  connected();
}

const handleStrangerDisconnected = () => {
  userDisconnect("Stranger");
}

const handleNullRequest = () => {
  // The if statement is inside the userDisconnect function
  userDisconnect("Stranger");
}

const handleWaiting = () => {
  clearAdd("Waiting");
}

const handleServerMessage = (event: backendEvent) => {
  addStatus(event.data);
}

const handleError = (event: backendEvent) => {
  settings.autoskip = event.data.includes("banned");
  console.log(event);
}

const startTyping = () => setTyping(true);
const stopTyping = () => setTyping(false);
\$\endgroup\$
1
  • 1
    \$\begingroup\$ I return early on the twiceSkipping (if twice skip retunrs true that means that we are not with the same session anymore) method because if the function returns true it stops processing of further events since the caller function of eventHandler is a loop \$\endgroup\$
    – PWall
    May 25 at 8:09

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.