Hot answers tagged

18

A few things: Variables in JavaScript should be camelCased. PascalCase is traditionally left for types. CSS classes are generally lower-case and hyphenated. Instead of #Name, consider #name (and the corresponding HTML attribute, id='name') Use $ instead of jQuery. $ is syntactic sugar that essentially resolves the same thing, but it's one character instead ...


14

Code should read like poetry People who don't know that jQuery(function); is the same as jQuery(document).ready(function); will have a hard time understanding this code. For better readability start your variable names with a $ if it is a jquery object.: var $btn = jQuery(this); Use descriptive variable names. I really hate all those btn, frm, sub, .. ...


13

I'm polarized by this code. I see many good practices in use here: you use strict, consistently indent your code and apparently lint it too; you correctly use scoping to provide encapsulation, split code in modules and don't use globals; methods are small and clean, the code is easy to read. However, if you ask me to look at it at a more conceptual level, ...


13

In a sense, your code has already been reviewed, though I disagree with the excessively negative assessment. It is far from being awful and some of the worst JavaScript code ever. you manually concatenate your data string instead of using the JSON library As the documentation for $.ajax() says: The data option can contain either a query string of the ...


12

Move all your Javascript out of the way. From Yahoo's Best Practices for Speeding Up Your Web Site: Make JavaScript and CSS External Many of these performance rules deal with how external components are managed. However, before these considerations arise you should ask a more basic question: Should JavaScript and CSS be contained in external files, ...


10

Don't mix German and English code. I'd highly suggest you to use only English in your code (and I'm German myself). Don't mix different variable naming styles and only use variable names starting with a capital letter for constructor function which must be used with the new prefix. A good practice is to adopt the convention of the language you use, in case ...


9

Here are some tips: Use $.getJSON() to get JSON. Also, name the first parameter json for clarity. Note from documentation for jQuery.getJSON() JSONP If the URL includes the string "callback=?" (or similar, as defined by the server-side API), the request is treated as JSONP instead. See the discussion of the jsonp data type in $.ajax() for more ...


9

Yes! Introduce a keyCode variable to hold the one difference between the two otherwise identical code blocks. var keyCode = isNodeWebkit ? 113 : 112; $(document).keyup(function (e) { if (e.which == keyCode) { $.ajax({ type: "POST", url: "/submit/role/", data: {}, dataType: "json", ...


7

Giving your code a quick look does not make me understand a single line. Will you understand it if you go away for two weeks? Giving it a second look, I notice you've added some short comments describing each block. That tells me each block is an excellent candidate for its own function. Notice in the following example how the function given to $(document)....


7

From a quick once over: JsHint.com has very little feedback, good! Consider using use strict to activate strict mode Avoid console.log Both beforeSend , error and complete should be customizable by the caller success() returns ( PlainObject data, String textStatus, jqXHR jqXHR ) you should pass all 3 to success_callback JavaScript is lowerCamelCase, ...


7

In chronological order: session.php: If you're into ternaries, you could always change it to: (session_status() != PHP_SESSION_NONE ?: session_start() ); My Ajax is terrible, so I'll have to skip that, sorry. add_to_cart.php: You call two main variables: $error & $success. For $error, instead of storing strings like 'Error', use booleans. $error = ...


7

This is very weird. If I read the code without this line, I wouldn't have a clue what that might be: var that = $(this); // this is very weird // I suggest the following name, (or thisElement, or something like that) var $elem = $(this); // I use the $ in the name to indicate 'jQuery Object' Saving the object is a good thing, saving multiple DOM look-ups, ...


7

The responsability of a controller is to create ViewModels and return them. This usually imply some boilerplate code, creating new instances, setting parameters, etc.. Then, you need to return the good view, which also leads to some boilerplate code. Your controllers are thin. Very thin I'd say. I think you're searching too far to refactor perfectly fine ...


7

For starters, you can make compound selectors so this: $('#tenancy_furnished').html(initial_target_html_property); $('#tenancy_rent_amount').val(initial_target); $('#tenancy_letting_service').html(initial_target_html_property); $('#tenancy_tenant_find_fee_type').html(initial_target_html_property); $('#tenancy_management_fee_type').html(...


7

I heard you like wrappers So I wrapped your wrapper (jQuery) in a wrapper (your thing) so you can do ajax. Why do you do this again?? Instead of wrapping over (slow) jQuery you could just wrap the native ajax functionality. In general I expect that to be faster. Additionally this frees your API to be changed independently of jQuery and it removes that ...


6

@ApplicationScoped public class GeneralController { ... } I think your bean should be @SessionScoped instead of @ApplicationScoped. Otherwise all of you clients will use the same controller instance. Instead of an array I'd use a List. It's easier to work with. String[] questions = {"What is your age?","Where do you come from?","Are you married?"}; ...


6

Let me start with few common observations. It's better to generate a complete DOM element before you attach it to DOM tree (add to page) if you can, because it tends to be faster. For example setting its text after can trigger another reflow. Do it often enough and it start to matter. Not every change is expensive (e.g. setting ID before or later doesn't ...


6

Without having any other information available as to the restrictions of the project, I have to say that I do not like this implementation whatsoever. The smartest way to go about swapping css "themes" is by ONLY changing class definitions. There are several ways to do this, but in the case of a "themed" application the most practical approach would be ...


6

You can use jQuerys .when function. As $.ajax returns a defered/promise object you can pass that object to jquery.when. Like this. function get_retweet(id) { var url = 'https://api.twitter.com/1/statuses/show/' + id + '.json', karma; // return the defered object return $.ajax({ url: url, type: 'GET', dataType:'...


6

Realizing that you are trying to make reusable code, I figured I would share my methodology, as I believe it will extend your structure here. I like to wrap everything into an object with functions appropriate to AJAX, and I added in your .overlay components: var ajax = { showProgressBar:function($overlay){ var $doc = $(document), ...


6

Lots of if's tend to signal the need for some classes. What are you using for the main routing that's being done by your app? In other words, when you point to domain.com/blog, how does your app know where to go? One way to do routing is to use a front controller. It parses the request and makes the appropriate calls. From there, you can create classes for ...


6

Where you have a load of html elements with similar functions it's useful to either include them in one containing element or give them all the same class. Then you can access them in the JavaScript using: var dataCell = document.getElementsByClassName('dataCells'); or var dataCell = document.getElementById('dataContainer').children; You could use for ...


6

What you should really do here is turn this part into a function with a parameter for your keyCode $(document).keyup(function (e) { if (e.which == 113) { $.ajax({ type: "POST", url: "/submit/role/", data: {}, dataType: "json", success: function (admin) { if (admin == ...


6

If you're using a jQuery collection more than once, make sure to set a variable, this means you don't have to do unnecessary DOM lookups: var $table = $('#table-body'); You can probably improve your append code to make it more legible. Personally I prefer utilising the jQuery methods where possible, such as .text(). data = JSON.parse(data); $table....


6

Security: XSS You are open to reflected XSS via this payload: p=foo","","");alert(1)// Note that the injection takes place into a JavaScript context, which is worse than injections into HTML context because browser filters have a hard time catching it (if you try the payload with eg chrome, it will execute, while "</script><script>alert(1)</...


6

Feedback Nice use of arrow functions, the fetch API (and associated promises), etc. The layout looks nice as well. Suggestions/Review Points Options could appear multiple times The code in getRandomCountry() does not account for the scenario where Math.floor(Math.random()*this.state.countries.length) yields the same value within 4 consecutive calls. Even ...


6

The screen design looks very nice. It is minimalistic, yet pleasing. It almost perfectly fits on the small screen of my phone (only sometimes, I have to scroll down to see all the answer buttons). The flags are nicely big to explore the fine graphical details. To appreciate the shape of the Nepal flag, there should be no border around it, and the background ...


6

There are only two hard things in Computer Science: cache invalidation and naming things. -- Phil Karlton For the life of me I wouldn't think of the $core->extensions property as a list of file extensions allowed to download. The only meaning I'd make is a list of active plugins. So the main refactoring should be renaming sys class and $core ...


5

Small recommendations: It seems to me that you can remove async: false parameter for the $.ajax call. You can remove result.data from the data returned by the ajax call. (After that you should and the line with var colD = result.data). The data will be not really used because you call trigger('reloadGrid'); immediately. On the other side the values for ...


5

You could do: ajax.callback=function(data){ var elements = ["ID", "test", "year", "code", "Label", "contents", "test"]; for (var i=0; i<dataObj[0].data.length; i++){ var td; var tr=document.createElement('tr'); for (var j=0; j < elements.length; ++j){ td = document.createElement('td'); td....


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