Random quote machine: Any advice, feedback, ways to improve code, style?

Random quote machine: Any advice, feedback, ways to improve code, style?
0.0 0

#1

Hello Campers!! Any feedback on my attempt to code the random-quote-machine?? It’s my attempt of not using any css and or js frameworks!!! I learned flexbox, javascript, css-transitions, along the way, it’s so fun!! If you have some advice, like if have a dirty code, or bad way of naming css classes, or i didn’t use flexbox that good, fill me in!! I’m willing to learn! :slight_smile:

I hope someday I can help others too in feedback and advice!!

Happy Coding!!

codepen link: https://codepen.io/jcunanan05/full/yKaoRm/


#2

Great work !
Luv the changing background :slight_smile:


#3

#4

Not sure what you mean by the above statement. You are definitely using CSS and JavaScript in your project. You do not appear to be using any frameworks, so that part is true.


#5

I think he meant to say not using any css frameworks or javascript frameworks. I’m not sure what the correct grammar for that is. But the commas make it confusing


#6

Thank you for your feedback Xiija! What terms or concepts of designs do you think I can tackle (or search the internet) and learn next to improve my css or styling? I saw one of your codepen loader it is good!


#7

Sorry for the confusion! I’ve edited it as css or js frameworks. :slight_smile: Anyways, what improvements or concepts do you see I need to improve on my code?


#8

There seems to be several unnecessary functions (see below).

#1) You have a function named changeColors which only calls another function named changeBackgroundColor. Why not just skip the middle function (changeColors) and just call changeBackgroundColor([body]) where needed?

#2) The following function seems unnecessary because is is just parsing the JSON text response and converting it into an object. Why create another function for something already handled by an existing built-in function (i.e. JSON.parse)? Also, what is the purpose of the if statement?

function convertToJson(jsonText) {
  if (!JSON.parse(jsonText)) {
    return "";
  }
  return JSON.parse(jsonText);
} 

Getting rid of the above function, would simplify your updateQuote function to the following:

function updateQuote(responseText) {
//  var quote = "";
//  if(responseText) { // Why do you think you need this if statement?  If responseText is null or a blank string, quote was going to be "" and nothing would display to the page based on your checks (which I removed in the code below) for quote (object) having a quote and author property which was not undefined.
    quoteObj = JSON.parse(responseText)
//}
  
  //update html
  quoteStatement.innerHTML = '"' + quoteObj.quote + '"';
  // I did add new code below just in case the author is blank 
  citation.innerHTML = "- " + (quoteObj.author ? quoteObj.author : "unknown");
}

OTHER ISSUES: I noticed another issue inside your onreadystatechange function. The following if statement is fine, but if xhr.status is not equal to 200, your code should not call the other 3 functions (updateQuote, updateTweet, changeBackgroundColor). Why? Because there will not be any data and instead there is probably an error of some kind. Instead, you should think about displaying a message to the user that something went wrong. You probably want an else statement to only call the other 3 functions when xhr.status is equal to 200.

    if(xhr.status !== 200) {
      console.log('Looks like there was a problem. Status Code: ' + xhr.status);
    } 

#9

Thanks for this concept, Eddie! Hmm so I wrap the whole thing in an object or make a class in it of some sort?


#10

Thank you Randell for such wonderful insights! I really appreciate your time reviewing how I code! :slight_smile:

yeah for #1 You’re right I did an unnecessary function. changeBackgroundColor() is readable code enough

for #2 yeah im wrong. I thought when I pass a wrong string this will handle the error and return an empty string. But I’ve removed it.

and for the updateQuote() i put ifs so it wont override my default html text content with an empty string. Is that a good practice? or i put my default text at js file?

and for onreadystate thanks for pointing it out. i forgot the return statement at the end. haha it is a trick that i watch tutorials do in js so they wont use else statement

thank you so much for insights! having others review my code make me a better programmer :slight_smile: