[Custom Code Review] Alternative to chained if statements

Ask the community
, , , ,
1

Question:
What is a more efficient way to nest if statements? I custom coded a sports registration system on a multi-state box. Upon successful payment there are 9 if statements the code shuffles through (by URL) to put the correct user input in the proper CMS. The user input is entering the proper CMS 75% of the time.

Product:
Wix Editor

What are you trying to achieve:
Parent X picks a Pricing Plan from a dynamic page → Enters child information in multi-state box → Pays the proper amount → If payment is “successful” → if wixLocationFrontend.url == “www.sportsclub.com/baseball” → insert into Baseball CMS

What have you already tried:

import wixLocationFrontend from 'wix-location-frontend';
import wixWindowFrontend from "wix-window-frontend";
import { myCreateOnlineOrderFunction } from "backend/checkout.web.js";
import wixPayFrontend from "wix-pay-frontend";

$w("#payNowButton").onClick(async () => {
  	try {
    	const order = await myCreateOnlineOrderFunction(planId);
    	const result = await wixPayFrontend.startPayment(order.wixPayOrderId);
    	if (result.status === "Successful") {
        
        if(wixLocationFrontend.url == "https://www.sportsclub.com/plans/baseball/registration"){

				// Collect data from the form fields
    			let name = $w("#athlete").value;
 			    let birthdate = $w("#athleteBirthdate").value;
   
                // Create an object to store the collected data
			    let formData = {
                "name": name,
                "birthdate": birthdate
                 };

         wixData.insert("BaseballRegistration", formData)
				.then(() => {
					console.log("Data stored successfully");
                    // Additional actions upon successful storage, like     
                    //showing a thank you message
					console.log("Successfully Ordered");
				})
				.catch((error) => {
					console.error("Error storing data", error);
                    // Error handling actions
				})
}//end of baseball if



if(wixLocationFrontend.url == "https://www.sportsclub.com/plans/track/registration"){

				// Collect data from the form fields
    			let name = $w("#athlete").value;
 			    let birthdate = $w("#athleteBirthdate").value;
   
                // Create an object to store the collected data
			    let formData = {
                "name": name,
                "birthdate": birthdate
                 };

         wixData.insert("TrackRegistration", formData)
				.then(() => {
					console.log("Data stored successfully");
                    // Additional actions upon successful storage, like     
                    //showing a thank you message
					console.log("Successfully Ordered");
				})
				.catch((error) => {
					console.error("Error storing data", error);
                    // Error handling actions
				})
}//end of track if statement

Any help would be appreciated! I am sure there is a better way. Thank you.

2

A. What does SalesForce have to do with your code?

B. You’re using the complete URL, which is bad practice, it shouldn’t be hardcoded, and your code would need to be changed based on where you host the site - Use wixLocation.path instead.
Now, this depends on how this page is built, to me it looks like a dynamic page with the URL structure like so:
https://www.sportsclub.com/plans/<TITLE>/registration
If that’s the case, then instead of the entire URL, we are only interested in the second path section <TITLE>:
wixLocation.path[1]

B2. Since we’re only checking a single variable in all cases (path[1]), we can use Switch/Case instead of chaining ifs

switch (variable) {
    case value1: doStuff(); break
    case value1: doStuff2(); break
    default: doStuff3(); break
}

B3. Just to note, even if you were to chain if statements, you should’ve done it via else if so that once a statement is true the program doesn’t check the rest

C. You’re importing wix-window-frontend but not using it

D. Your if statements seem to be performing the exact same thing, with the only difference being the collection you insert into, so - we want your code DRY (Don’t Repeat Yourself) - make a function for it
(I called it register in the code below)

E. You’re using let for variables that do not get assigned more than a single value, use const instead for better memory and performance
(Well, I ended up not saving those variables at all, but point stands)

With all these taken into consideration, the optimized code should looks something like this:

import wixLocation from 'wix-location-frontend';
import { myCreateOnlineOrderFunction } from "backend/checkout.web.js";
import wixPayFrontend from "wix-pay-frontend";

function register(collectionName) {
    wixData.insert(collectionName, {
        name: $w("#athlete").value,
        birthdate: $w("#athleteBirthdate").value
    }).then(() => {
        console.log("Data stored successfully");
        // Additional actions upon successful storage, like     
        //showing a thank you message
        console.log("Successfully Ordered");
    })
    .catch((error) => {
        console.error("Error storing data", error);
        // Error handling actions
    })
}

$w("#payNowButton").onClick(async () => {
    try {
        const order = await myCreateOnlineOrderFunction(planId);
        const result = await wixPayFrontend.startPayment(order.wixPayOrderId);
        if (result.status === "Successful") {
            switch (wixLocation.path[1]) {
                case 'baseball': register('BaseballRegistration'); break
                case 'track': register('TrackRegistration'); break
                default: console.log('Unknown plan'); break
            }
        }
    } catch (e) { console.error(e) }
})

P.S. There’s no loop in your program at all

3

Important

I missed a problem quite major in your code - It seems like you’re giving anyone access to add records to your CMS collection, this thing should not be in the page code (frontend) at all!

You should:

  • Restrict permissions to your CMS so that only admin may add/edit records
  • Have backend/events.js handle this using the event onPaymentUpdate

Also - Since it seems the collections’ data is identical, you should probably store all registrations in a single collection, and simply have another field for the program they registered for

Perhaps something like this should do:

page.js

import { myCreateOnlineOrderFunction } from "backend/checkout.web.js";
import wixPayFrontend from "wix-pay-frontend";

$w("#payNowButton").onClick(async () => {
    try {
        /* Payment data should include program, name and birthdate
            program:    wixLocation.query[1]
            name:       $w('#athlete').value
            birthdate:  $w('#athleteBirthdate').value   */
        const order = await myCreateOnlineOrderFunction(planId);
        wixPayFrontend.startPayment(order.wixPayOrderId)
    } catch (e) { console.error(e) }
})

backend/events.js

import wixData from 'wix-data'

export function wixPay_onPaymentUpdate(event) {
    if (event.status === 'Successful') 
        wixData.insert('Registrations', {
            // Get program, name and birthdate from payment data
            program: '',
            name: '',
            birthdate: ''
        })
        // SuppressAuth may be necessary
}
1 Like
4

DeanAyalon,

Thank you for putting some thought into this.

A. Fixed the sales force. Forum autogenerated that

B. Is it bad practice to use the complete URL because it’s easier to be broken?

And you are spot on, it’s built as a dynamic page and dictates direction. Thank you for pointing me to a better function. I knew my URL method was inefficient but it worked!

I will look into switch/case. This is logic I am not too familiar with. Seems like it provides greater line efficiency compared to an if statement.

B3. That’s the answer I was looking for to just make the bug stop! Thank you for going above and beyond with your answers. Why does running through multiple if statements create less accuracy in code? My first guess is because it doesn’t go back to the correct if statement.

D&E. Thank you for the tips! I love how consolidated the code became and not to mention…efficient.

The process is to end after payment. Not sure why I would need a loop.

Many thanks,
Amber

5

B. It is bad practice for the following reasons:

  1. The code should be built in such a way that it works no matter what domain points to your website.
  2. It is simply unnecessary, the only thing that interested your code in the URL was the second segment of the URL path, and so, wixLocation.path already synthesizes that for you

Switch/case works like so:

switch(variable) {
    case value1: // if variable === value1
        doStuff();
        break;
    case value2: ...
    default: // if none of the cases matched
}

Note that if break is not specified, the code keeps executing the next case regardless of the variable’s value

However, as you can see in the second revision of my code, I didn’t even use that, if the CMS collections are merged, there’s no condition necessary, simply insert to the ‘Registrations’ collection, with a field program set to wixLocation.path[1] (track/baseball/etc.)

B3. Not sure, the way you’ve written the code, it seems to be checking each IF statement, but it shouldn’t be entering them

Just make sure to take my second reply into mind, as the original code by itself is very insecure, you want your backend to create the CMS records, not the site visitrs themselves

Glad to help :slight_smile:

6

Currently the system is setup where members must be logged in to access registration. A simple sign up. Permissions & Privacy for each CMS are set to collect content from members only. Would you still consider this insecure?

Simply listing the payment data in the OnClick function will be enough for values to transfer to the backend?

Lastly, do you know of any way to add payment data to sales > payments information or subscriptions. It seems to be Wix hardcoded with no way to modify. It would be nice for parents to know which child they registered on record. So far I haven’t found anything.

Thank you!

7

Yes, I most definitely would.
So I’m a logged in member, now I have entered the page that has the registration functionality, and now, I can simply insert records without ever paying

No, I have not added the code inserting that data to the payment itself, that should probably be implemented within myCreateOrderFunction()

I’m sorry, I’m not sure I completely followed, sorry