I
I
Ivan25072021-03-19 18:12:18
JavaScript
Ivan2507, 2021-03-19 18:12:18

Code review. What can be improved in this code?

Hello, I have been studying front-end development for 2.5 months, I decided to switch to practice and wrote a todo list in 8 hours
1) Please write your opinion about the code and how it can be improved (preferably in html, css too, but in priority JS)
2) I want to start learning React, can I start already or is it better to practice on JS?

Link to code: https://codepen.io/domarchuk77/pen/xxRoMBY

Answer the question

In order to leave comments, you need to log in

4 answer(s)
S
Sun_Day, 2021-03-19
@Sun_Day

Frankly, this is bad code. It makes no sense to understand it, it takes a long time to explain all the points (all this will just have to be rewritten to the root). But all newcomers to programming write something like this, this is normal.
A few things can be noted:
1) Naming css classes - read about BEM, you have something unintelligible. Yes, and BEM is not needed here, if you look strictly straight - it has its own tasks.
2) Use strict equality ===
3) Conditions inside methods are a nightmare. In general, in spaghetti methods, the code for working with dom. It's not done that way. You need to decompose the logic and write concise and expressive code.
4) Something could be passed through constructor () when creating an instance of the class. Why shove all this into the constructor itself.
As for html, just try to make a landing page, you will better understand what's what.
In general, do not lay it out seriously anywhere). Well, I advise you to use typescript.
React can be studied of course.

S
Sergey Sokolov, 2021-03-19
@sergiks

Everything was pushed into the Task class. And it would be better to somehow separate it: here is the Task, there may be none or several. Here App is an application, a form for creating a new task; maybe a handler for all events that pop up from tasks, including; collection of created tasks; saving them in LocalStorage; rendering of a part of tasks in accordance with the filter.
Inside the class, the methods have a common prefix of the name taskSomething-there - superfluous, IMHO.
Josco's code contains the names of classes of elements with which to work, where to look, etc. Maybe it's better to make the class independent of the markup and pass already created elements to it. If the elements are created inside the class, keep references to them.
p.s. React yes you can. Or Vue.

A
AlbertEnshtein, 2021-03-19
@AlbertEnshtein

1) do not put semicolons, instead of if-else use switch case, when there are conditions not to perform some function, you need to exit it as soon as possible, for example, in the filterCheckActive function, you can do this:

filterCheckActive(){
      if(this.filter.value != "active") return;        
            this.itemProgress.classList.remove('show');
            this.itemProgress.classList.remove('hide');
            if(this.itemProgress.classList.contains('task-complete')){
                this.itemProgress.classList.add('hide');
              return;
            }
                this.itemProgress.classList.remove('show');                  
    }

2) I think you can start, good luck!)

A
Anvar Shakhmaev, 2021-03-19
@RxR

Bem is wrong, breaks the concept - an element cannot contain other elements. I did not look further, I am sure that there are more errors.

Didn't find what you were looking for?

Ask your question

Ask a Question

731 491 924 answers to any question