udacity / frontend-nanodegree-mobile-portfolio

339 stars 3.64k forks source link

Small corrections you should make. #2

Closed mouthbreathingretard closed 8 years ago

mouthbreathingretard commented 9 years ago

The file project-webperf.html, line 8:

<title>Cameron Pitman: Portfolio</title>

Every other page says Pittman, not Pitman.


The file project-2048.html, line 41:

<img class="img-responsive" src="http://cameronwp.github.io/udportfolio/img/2048.png">

That image file is hosted at that link, but I believe what you really meant was this instead...

<img class="img-responsive" src="img/2048.png">

...because the same file is also hosted there, in this repo's assets, and that's the only place students will have any control over the optimization of 2048.png — line 41 stands out from all other links to images in the repo img folder, so I'm pretty sure you meant my suggestion instead line 41.


The file views/js/main.js, lines 152–213: (seems to have been resolved in a commit)

function getAdj(x){
  switch(x) {
    case "dark": [. . .]
    case "color": [. . .]
    case "whimsical": [. . .]
    case "shiny": [. . .]
    case "noisy": [. . .]
    case "apocalyptic": [. . .]
    case "insulting": [. . .]
    case "praise": [. . .]
    case "scientific": [. . .]
    default: [. . .]
  };
};

The final two semicolons are un-necessary. This same mistake happens again in exactly the same way for the next function, getNoun, as shown below.


The file views/js/main.js, lines 216–283: (seems to have been resolved in a commit)

function getNoun(y) {
  switch(y) {
    [. . .]
  };
};

A problem identical to the one just described above — there are two un-necessary semicolons.


The file views/js/main.js, lines 202–206: (seems to have been resolved in a commit)

case "scientific":
      var scientific = ["scientific", "technical", "digital", "programming", "calculating", "formulating", "cyberpunk", "mechanical", "technological", 
      "innovative", "brainy", "chemical", "quantum", "astro", "space", "theoretical", "atomic", "electronic", "gaseous", "investigative", "solar", 
      "extinct", "galactic"]
      return scientific;

Missing semicolon. The same semicolon is missing on line 210 where the same code was copy-pasted to make a default case for the switch statement. (It seems Cameron added default cases of "scientific" and "scifi" for the random adjective and noun generators — he loves science and physics but does he have enough scientific integrity to acknowledge the incontrovertible fact of controlled demolitions on 9/11, I wonder...?)

The semicolon is also missing at the code's source, so this seems to be a case of copying code with errors in it, with no linting.


The file views/js/main.js, lines 152–213:

function getAdj(x){
  switch(x) {
    case "dark": 
      var dark = [...];
      return dark;
    case "color": 
      var colors = [...];
      return colors;
    case "whimsical": 
      var whimsy = [...];
      return whimsy;
    case "shiny":
      var shiny = [...];
      return shiny;
    case "noisy":
      var noisy = [...];
      return noisy;
    case "apocalyptic":
      var apocalyptic = [...];
      return apocalyptic;
    case "insulting":
      var insulting = [...];
      return insulting;
    case "praise":
      var praise = [...];
      return praise;
    case "scientific":
      var scientific = [...];
      return scientific;
    default:
      var scientific = [...];
      return scientific;
  }
}

Besides the extraneous semicolons already mentioned, and besides the missing semicolon already mentioned, there is a bug that no linter will catch. Examine the execution path the app takes based on lines 285–303 of the same file:

var adjectives = ["dark", "color", "whimsical", "shiny", "noise", "apocalyptic", "insulting", "praise", "scientific"];
var nouns = ["animals", "everyday", "fantasy", "gross", "horror", "jewelry", "places", "scifi"];

// Generates random numbers for getAdj and getNoun functions and returns a new pizza name
function generator(adj, noun) {
  var adjectives = getAdj(adj);
  var nouns = getNoun(noun);
  var randomAdjective = parseInt(Math.random() * adjectives.length);
  var randomNoun = parseInt(Math.random() * nouns.length);
  var name = "The " + adjectives[randomAdjective].capitalize() + " " + nouns[randomNoun].capitalize();
  return name;
};

// Chooses random adjective and random noun
function randomName() {
  var randomNumberAdj = parseInt(Math.random() * adjectives.length);
  var randomNumberNoun = parseInt(Math.random() * nouns.length);
  return generator(adjectives[randomNumberAdj], nouns[randomNumberNoun]);
};

The bug here is that getAdj(adj) sometimes evaluates as getAdj('noise') (the 'noise' is passed in from line 302, and it needs to be 'noisy' instead!), and getAdj('noise') is (almost) undefined! It is undefined for the original source code, but in this file, it hits the default case and returns the scientific adjectives array.

So you're never returning any of the "noisy" adjectives from the switch block in getAdj. That's a full bug in the original (the original works when you select "noisy" from the drop-down menu, but it fails when the randomName function runs), but it's just kind of an unobserveable bug in Cameron's Udacity version.

You never get "noisy" adjectives, but users never know that noisy adjectives are possible, so they don't know there's any problem.

Long story short, to fix a bug, you should change "noise" to "noisy" on line 285.


The file views/js/main.js, line 370:

pizzaContainer  = document.createElement("div");

There are two spaces before the =, for no reason.


The file views/js/main.js, line 430:

//TODO: change to 3 sizes? no more xl?

That to-do is ta-done! The app is working with 3 sizes and there is no longer any trace of xl.


The file views/js/main.js, line 494:

console.log("Average time to generate last 10 frames: " + sum / 10 + "ms");

This might be too misleading for students, because it's only reporting the amount of time spent in JavaScript on average for the last 10 frames — while the browser may be spending large chunks of time each frame on Rendering and Painting.

Maybe this distinction is too small to care about, but it would be more correct to write something along the lines of "Average time scripting last 10 frames".


cameronwp commented 8 years ago

Sorry for the delay! (Why so late? Long story short, I've had some bad notification settings on GH for the last few... years.)

Thanks for your suggestions! I just pushed fixes for all the remaining issues.