Please give ur feedback on my random quote machine project

Please give ur feedback on my random quote machine project
0

#1

Hi, fellow campers,

I require your valuable feedback for my random quote machine project. For now, I have not used any frontend library and have made the project purely in HTML, CSS, and Javascript. I would later use any frontend library but for now, I wanted to test my javascript skills. I have done it using the array method as I have not learned yet to work with APIs.
Suggestions for improvement are most welcomed.

Here is the link:
Debug mode: https://s.codepen.io/ridafatima15h1/debug/XPZZex/gakeYZBXVQnk
editor mode: https://codepen.io/ridafatima15h1/pen/XPZZex?editors=1000


#2

Visually very nice. Responsive. I like the fade in/out of the quotes. One suggestion would be to have the new images also fade in/out.

FWIW, I walk my cat on a leash.


#3

Thank you for your feedback!
Adding fadeIn/Out to the images as well is a great suggestion. I would surely work on that.:+1::slightly_smiling_face:

That is simply awesome! :sweat_smile::rofl:


#4

Oh, I don’t know if you’ve run across this in your testing, but I noticed in your Javascript that you don’t have a way to prevent the same quote from coming up twice. In my version. You could check the current value of an element against the value of a member of the copy[random] object.


#5

Yes I have tried to handle this problem like this:

  • I have a limited dataset (20 quotes objects in quotes array), so what I have done is I have created the copy of the original quotes array named copy.
  • I generate a random number by functiongetRandomInt(max), I pass copy array current length as the argument.
  • the randomly generated number is then used to display the quote
  • then I slice off that particular array element so that it does not repeat until the copy array gets empty
copy.splice(random, 1);
  • when the copy array gets empty I recopy it from the quotes array
if (copy.length === 0) {
        copy = Array.from(quotes);
    }

With this limited dataset, I have so far have not noticed the repetition issue but I might need to change this with bigger dataset/ using APIs.


#6

Okay. I see. I got involved in this curriculum when it was in Beta, and did it as a React/Redux project. You are using global variables for some aspects of the application state. Two suggestions regarding using more descriptive names for variables:

  1. “copy” could be renamed “remainingQuotes” or something.
  2. “flag” could be renamed “hasDisplayedFirstQuote”.

As for repetition, you did address it, I didn’t fully understand your code. Your current logic shouldn’t result in any repeats, no matter how big the dataset.


#7

Yes, I have done this project in javascript because I think I should have a strong grip on it before I move on with the frameworks.
And yes regarding variables, you are right, I would be careful about that :sweat_smile:.

Currently, the only chance of repetition is when if the last element of the copy array and the next random element (of the newly recopied array) get same, otherwise it would be fine


#8

Good catch. So, if you were interested in refactoring, why not shed all the logic around the “copy,” and just test whether the newly-selected random quote’s text or author is the same as that displayed in the corresponding HTML element, and if it is, keep pulling new random integers until it isn’t. This can be done by putting the call to randomInt() inside a while loop whose exit condition is the comparison i mentioned above.


#9

Yes actually I thought a way similar to this, But ultimately I choose the splicing of array method for uniform randomness

The problem with the while loop case would be that the repition might not occur consecutively but it would still occur anyway like see this order of randomness
1 4 5 1 6 4

This could be a possible outcome, although it is random still repetition is there.


#10

It’s an academic point, but randomness involves clumping. That being said, from a UX perspective, I understand your decision.

So, is the check for the repetition when getting a new copy array into this piece of code?

function new_Quote() {

    if (copy.length === 0) {
        copy = Array.from(quotes);
        let x = indexOfObject(copy, compare);
        copy.splice(x, 1);
    }

where compare is the object that you “spliced” from the copy array? If so, naming of variables could help you write much cleaner code. Also, you define a range variable that you use once, which strictly worsens efficiency. Some of the info you are using twice, but processor speed is your cheapest resource, RAM is less cheap, and bandwidth is the most expensive. Were it me, I’d find/replace compare with currentQuote, and instead of this chunk where you copy out the properties of the current quote object into local variables, then use them to populate the UI,

    let range = -1;
    let random = -1;
    let text = "";
    let author = "";
    let image = "";
    let tweetText = "";

    range = copy.length;
    random = getRandomInt(range);
    text = copy[random].text;
    author = copy[random].author;
    image = copy[random].image;
    tweetText = textTweet(text, author);
    compare = copy[random];

    copy.splice(random, 1);

    document.getElementById("text").innerHTML = text;
    document.getElementById("author").innerHTML = author;
    document.body.style.backgroundImage = "linear-gradient(rgba(0, 0, 0, 0), rgba(0, 0, 0, 0.7)),url(" + image + ")";
    document.getElementById("tweet-quote").href = 'https://twitter.com/intent/tweet?text=' + encodeURIComponent('"' + tweetText + '" ' + author);

you could get away with four fewer variables being created per iteration, and the same chunk of code would look like:

    let random = getRandomInt(copy.length);
    currentQuote = copy.splice(random, 1)[0];
/* Array.prototype.splice() returns an array of the "spliced out" elements,
 * in this case, it's a 1-element array, so we can access the quote object at index 0.
 */

    document.getElementById("text").innerHTML = currentQuote.text;
    document.getElementById("author").innerHTML = currentQuote.author;
    document.body.style.backgroundImage = "linear-gradient(rgba(0, 0, 0, 0), rgba(0, 0, 0, 0.7)),url(" +currentQuote.image + ")";
    document.getElementById("tweet-quote").href = 'https://twitter.com/intent/tweet?text=' + encodeURIComponent('"' + textTweet(currentQuote.text, currentQuote.author)+ '" ' + currentQuote.author);

#11

@vipatron sorry for getting back late, I have followed your advice and have tried to make my code concise. I would like your review on it.
I have also tried to work on fadeIn/FadeOut for images.
I have added a preloader function, which although has made the alternating of images somewhat smooth, I am unable to get the fadeIn/Out to work.

I have made this sample pen. I have used image transitions and image fadeIn/fadeOut perfectly. I have applied the same to this project however I am unable to figure out why it is not working.


#12

Modularization
I like that you pulled the functions that place the quote info into the HTML out into their own function. Nice job! It also improves readability.

Organization/Global Variables
You take two lines to both declare the var quotes global array and initialize it. You could do both in the same step.
var quotes = [ {...}, {...}, ... ];

Because of your image preloader, you have a global variable img[] floating around in the middle of your code. You should move it up to the section in which you declare your global variables so there isn’t potential for overriding it with a local variable name.

Slimming down your code
In function indexOfObject(arr,obj), in the very heart of the nested for loop, in the innermost level, the 2nd condition of the 2nd if statement is redundant:

function indexOfObject(arr, obj) {
    let objKeyArr = Object.keys(obj);
    for (let i = 0; i < arr.length; i++) {
        for (let k = 0; k < objKeyArr.length; k++) {
            if (arr[i].hasOwnProperty(objKeyArr[k]) === true) {
                if (obj[objKeyArr[k]] != arr[i][objKeyArr[k]]) {
                    break;
                }
                if ((k === (objKeyArr.length - 1)) && (obj[objKeyArr[k]] === arr[i][objKeyArr[k]])) {
                    return i;
                }
            }
        }
    }
    return -1;
}

You’ve already checked for the second condition in the 1st innermost if statement. By virtue of the fact that you didn’t break the for loop, you only need to check whether (k === objKeyArr.length - 1) Notice I left out some of the parentheses you included. If you look at MDN’s operator precedence table, you’ll see that the general rule is:

  • unary (++/--)
  • binary (+-/*)
  • tests (==, <=)
  • boolean functions (||, &&)
  • then finally, assignment(=,+=,*=)

If really readable code is your goal, then that table’s a good thing to bookmark. I made a fiddle to show you an example of this stuff in play See if you can understand why a and b are what they are and why the test passed.

In the Main Function, you are testing your boolean flag, instead of just recognizing that it already has a value of 0 or 1 (truthy values so this code will work, but you’d be better off assigning false initially for legibility). Very common mistake. Now that you’ve named your flag well, let’s see if this makes your application logic more understandable:

function main() {
    if (!hasDisplayedFirstQuote) {
        new_Quote();
        fadeIn();
        hasDisplayedFirstQuote = true;
    } else {
        fadeOut();
        setTimeout(new_Quote, 1000);
        setTimeout(fadeIn, 1000);
    }
}

Variable Name Suggestions

function getRandomInt(max) {
    return Math.floor(Math.random() * Math.floor(max));
}

Since you know you will use this function pretty much exclusively to pick a random index for an array of size max (a very common programming pattern) I would rename it thusly:

function getRandomIndex(arraySize) {
    return Math.floor(Math.random() * Math.floor(arraySize));
}

The only reason I’d do it that way is to separate it from the pattern of getting a random number between 1 and N, inclusive, where max is a perfectly explanatory variable name.

Cross-fade Woes
Now to the juicy stuff! Are you using Firefox? Because I was having the same issues with your sample pen. The top answer to this question on StackOverflow said you did it correctly, but it doesn’t work on Firefox or IE. I tried it on chrome after forking it, and it worked perfectly, but the modified CSS looks smoother in my opinion (timing is your choice):

transition: background-image 1s ease-in-out;

I just had simple colored backgrounds, but I found an example of an idea that I thought would work for you: 2 HTML img elements, one on top of the other by z-index, and you animate the opacity. His example does it on hover, and doesn’t manipulate the z-index, but that should give you plenty to get started (you could keep the z-indices for each element fixed, but alternating which image you place on which canvas by using a boolean flag oddNumberedImage or something.

FYI, I have never heard of a preloader, but it’s awesome with your high-res images. You taught me something I’m totally going to use!


#13

@vipatron Let me first thank you for taking the time to give me an in detail answer. :grinning:
Your all suggestions about code organization, modularization, redundancy and naming variables has really helped me immensely and would be useful for me in the long run. It has given me the idea of how I can improve my code and make it more readable. However, I think I need a lot more learning to do.
I have made all the suggested changes in my code.

I am using chrome browser. Actually, the sample is working fine, transitions occur as expected. I am having trouble with adding those transition to this project.

I have defined the transition property in the body

body {
    height: 100vh;
    width: 100%;
    display: grid;
    align-items: center;
    justify-content: center;
    background-image: linear-gradient(rgba(0, 0, 0, 0), rgba(0, 0, 0, 0.7));
    background-size: cover;
    background-position: center center;
    background-repeat: no-repeat;
    background-attachment: fixed;
    transition: background-image 1s ease-in-out;
    -webkit-transition: background-image 1s ease-in-out;
    -moz-transition: background-image 1s ease-in-out;
    -ms-transition: background-image 1s ease-in-out;
    -o-transition: background-image 1s ease-in-out;
}

and I am changing my image through setQuote() function which is called in the new_Quote() function, that is used in the main() which runs on onclick event of the button. I have also tried changing the transition property tobackground instead of background-image but no success.

transition: background 1s ease-in-out;

I think I am missing out on something here.

I am glad :sweat_smile:, this two-way process is the best thing about learning. I actually learned about preloading images from this article. It worked fine for the 20 images.


#14

I noticed that in your set-quote function you were setting two backgrounds. Rule number one of debugging is: roll it back to when it worked and add things one by one until they break the code. If you’re debugging someone else’s, remove what might be problematic and add the pieces back one by one.

In your CSS, you animate the background-image of your body tag. So, I removed the linear gradient from the background-image and it works now:

//document.body.style.backgroundImage = "linear-gradient(rgba(0, 0, 0, 0), rgba(0, 0, 0, 0.7)), url(" + image + ")";
 document.body.style.backgroundImage = "url(" + image + ")";

You could work on animating the linear gradient as an exercise, but I couldn’t get it work after slapping together some code to replace the above-cited code. Feel free to try:

let randomRed = Math.floor(Math.random()*256);
let randomGreen = Math.floor(Math.random()*256);
let randomBlue = Math.floor(Math.random()*256);
document.body.style.backgroundImage = "linear-gradient(rgba(0, 0, 0, 0), rgba("+randomRed+", "+randomBlue+", "+randomGreen+", 0.7))";

Update: Forked Pen w/ code:


#15

It gets even worse. I remember reading that the reason the transition: background-image is not supported by all browsers is that it’s not the standard, but may be for CSS4 (but probably not). So, that code is browser-dependent. Which is why opacity hacks are a thing. In fact, this article I just found on animating linear gradient backgrounds says:

Always be sure to check if a property is actually animatable. I’m talking to you, people who try to transition from ‘display: none’ to ‘display: block’ and wonder why it’s not a smooth fade-in.

About linear gradients:

Not animatable. This is why animating CSS gradients is an issue. Just like you can’t smoothly transition from one background image to another, you can’t smoothly transition from one CSS gradient to another. It’s binary; either one background-image is shown or the other.

What you can do, however, is have two gradients, stacked on top of each other, and then transition the opacity of the top element. The opacity property is ubiquitous and animatable, making it perfect for this job.


#16

@vipatron That fixed the problem!:grinning: . I applied the linear gradient to main::before instead. so that I only have to change backgroundImage in my setQuote function.

That means, for now, the only cross-browser solution for proper animation/ transition of linear-gradients and background-images is by animating the opacity property.


#17

@ridafatima15h1 truly impressed! You’re way beyond where I was at the time I did this challenge.

I liked:
How clean and organized your code was. The comments in all three files were outstanding and greatly improve the readability of your code.
Your css skills are next level. I’ve been doing css for 2+ years now and I don’t think I could claim to have more understanding of it than you display here.

Room for improvement:
You almost never see id in production code (in my experience). Avoid at all costs and prefer class.
This function seems overly complicated to me indexOfObject(arr, obj). Something with nested loops and multiple if checks is probably going to be error prone and definitely difficult for another developer to maintain. Could you use indexOf instead? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/indexOf

Keep up the good work!

  • Luke

#18

@lhovee thankyou so much for taking the time to give your feedback on my project. I can’t thank you enough for your words of appreciation. Your feedback has given a boost to my confidence at a time when I needed it badly :slightly_smiling_face:.

I would definitely work on your suggestions. To avoid the use of id’s is something new to me. I did a quick reading on this topic and found that the use of class is better than using id performance wise. Thank you for adding that to my knowledge :smiley:.

About indexOfObject(arr, obj) function, actually, I needed to return an object and the problem with indexOf() was that it does not return object. But like you mentioned, I would work on simplifying this function.