14

Use CSS classes rather than inline styles. Instead of modifying the style of each TODO item, use CSS classes. This offloads the styling into a style sheet where it belongs, and it still allows you to test an item to see if it is already completed: function completed() { if ( !$(this).parent().hasClass('completed') ) { $(this).parent().addClass('...


14

I see a number of things that may help you improve your code. Don't abuse using namespace std Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. Know when to use it and when not to (as when writing include headers). In this particular case, I happen to think it's perfectly appropriate because it's a single ...


10

Redundant properties You have a few redundant properties. The margin on your 2nd body declaration is doing nothing because you haven't set a width. html, body { margin: 0; /* original */ padding: 0; } body { font: 14px 'Helvetica Neue', Helvetica, Arial, sans-serif; line-height: 1.4em; background: #f5f5f5; color: #4d4d4d; ...


9

Self-documenting code char h1 = { (e.hour / 10) + '0' }; // Extract the first digit and convert to char (if any, else 0) As you considered the division by ten and the 0 too enigmatic you added a comment, this is much better than leaving the reader wondering so good job. The even better option though is stating your intention in code: char h1 = ...


8

$scope.user = gon.current_user What's gon? I assume it's your authentication/session object with all the things the app needs before running. Best if you put it inside a service or a value so that you can easily use dependency injection with it. That way, your controllers are uniform and only depend on stuff that comes from dependencies, not from ...


7

3 things Comments Consistency Fewer new lines We all know that this is the Enter Key, but you should give a comment why you are using a Magic Number here $('#new-text').keyup(function(e) { if (e.keyCode === 13) { addListItem(); } }); You use different bracing styles throughout your code, and it gets a little confusing. function ...


7

Don't have time to do a full review but you probably want to reorganize your style and script loading. This will allow the browser to download the stylesheets concurrently while downloading+executing your scripts. Also you probably want to load jQuery before knockout as knockout will delegate to the more robust jQuery method where applicable. In the press ...


7

Lyle's Mug gave a nice answer regarding how to improve the JavaScript overall. I'd like to instead look at the HTML: Don't put an input inside a heading. HTML elements have meaning, and an <h1> element means "this is a heading for the document or a part of the document". But it seems you're just using it to make the input larger. Besides, an input is ...


7

Your code looks nice and is properly formatted. Still, a few points could be improved. Magic methods As mentionned in ChrisWue's nice answer, the __str__ magic method is supposed to return a string and is not expected to have any side-effects (such as performing some printing). Also, I would not consider it a good idea to change the signature of a usual ...


6

This will be my first review, so bare with me. I'm going to step though every function: First, I'd start with saving the reference to the todoList. This way, you only call it once and can use it thoughout the script: var $todoList = $('#todo-list'); // I use the $ to indicate a jQuery Object var $newText = $('#newText'); // Dito for newText addListItem() ...


6

return { tasks:tasks, numCompleted:null } numCompleted isn't being used anywhere. Remove if from the data. Also, it's a "computed property", one you can compute off from tasks. I notice you compute it on render. Consider putting it in a function instead. getCompletedTaskCount: function(){ return this.state.tasks.filter(task => task.completed)....


6

From staring a while at your code; Grokking Your HTML stretches really wide sometimes, for maintainability I would indent more: <td class="col-lg-5"><span data-bind="text: title, visible: !editable(), click: $parent.toggleEditable"></span><input data-bind="value: title, disable: isDone, visible: editable, hasFocus: editable" /></...


6

Even though @Herickson already made most of the points but one thing he kindly left for me to note: strlen(3) returns the type size_t which can be anything (but mostly an unsigned long). You should have got a warning from the compiler. C is a language that is quite dangerous to the permeability of your feet and you should not risk it by switching off ...


5

Don't pollute the global namespace. You have 1 variable and 3 functions all going into the global namespace. This is especially a problem because of the generic names you have. So they could be overwritten without you knowing it and it will cause issues. The easiest way to fix this is to wrap all your code in a self executing function. (function() { .....


5

From your answer to my comment, I will drop the intended Python 3 paragraph (even though there are some goodies you may like in the most recent versions, such as the new pathlib that supersedes the old os.path module) and go straight to the main point: Qt Designer is the way to go Writing user interface code is something that nobody likes to do. You could ...


5

A minor point, though it has the potential to get you into trouble: From the GCC manual section 1.3.3 - Reserved Names: reserved names include all external identifiers (global functions and variables) that begin with an underscore (‘_’) and all identifiers regardless of use that begin with either two underscores or an underscore followed by a capital ...


5

Just a few more comments I haven't seen anybody mention. Avoid magic constants An obvious example would be: cout << "\033[2J\033[1;1H";. If you're going to use ANSI escape sequences, I'd recommend giving them meaningful names, then writing them out: const std::string ClrScr = "..."; const std::string Home = "..."; // ... std::cout << ClrScr; ...


5

Your "clear" button To get your clear button to work, you just need to add the event listener to each button as its created, rather than at page load. To do that, take out your .forEach() function and add this line to the end of your addField function: remove[remove.length - 1].addEventListener('click', removeField); You'll also want to change the remove ...


5

I'm not exactly sure of your environment (I'm going to assume you're using Node), and with this assumption, I want to address a few things starting with specific things, then more general: Specific You shouldn't be keeping inputText in the global store. This creates lots of store dispatches and updates which are completely unnecessary and costly. Instead, ...


5

I have several comments about styling: check the indentation and fix it. There are some different styles that can be observed but universally lines that are inside a function (or even more generally - a scope) would have one indentation level more than before. Spacing - be consistent. In one line you have checkbox.value = "value"; with spaces around the ...


4

There are a lot of things wrong here that I'm probably not mentioning at all. There is so much code that I instead want to focus on the big picture problems and the repeated problems (that you need to correct now and keep correct in the future as you write more code). weak vs strong IBOutlet properties should not be declared as strong unless you have good ...


4

There's some stuff that's confusing in this coding and it's structure. A few things of note: First when defining a class you do not need the beginning ( and ending )(); What that is for is to create a nameless function that will then be run once and then for all intents and purposes be removed from memory after. You are wanting to create a class that can be ...


4

Here are some recommendations: Use if as expression when it's appropriate. var done = "" if (isDone) { done = "X" } The better way is to write: val done = if (isDone) "X" else "" Also try to use val as much as it's possible. That will make your code simpler and more reliable. Don't write obvious comments like comments for toString or toLine ...


4

Your program expects the user to refer to completed or incomplete tasks by a numeric index. However, the Markdown file contains an unnumbered bulleted list, which forces the user to count the items manually (starting from 0). That design is inhumane. I suggest that you change the Markdown to use numbered lists. By the way, "uncompleted" is not common in ...


3

I can't offer a full review for you right now, but here are a couple little things: A couple simplifications In ToDoContainer.render, this: var numCompleted = this.state.tasks.filter(function(task){ return task.completed; }).length var numRemaining = this.state.tasks.filter(function(task){ return !task.completed; }).length is doing extra work. ...


3

From a quick once over: Your first line creates an anonymous function, it offers you the opportunity to pick a great function name that conveys to the reader what he is about to be exposed to like (function todoApp(){ This init: function () { this.todolist = []; this.todolist = JSON.parse(localStorage.getItem("tododata")); if (this.todolist === ...


3

I'll dissect your JS from top to bottom. But first, the overarching stuff: you tagged this question with OOP, but it's not taking advantage of OOP. It would be very natural to have Task and TodoList classes that you could build upon instead of having some giant methods object. // VanillaJS Todo App with full CRUD Just a comment that CRUD usually implies ...


3

Two things: Don't use functions before you define them. Yes, this works fine with function declarations, but still it's a bad practise that disrupts the reading flow. Don't use global variables. All these functions are declared globally even though you don't seem to need this, and pollute the global scope. Instead, you should move them into the the module ...


3

for (int i = 1; i < size; i++) { for (int j = i; j > 0 && (list[j - 1].hour > list[j].hour || (list[j - 1].hour == list[j].hour && list[j - 1].minute > list[j].minute)); j--) { The condition in the second for is pretty hard to read. You should extract it to a function.


Only top voted, non community-wiki answers of a minimum length are eligible