Code Review Friday: Post your code and get feedback!

[brett-haralson]

April 17, 2020
The Corvid Masters have organized a weekly code review post and are encouraging the community to reply with your code you want analyzed! I would recommend utilizing their expertise, and also feel free to jump in if you can contribute!

Please reply with these details:
1: What type of code review do you want?

• Refactoring: Want the code to be more readable and scalable?

• Performance: Want to improve the performance?

• Not Working: Do you have code that isn’t working? Be sure to list details!
  2: Include database structure please. Collection name & Field type, etc.
  3: Is this a live site? If so, drop a link so we can see.
  4: Videos are always helpful!
  5: Please submit your code in jsfiddle or code-sandbox so the code from different pages are organized, and you can delete the code later.
  6: Do NOT include any sensitive code (like passwords).

[hi3000]

Thank you for the help, I have a problem that I posted about on the forum but still not working with me.
I have a reference field in my database for user’s email and I’m trying to connect it with the members database so I can use the user’s data. I connected it form the database but when I try to insert the email value in the referenced field it always shows broken reference. I’ve read a lot of answers on the forum but nothing seems to work. I first added the email without any editing then I tried to convert it to a string but neither worked.

let newUser = {
     "title": $w('#title').value.trim(),
     "image": image,
     "host": email.toString() // this is the referenced field 
}

wixData.insert("users", newUser)
    .then((results) => {
    console.log(results)
})
    .then(emailData => console.log(emailData))
    .catch(err => console.log(err))
        // console.log(results)
    })
.catch((err) => console.log(err));

[salman-hammed]

You will need to use the user id not there email

if you wanna connect the reference field you will need the “_id”, in case of the Member database, it’s there “userId”.

you can get the user id by using wixUsers module

import wixUsers from "wix-users";

const userId = wixUsers.currentUser.id;

[hi3000]

@salman-hammed it didn’t work for me, do you know if we need to convert it to a string or something else?

[salman-hammed]

U don’t need to do that. Can u log the newUser object?

[hi3000]

@Salman2301  it's like that when I used the id instead of the email,still showed broken reference in the database.
{title: "Meeting", image: "image://v1/ec0ed2_6aec2259bd4f440cb11e735ad460dcaf…0/ec0ed2_6aec2259bd4f440cb11e735ad460dcaf~mv2.png", description: "asfsafsdfasssssssfffaasd", host: "ec0ed203-48d2-46fe-b348-78b8af4sdad8"}

[salman-hammed]

@hi3000 the logslooks good to me did u try in live site.

[hi3000]

@salman-hammed yes!! it actually works on the live site but broken on the preview.
Thank you so much for your help!

[stephenmccall]

This is pretty amazing team! Thank you.

I have some working code but thought it could be slicker and more robust. It basically uses 4 images used to switch the data within a repeater.

let searchVariable;

$w.onReady(function () {
searchVariable = 'Scrambling'
fillRepeater();
});

function fillRepeater(){
$w('#image1').onClick((event)=>{
searchVariable = "Scrambling";
fillRepeater();
});

$w('#image2').onClick((event)=>{
searchVariable = "Open Canoe Holidays";
fillRepeater();
});

$w('#image3').onClick((event)=>{
searchVariable = "Winter Skills";
fillRepeater();
});

$w('#image4').onClick((event)=>{
searchVariable = "Sea Kayak Holidays";
fillRepeater();
});

wixData.query('CourseItems')
.contains('coursesCategory',searchVariable)
.limit(4)
.find()
.then((results) => {
if (results.totalCount > 0) {
$w('#itemRptr').data = results.items;
$w('#itemRptr').forEachItem(async($w, itemData, index) => {
//repeater data here
})
}
})
}

Thank you

[mikael]

Hello,
Can you kindly take a look on this?
https://www.wix.com/corvid/forum/community-discussion/dropdown-filter-to-repeater?origin=member_posts_page
Thank you

[info87417]

Mikael, the point is so we can hold it all in one location, i habve posted your code below so the other guys can see it easier

Hello, I use a dropdown to filter a repeater.
The repeater is hidden when the page is loaded and the user must select the dropdown first to get the result through the repeater.

I don’t manage to filter the repeater properly.

I’d tried for 1 week unsuccessfully, please help!!!

I use 2 database: 1 for the repeater and 1 for the dropdown

Here the codes:

import wixData from 'wix-data';

$w.onReady(function () {});
let selectedType = ""

export function dropdown2_change(event) {
if ($w('#dropdown2').value)
$w('#repeater1').show();
else {
$w('#repeater1').hide();
}

wixData.query("MyDropdownCollection")
.eq("title", $w('#dropdown2').value)
.find()
.then((results) => {
let firstItem = results.items[0]
selectedType = firstItem._id;
wixData.query("MyRepeaterCollection")
.eq("title", selectedType)
.find()
.then((results) => {
$w('#repeater1').data = results = results.items;
})
.catch( (err) => {
let errormsg = err;
}  );

});

}

Thanks!
Mikael

[clsnlsen]

Hey Stephen
Good to hear you made the code function, now when it comes to refactoring code (making it slick as you put it)
A good start is to ask yourself 3 things,

1. Can you make the code do more with less (less is more concept).

2. Can i improve the runspeed of my code.

3. Would any programmer understand the code architecture (If you have to explain something in comments, its typically a sign that you need refactoring).

Ill say since its corvid number 3 is rarely a concern.

So ill try and guide you a bit instead of telling you directly how I would change the code.

1. the onclick functions are run everytime an image is clicked as soon as they are instantiated. Meaning there is no need to put them in a function that is being run multiple times.

2. Is there a method to merge the 4 onclick functions into a single one ? (Finding this to me will show a better understanding of Corvid and needed in cases where you example had 500 onclick functions)

3. When you make an IF statement, its generally because there is an alternative else case. Otherwise there might be a different runflow possible.

4. Is it needed to have a saved global variable to search after. when the searching happens the second you click and image anyway. (This I would not care about as long as you know its not actually needed but you sometimes see programs with thousands of global variables and then it starts to make sense)

Hope this helps with understanding a bit of thoughts you could use for this code and hopefully will influence you next time you program to make “slicker” code without having to set time off to refactor it.

best regards
Claes

[plomteuxquentin]

@Stephen Here is my go at your code (to complete @Claes Nielsen good comments ps: try to keep code indent for reading)

//use constant to configure your code behaviour
const REPEATER_SIZE = 4;
const IMAGES = {
    image1: "Scrambling",
    image2: "Open Canoe Holidays",
    image3: "Winter Skills",
    image4: "Sea Kayak Holidays",
}

$w.onReady(function () {   
    // bind image need to be done only once so no need to create a 
    // function for that (but that can be done for more complex 
    // code)
    Object.keys(IMAGES).forEach(imageId => {
        $w('#'+imageID).onClick(event => {
            let searchTerm = IMAGES[imageID];
            fillRepeater(searchTerm);
        });
    })
    
    // Init repeater with first image
    // returns a promise that will load the page once the content     
    // is available
    return fillRepeater(IMAGES.image1);
});


// Use separation of concern. Each method/function should be  
// responsible for doing 1 thing: here retrieving the content
// avoid using a global variable if you do not need to persist the // state
function fillRepeater(search) {
    // returns a promise so you'll know when the data is retrieved
    return wixData.query('CourseItems')
    .contains('coursesCategory',search)
    .limit(REPEATER_SIZE) // dont hard code value in code
    .find()
    .then(results => {
        if (results.length) {
            $w('#itemRptr').data = results.items;
            $w('#itemRptr').forEachItem(bindRepeater);
    }).catch(console.error) // <---- SUPER IMPORTANT always ALWAYS //catch error
}

// Use separation of concern. Each method/function should be 
// responsible for doing 1 thing. Here receiving the data and binding it without any regards on where the data comes from
function bindRepeater($item, dataItem, index) {
    // bind the data here
    // do not use $w but $item. $w is already used by Wix and is     
    // not the same as $item ($item is a local selector and $w is a     
    // global selector
}

[yisrael-wix]

@hi3000 The APIs in wix-users are only partially functional when previewing your site. View a published version of your site to see their complete functionality.

[info2035]

Hi, This is amazing! I’ve been struggling with my code so this is just great.
I am trying to code a job-site that would display all available jobs from my database. I’m using repeaters, and am primarily working on two things: 1) incorporating a ‘show more’ button for the description text box that would allow users to view the entire description, and 2) a search box that would allow users to search position titles or companies. If I could get an industry dropdown too, that would be great, but not as much of a priority.

My database is titled JobPosting and relevant Field Names are PositionTitle, Description, Industry.

Here’s my current code for the search:

import wixData from 'wix-data';

export function search_keypress(event){
$w("#dynamicDataset").setFilter( wixData.filter()
  .contains("positionTitle", $w("#search").value))
.then( () => {
  console.log("Dataset is now filtered");
} )
.catch( (err) => {
  console.log(err);
} );

}

& here’s my code for the ‘show more’ & ‘show less’, which almost works perfectly. Only issue is, the description text box isn’t shortened when you load the page, but displays the ‘show more’ button with the ‘show less’ functionality. After you click it though, the text is shortened and the code works fine afterwards (‘show more’/‘show less’ work properly)

let fullText;
let shortText;
let shortTextLength = 300

$w.onReady(function () {

$w("#listRepeater").onItemReady(($item, itemData, index) => {

    fullText = itemData.description
    shortText = fullText.substr(0, shortTextLength) + "...";
    $item("#text5").text = shortText

});

$w("#button1").onClick((event) => {

let $item = $w.at(event.context);
let clickedItemData = $item(“#dynamicDataset”).getCurrentItem()

if ($item(“#button1”).label === “Show More”) {

        $item("#text5").text = clickedItemData.description
        $item("#button1").label = "Show Less"

    } else {

        fullText = clickedItemData.description
        shortText = fullText.substr(0, shortTextLength) + "...";

        $item("#text5").text = shortText
        $item("#button1").label = "Show More"
    }
});

})

Lastly, if you could also let me know in which order both code snippets would go in, that would be great, because I’d need both to function on the same page.

Sorry if this is a lot. Thanks so much!
Juwairia

[sankhak]

import { fetch } from ‘wix-fetch’;
import wixData from ‘wix-data’;
import wixPay from ‘wix-pay-backend’;

console.log(‘Backend Loaded’);

export function wixPay_onPaymentUpdate(event) {

console.log(‘Payment Updated’); // This is not getting fired

}

/ Why is the above method not getting fired in the events.js when admin marks the order as paid? /

[salman-hammed]

The code is correct, the only thing missing was that the code is not called when the dataset is ready. I have rewrite the code and make ire short, so it’s easy to understand I will suggest renaming all the id that you are gonna use in the code to easy to understand e.g: #button => #btnDesc #text5 => #textDesc Instead of export search keypress I used $w selector Which is my preference Here is the code

import wixData from "wix-data";

$w.onReady(function () {
  $w("#dynamicDataset").onReady(()=>{
    // SHORT DESC CODE
    $w("#listRepeater").onItemReady(($item, itemData) => {
      const { description } = itemData;
      // on item ready set the desc shorter
      $item("#text5").text = shortText(description);
      $item("#button1").label = "Show More";

      $item("#button1").onClick((event) => {
        // toggle short desc.
        if($item("#button1").label === "Show More") {
          $item("#text5").text = description;
          $item("#button1").label = "Show Less";
        } else {
          $item("#text5").text = shortText(description);
          $item("#button1").label = "Show More";
        }
      });
    });

    function shortText(text, maxLen = 300) {
      return text.substr(0, maxLen) + "...";
    }
  })

  //  FILTER CODE
  $w('#search').onKeyPress(async ()=>{
    try {
      let filter = wixData.filter().contains("positionTitle", $w("#search").value)
      $w("#dynamicDataset").setFilter(filter);
    }
    catch(err) {
      console.log(err);
    }
 });
})

https://gist.github.com/Salman2301/dd2da2cb356609bd31604375bd6821ea#file-code-shortner-js

[clsnlsen]

Hey
Why is the dropdown value not simply the _id in the first place ? I do not see a reason for the dropdown database.
Other than that your trying to populate the data using a line saying.

$w('#repeater1').data = results = results.items;

This is the code as suggested by wix API in how to set a repeater

import wixData from 'wix-data';

$w.onReady(function () {
  $w("#myRepeater").onItemReady( ($item, itemData, index) => {
    $item("#bookTitle").text = itemData.title;
    $item("#bookSubtitle").text = itemData.subtitle;
    $item("#bookCover").src = itemData.pic;
  } );

  wixData.query("Books")
    .find()
    .then( (results) => {
      $w("#myRepeater").data = results.items;
    } );
} );

I found it in the API documentation where us corvid masters learned all our coding

Repeater - Velo API Reference - Wix.com

In your case you have to use your selectedType as a filter in the query much like your trying in your own code and call it when ever you change the dropdown. The reason for the repeaters onItemReady is to bind the itemData values from the database so when you in the last line bind result.items to the repeater it knows which items use what data.

Hope this helps along the correct path

best regards
Claes

[info2035]

@salman-hammed Thank you so much! It works! There’s a slight lag when typing in the search - any way we can adjust that? Also, if I wanted to allow users to search through Position Titles AND Companies, how would I add that in?

Thanks so much again!

[salman-hammed]

The reason for the lag is keyPress excuted immeditaly before the DOM is render
so, we use setTimeout which will wait for the certain time in our case 40ms (milli second) before excuting the code

I have refactor the code and update my gist
https://gist.github.com/Salman2301/dd2da2cb356609bd31604375bd6821ea#file-code-shortner-js

import wixData from "wix-data";

$w.onReady(function () {
 $w("#dynamicDataset").onReady(() => {
 // SHORT DESC CODE
 $w("#listRepeater").onItemReady(($item, itemData) => {
 const { description } = itemData;

 // on item ready set the desc shorter
 $item("#text5").text = shortText(description);
 $item("#button1").label = "Show More";

 $item("#button1").onClick((event) => {
 // toggle short desc.
 if ($item("#button1").label === "Show More") {
 $item("#text5").text = description;
 $item("#button1").label = "Show Less";
        } else {
 $item("#text5").text = shortText(description);
 $item("#button1").label = "Show More";
        }
      });
    });

 function shortText(text, maxLen = 300) {
 return text.substr(0, maxLen) + "...";
    }
  });

 //  FILTER CODE
 $w("#search").onKeyPress(() => {
 // keypress event trigger before the dom is rendered
 // setTimeout will wait 40ms before excuting the code in filterDs
 setTimeout(filterDs, 40);
  });
// Dropdown with all the company name
 $w("#dropComp").onChange(() => {
 filterDs();
  });
});

function filterDs() {
 // inital filter
 let filterBuilder = wixData.filter();

 let keyword = $w("#search").value;
 let dropComp = $w("#dropComp").value;

 if (keyword) {
 filterBuilder = filterBuilder.contains("positionTitle", keyword);
  }

 if (dropComp) {
 filterBuilder = filterBuilder.eq("positionTitle", dropComp);
  }

 $w("#dynamicDataset").setFilter(filter);
}