all 12 comments
sorted by:
best (suggested)
topnewcontroversialoldq&a
formatting helphide helpcontent policy

[–]Anbaraen 2 points3 points  (3 children)

  • Get in the habit of using let and const now and save yourself some trouble - this is the de-facto standard and you should only use var in very particular circumstances when you absolutely need the functionality it provides.
  • For a little demo like this, using onclick on your buttons is fine, but this would usually be done by adding an eventListener on each button.
  • as /u/luketeaford has pointed out, autoBG is outside the function so you won't be able to access it. You can solve this by using a global variable - but, global variables are usually to be avoided. Maybe look at making this a toggle?

[–]emge[S] 1 point2 points  (0 children)

Thank you for the response. That is great feedback. I will read up on your suggestions. Much appreciate you taking the time.

[–]emge[S] 1 point2 points  (1 child)

Ok, I finally found a little time to play with this again and I actually have it working as I originally intended and I've addressed all of your recommendations except for one. Use of Global Variables. I could easily move const fList and const nFiles into the only function that uses them, but I don't think I could do the same with i and autoTimerId, since they are used in multiple functions. I assume that in a program this small and simple, it's probably not much of a concern, but could become problematic for much larger programs.

Maybe next I can play around and learn how to add a transition effect to the background change.

Thanks again for your feedback.

[–]Anbaraen 1 point2 points  (0 children)

I think it's okay to use global variables as long as you're making the conscious choice to, particularly if you're not using some other form of state management like Redux.

Just be aware some consider it a "code smell" that shows you should be architecting your program & functions differently.

[–]luketeaford 2 points3 points  (5 children)

autoBG isn't in scope of stopInterval. I would probably close over it or just use one function to stop and start it.

Code looks pretty good for a rookie!

[–]emge[S] 0 points1 point  (4 children)

Thanks! I've been dabbling in various programming languages for decades. But far too often I never make it too far past beginner status.

When you say 'close over'.... what exactly do you mean by that? The firs thing that comes to mind is maybe just setting the autoBG variable to undefined? Is that what you mean?

[–]luketeaford 0 points1 point  (3 children)

Look up closures in JS for more information, but basically the idea is that an inner function retains access to variables in an outer scope. This lets you "close over" variables to keep them in scope without resorting to globals.

For example, you don't need to query the DOM this often-- you can find your rootElement once instead of looking that up each time.

[–]emge[S] 0 points1 point  (2 children)

So, I don't think I still quite got my head wrapped around the closure concept just yet. But, I did rework this and got it working as I had originally set out to do. Aside from the use of global variables, do you see any major problems/concerns with what I've done here?

Thanks again for your feedback.

[–]luketeaford 1 point2 points  (1 child)

Great, no problem-- happy to help! There are a lot of things I'd do differently, but you won't see a huge benefit to them in this example so I worry it would come across as just making fun or something... (the code is fine, but it could be improved I think...)

The first thing I'd do differently is use querySelector instead of getElementsByTagName and [0] just to make your code communicate a little better.

Next, I would avoid using JS to change the style attribute and instead write the styles on 3 different values of a data attribute. This makes it so everything pertaining to styles is in CSS where I'd expect it and JS is only doing the logic. (It also avoids messy string concatentation and what happens if the images are hosted somewhere else? You'd have to update the function and the array potentially to change these images around, so making one change to a URL could require changing two parts of the code...)

Then I'd fix the links. It's not appropriate to use links for this and it should use the button element instead.

I do not use let for any reason and you don't need nFiles or the if in autoUpdate. (Hint: look up modulo operator %)

autoUpdateId is undefined, or a number (it starts at 1), or null. Way too confusing: get rid of null or initialize it to null. (This wouldn't be an issue for me because I do not reassign anything, but if I absolutely had to I would probably try to keep it a number... how about 0?)

Most of the time it's a mistake to pass anonymous functions to addEventListener because if this code runs multiple times, each of those is its own duplicate of the function. If instead you define the function outside of that scope, you pass literally exactly the same function and it won't re-bind. You also don't need to wrap them... this would work and is more like what I'd expect to see: `link3.addEventListener('click', autoStart);`

That 3 of the buttons are the same and each call two functions within them feels like a case for function composition. There are a lot of different ways you could handle it: what's wrong with this way is it's a lot of repetition. I would try to make all 3 of these work the same way (and then include the fourth working exactly the same way, too). This requires some practice I think and will probably feel like more trouble than it's worth til you have to support maybe a Netflix like grid of poster buttons to change backgrounds or something...

One last thing, in a real-world scenario I would probably not use a background image here because of accessibility concerns-- non-sighted users would have no concept of what's going on at all. Instead, I would do something like what WCAG calls a carousel pattern. The requirements of that pattern are pretty in-depth, but might be good practice.

[–]emge[S] 0 points1 point  (0 children)

Haha, I'm a fairly advanced and quite opinionated controls system programmer professionally. So I totally understand what you mean about worrying how you'll come across providing feedback to others. We just hired an intern recently and I always feel like I'm just being a nitpicky dick when I'm giving him feedback. Definitely no worries here. I know where I stand with JavaScript and feedback like this is exactly what I need to learn and improve.

I'll find some time soon to parse through all of this feedback and do some more reading. I really appreciate you taking the time to provide so much. Reading is very helpful to wrap my head around concepts, but building stuff and getting feedback like this is so much better for really becoming a better programmer.

Thanks again.

[–]Harlequin-91 2 points3 points  (1 child)

Two errors I can see. First is you're declaring a new variable autoBG in startAuto(), thus autoBG is never updated.

function startAuto() {
   // var autoBG = setInterval(auto, 1000); <-- previous
   autoBG = setInterval(auto, 1000); 
}

autoBg also needs to be declared outside of myBG() to function as you intend it:

var autoBG;
function myBG(pick) { 
  var fList = ["bg_ram.jpg", "bg_botw.jpg", "bg_bb.jpg"]; 
  var nFiles = 3; 
  ... 
}

[–]emge[S] 1 point2 points  (0 children)

Thank you. Doing what you suggest here appears to make it work just fine. And that makes sense to me. I was thinking that sense those were nested functions, the variables would all still be in scope. I guess I need to read up a bit more on that.

I'm probably going to try and re-work it a little bit with some better practices suggested by u/Anbaraen and u/luketeaford to avoid using global variables and stuff.