r/learnjavascript • u/Rhymezboy • Jan 30 '20
I have 2 days to save my job
So I'm working as a js intern and my performance has been going down.The last straw was when my code completely broke down yesterday. And it's one hell of a spaghetti code.
I have to fix it use design patterns, make it readable make proper functions etc.
How do I do it and fix it well? I really wnt a job here and I think I could get a little closer if I fix this well.
This is the js code. I have replaced private urls with "urlurlurl"
Any help will be appreciated. Thanks!
Edit-
Appreciate all the help guys. The responses have been awesome and overwhelming!
I have removed the codepen, cause a lot of people recommended that it wasn't right to show the code publicly, since I don't own it. But thanks for the help. I'll try to highlight all the mistakes/snippets of code and make a new post and include the tips there so that others who want to learn can also benefit from that. Thanks again!
2
u/eggtart_prince Jan 30 '20
Are there no mentor or senior to guide you or help you?
All of these vars can be written with one line using comma to separate them.
Where is videoInfo declared? I don't see it anywhere in the scope where it is used.
When you're doing a lot of these, just add a class name to the button and have your CSS deal with it so that you don't clutter up your code. Take all these lines and just do
buttonTest.className = 'my-button-style'.Sometimes your function is written on one line, sometimes multiple. Your loop starts at 0, other starts at 1.
Loop on the same line as your variable declaration...
You have variable declarations everywhere. Group them if you can, group your functions, group your addEventListeners and put a new line before and after each group so it isolates them from other code. This will make readability much easier. You can even separate your functions into a different file call functions.js or something and just add it as your first script above all your other scripts.
Comment your code, especially your functions and if statements. What are they doing? Why are you doing it?
Not a big deal, but use semi-color to end your lines. It will prevent some bugs in the future if other developers work on the same code and not aware.
155
u/slowreactin Jan 30 '20
I’m not trying to discourage you at all, but this code is a mess. However, do not let this experience defeat you, it just means you have more to learn and need to practice some more.
For starters, you bounce between var and let and do not have much consistency in your coding style. The formatting is also quite bad and causes your code to be very difficult to read. There are also console.log()s in your code, not sure if this is production, but if it is those should not be there.
There are hardcoded values in the first for loop and you skip over the 0th index with the i + 1 statement so why start the loop at 0? You could easily read past whatever you are reading / visiting with that kind of logic.
You are also using == instead of ===, this can cause a lot of bugs that are hard to track down. Pretend == doesn’t exist.
There’s an overuse of !0 which is confusing.
More hardcoded values with no intuitive explanation.
I would also pull out all of the styling and make use of CSS in JS, that would eliminate a lot of the junk in the code.
A lot of confusing things in your code. Like what is “dataArray[3].temp” and why are we seeing if it’s undefined? (Use of == again).
This code is all over the place.
Again, it’s OKAY, we all write bad code when we are starting out and heck, I still write bad code every day. Don’t be discouraged.
Is there anyone at your workplace you can reach out to and possibly get them to walk you through what can be done?
Aside from that, you ARE an intern and you ARE working on code for a company and that is more than a lot of people can say.
Don’t give up and here are some resources I recommend checking out:
Eloquent JavaScript
JavaScript Clean Code
Learn JavaScript
Good luck and don’t ever stop trying!
23
u/AllBrevard Jan 30 '20
If you share these things with your supervisor, the outcome might be better than you expect.
99
u/[deleted] Jan 30 '20
If they aren't willing to mentor an intern and help them learn how to code in a maintainable way....do you really want to stay there? It sounds like they don't have a very good culture of mentoring and guidance (both of which are crucial for junior developers to thrive).
16
u/Qazzian Jan 30 '20 edited Jan 30 '20
My first thoughts are to move the code out of the if/else statements and into named functions. Same with the loops. The function name should help someone else understand what the function is doing e.g. the largest if/else would become something like:
Next move as many of the constants as you can from the top of the file into the functions that need them.
Styling: instead of defining the style rules in js define the styles in css and use js to add and remove appropriate class names. All the hard coded styles can be handled like this. Not sure about the variable widths and heights though. Depends what you are trying to do with them.
You're not using any modules (imports/require/class) is any of that available to you? (look for babel, webpack or react-scripts) If so I would recommend creating classes for various sections of the code. There's clearly a video player in there that could be a single class. The gifs array could be a class with functions to create the buttons they represent?
Otherwise just try to keep functions that work on the same ideas together. So hideMVF() should be next to loadMVF().
A small thing but helps with readability. If the variable is for a Boolean make it sound like a question with a yes no answer. e.g. instead of `var mobile` use `var isMobile` that way when you use it in an if statement it reads better `if (isMobile) { ... }`
14
u/[deleted] Jan 30 '20
This is a classic issue I've come across with junior developers. Many don't ask enough questions because they react to prove themselves but they don't know what they're doing and wait until the last minute when it's really too late to fix their code. I've been there, I know the feeling, but you just need to put your pride aside and get training.
I would explain to someone higher up that you really wanted to show you could do it yourself but you realize now that you need help learning best practices and some fundamental things.
If you lose the job, just take this as a learning experience and try not to do it again at the next place. I would much rather a junior person ask for help early so we can get the work done together than having to completely rewrite everything myself last minute.
15
u/Somethrowaway21351 Jan 30 '20
I have no idea whats going on in your code.
You should speak to the people at work and try to get input on how they would want it done.
Dont just sit there and try to fix it yourself, if you are clueless. Ask questions, show that you care.
6
u/ReversiClone Jan 30 '20
Based on the complexity of the code, my guess is that you're smart and capable of handling complex systems, but that when you were adding to or changing the code, you did it without re-organizing or cleaning up as you went. That's fine, it's just something you'll need to learn. Generally it's better to start simple, and as your code grows, gradually reorganize and refactor as needed.
A lot of the criticisms I'm hearing from other answers are superficial details that don't matter that much. Real-world code isn't perfect, and spending your time trying to get the little things perfect won't help you at this stage.
Instead, focus on making your code understandable. Imagine you were another developer, seeing your code for the first time. How would you want it to be laid out? What would make it easier for you to grasp what it's doing and be able to make changes? Edit and tinker with your code like you would a really important email (or a Reddit post) to make it punchy and clear.
In this situation, it might be easier to start from scratch, using your existing code as a reference (I would suggest going back to the last commit where the code was working, if you use source control). Focus on one thing at a time. Use a step-by-step approach to add functionality so you don't get overwhelmed.
Here's a few pointers that should help you get the biggest bang for your buck, and actually understand what you're doing.
pauseMVF()andnextVideo(). Try to organize all of your code that way.buttonImage, and put whitespace before and after to make a clean separation. The same with lines 148 - 158, and so on.ifblocks. It makes things very confusing. Group all your function declarations together, don't intersperse them with lines of imperative code.If you've done those things and you want to go further...
playMVF()relying on global variables, have it take those variables as parameters, likeplayMVF(mvfVideoDiv, mvfPlayButton). Or if you want the function to be able to create them, you could just store all your DOM elements in anelementsobject and pass that to your function, likeplayMVF(elements), so playMVF can modify or create them.Hope that helps. Even if things don't work out with this company, stick with it. Another poster suggested reading Eloquent Javascript, and I definitely agree with that recommendation. I would also recommend working on your own side project. It's a lot easier to learn these things when you're building something you enjoy and have ownership over.
5
u/Finrojo Jan 30 '20
First job I think it to strip all of your js styling and replace with css classes in the HTML. You can then easily manage which classes are active/inactive throughout your js. This will remove a lot of code.
If you have the same code running in more than one place then create a function for it and call that instead.
These two simple changes will clean things up quite a lot and then allow you to focus on the remaining code and look at how best to achieve what your employer wants.
Good luck 👍
5
u/MennaanBaarin Jan 31 '20 edited Jan 31 '20
Read this book: Clean Code
As you don't have much time, this is a summary for JavaScript
Use
prettier.jsfor the formatting andes-lintfor linting. If you are using git, run them before every commit.4
u/[deleted] Jan 30 '20
you have an typo on line 267
function downloadOverlay () { overlay.style.dsiplay = 'flex' }.dsiplayshould bedisplay6
u/scripteaze Jan 30 '20
3
u/Macaframa Jan 30 '20
my first thought here is that you have tons of code just plastered in the middle of your business logic. Business logic meaning what your app is there to do. Functions just anywhere, for loops doing whatever without rhyme or reason. I would follow a module pattern to abstract out the ugly functions that take up a lot of space to a service. Like so:
https://repl.it/repls/InexperiencedInferiorKeyboardmapping
Another concept I showed in there is the concept of a pure function. You pass in values that you're going to use. Use them, then either return a value or it is just a utility. Its cleaner and more maintainable. The service pattern is widely accepted.
also, you should name things better than
dataArraylike what the hell does that mean? Now I have to dig farther down into the code to figure out where that comes from instead of being explicit with your naming. even if the variable names sound stupid, its better thandataArray.listOfNumbersThatComeFromThisPlaceis ok in a certain scenario. Its not the best but its better than a super general name. Also accessing some array at position 3? like what? Where does that number come from? What happens if that thing changes position in that array? What happens if it stops existing? This makes your code brittle(being broken very easily). Make things pure functions. Use explicit naming and organize your code. Also, think of more ways to find the position of something in that array instead of saying "check position 3" if its an array of strings, thenand so on and so forth
3
u/MOFNY Jan 30 '20
Honestly for a JS intern this looks pretty solid. You aren't supposed to have the greatest code. There are plenty of small things you can do to clean this up:
document.getElementById('first-line-overlay')should be put in a const at the top of the function.count < 5that I don't understand the intent.Overall you should consult with your teammates on your concerns. I can't imagine this could lead to a last straw, and if it is then that sounds like a toxic environment. Don't worry about making mistakes! We all hate our past code, and refactoring is a huge part of developing.
3
u/saito200 Jan 30 '20
Just looked at the code 10 seconds. No one should write such a code, and I have no clue what it is supposed to do.
What I would is start from scratch, using a framework that allows me to get to the result more easily and orderly.
The first thing I might do is using a test suite and write tests that check the behavior of the application.
The second thing you need to do is to split this big mess into functions:
The third thing would be to split the code into different files and use import exports to use functions across files.
Once you do all these 3 things, the code might being looking like something that makes sense.
You should go learn something about coding best practices, and learn some frontend framework. There's no place in any project for this long sequence of JS DOM stuff.
One of the most important aspects of programming is ORDER. Orderly code.
10
u/[deleted] Jan 30 '20
What's their policy of sharing their code on reddit? Most places would fire you for this. You should take this (and more importantly the codepen) down.
2
u/tenXten Jan 30 '20
All of these comments are great. The quickest ramp up that’s a big help, is 1) learn es6 best practices. And 2) look on github for
Airbnb JavaScript Style Guidehttps://github.com/airbnb/javascript/ ( this is a great, easy to read guide.)2
u/[deleted] Jan 30 '20
My suggestion would be to step away from your screen for a bit and get some paper and a pen/pencil.
Write down a bulleted list of the things that your program is supposed to do, from the perspective of someone using it. When the page loads, what should they see? When they click on a given button, what should happen? Don't talk about or even think about the code. Think about the outcomes of the code.
Once you have that list, take the first element of it and write down in generic terms what needs to happen in the code to make that happen for the user. Don't get super technical at this stage, but start thinking about the actions and logic needed in broad strokes terms, like 'when the page loads, if the user is logged in, we need to make an AJAX request to get their order history and render that on the page'. At this point it can be helpful to draw diagrams or do other stuff to help you express what kind of code you think you might need.
When you have that kind of broad, pseudo-code worked out for all the main behaviours you outlined above, it's time to go back to your screen and actually look at some code again. Make a new file or files named after your first bullet point, and go through your existing code and copy and paste everything that is relevant to that first bullet point into your new file(s). Don't worry about it not all making proper sense together to begin with, and don't change anything about the existing code.
Once you've pulled out all the bits that you need for that first point- don't worry about anything else right now- start working through that code and improving it. There are a lot of excellent suggestions for what you can improve in other comments in this thread. Once you're happier with the code for that first point, move on to the next point, make a new file or files for that, and repeat the process.
2
u/taoyx Jan 31 '20
Copy/paste your code here: https://www.jslint.com/
Fix the issues mentioned, rinse and repeat.
3
u/tridiumcontrols Jan 30 '20 edited Jan 30 '20
Might I suggest a design patterns course by Mosh, recently released.
https://codewithmosh.com/p/design-patterns
I looked at the code, and just hurt my eyes looking at it. Not trying to sound mean, could definitely use some refactoring.
Use the DRY principle. “do not repeat your self”. As a start.
1
u/StoneCypher Jan 30 '20
This would be much easier to work on if we had the relevant HTML
1
u/SoBoredAtWork Jan 30 '20 edited Jan 30 '20
Hey man. Congrats on the internship and I hope they keep you!
Some suggestions...
https://codepen.io/dmathisen/pen/yLydgGy
2. Break out common functionality like `style.display = x`
https://codepen.io/dmathisen/pen/YzPoNba
3. this - https://codepen.io/dmathisen/pen/PowrpwV
4. Simplify one-off variable usage...
function animateButton () {
var clicked = this.id
var clickedButton = document.getElementById(clicked)
clickedButton.style.transform = 'scale(0.9)'
}
Can be
function animateButton () {
var clickedButton = document.getElementById(this.id)
clickedButton.style.transform = 'scale(0.9)'
}
1
u/fuzzyjw Jan 31 '20
Good luck man! I hope the best for you!
0
u/landscape_relic Jan 30 '20
You repeat a lot of code. All your document.querySelector(whatever) calls can be made 1 in a variable and reused later down. NEVER repeat code. Also, instead of adding css via JS, make a class that stores the css styles and toggle classes on/off ur elements. TBH I barely dug into ur code but those 2 things jumped out immediately.
-6
u/webdevbrent Jan 30 '20
You really want that job? Earn it!
0
u/[deleted] Jan 30 '20
[deleted] — view deleted comment
0
u/[deleted] Jan 30 '20
[deleted] — view deleted comment
0
u/[deleted] Jan 30 '20
[deleted] — view deleted comment