Hot answers tagged

16

I think surely there must be a way to avoid so many ugly looking else statements. Return early, for example: if (is_dir($this->plugin_dir)) { $this->load_output['error'][] = "PlugCMS has detected the default plugin directory is missing ({$this->plugin_dir}). Please check the installation and try again.."; return; } You can do the same ...


15

I'm aware of return early but the opportunity is unfortunately quite rare Two levels removed as @tim's suggestions, it's quite significant. With his other suggestions applied too, the whole thing becomes quite a bit shorter: if (!is_dir($this->plugin_dir)) { $this->load_output['error'][] = "PlugCMS has detected the default plugin ...


8

Thoughts: Variable names: I hate typing unnecessary text as much as the next dev, but variable names are not a good place to skimp. It's one thing when using i as an index variable, for example, but for others, please, please, please use meaningful names. If you have to provide a key after the fact, this should be a good clue that you've not adequately done ...


8

Grumble, grumble. Browser tab crashed and SO didn't save my draft apparently. I'll try to recreate everything I remember writing: First off, your question is fantastically written. Often one of the most important skills in writing good code is being able to communicate your intent and any problems you face clearly. You've done an excellent job of this. I'...


7

Overal, I think this is very good code, I only have a few pointers. Unused variables ( jshint ). scrollOffset continuousScrollSpeed autoScroll autoScrollSpeed autoScrollDirectionChangeDelay Yoda conditions : (undefined == $(this).data("wwSmoothScroll")) (scrollHandler !== undefined) Either have all Yoda conditions or no Yoda conditions, also compare to ...


7

Early Return You can return early to reduce the nesting of your if statements: if (plugins.isEmpty()) { plugin = null; return; } Also note that I changed plugins.size() > 0 to !plugins.isEmpty(), I think it's more readable. Plugins Field Code like this: plugins = pluginManager.getPlugins(); // assign field plugins = filterPlugins(...


7

The API design is rather weird. You expect your plugin to be called like $().typeText({ selector: '.words' }) But a design like this would be much more natural for jQuery: $('.words').typeText()


6

Stylistic (Always assuming this is not garbled by the pasting) there is inconsistent indentation in some places, parameter lists sometimes have spaces, sometimes they don't and the names aren't that great (e.g. not for notification, o, cs are bad). The comments aren't very useful, except for the one in MBeanContainer, //hide trace..., because it actually ...


6

You can greatly simplify this code by introducing a helper function that calculates the area_score for one of the categories. Let's take the first category as an example. You do three times the same job, with a different expression each time. This can be put into a function that takes an expression and returns the total area for that expression: def ...


6

Wow. That's a lot of code. The very first thing that needs to be done with it is modularization. You need to structure your code such that named functions and/or classes encapsulate shared functionality and make it more readable. For example, if I'm writing a function that takes some user input, validates it, and performs a lot of operations, I'm going to ...


6

This looks pretty good. Some suggestions: [[ is preferred over [ in Bash. This script could benefit from set -o errexit -o nounset -o pipefail. You'll need to set cpu_cores=0 before referring to it in the loop, but that's just best practice. Dividing by one isn't going to do much :) The quotes are a cute feature, but I'd much rather see a summary of the ...


5

In the code I only would modify the variable declarations: var defaults = {blurRadius:2, deblurOnHover:false}, settings = $.extend(defaults, options), img, blurContainers, clone; Personal preference, but you can read a discusion about it here. Answering your questions: The efect work great. I think the approach is good, don't think that adding four ...


5

From a once over; Very well commented Assigning element to element is overkill here and does nothing: var $element = $(element), // Reference to the jQuery version of DOM element element = element; // Reference to the actual DOM element From a naming perspective, I would refrain from using the tag type in the variable name: // Declare method's ...


5

something that I would definitely do on the next iteration, before this code gets too big, is to make the variables meaningful. I can't take a quick look at this and tell you that I know what it is doing or how to plug into it. JavaScript is an older browser language that, for a long time, required using as little code as possible to achieve it's goal, ...


5

One thing that caught my attention: // Check for Leap Year if (parseInt($yearElement.val(), 10) % 4 === 0) { I reckon you know that % 4 === 0 is not a reliable check for a leap year. Although as far as I am aware all leap years fulfill this rule, not all years fulfilling the rule are leap years. I suggest that you try to avoid building calendar logic into ...


5

One thing that jumps out to me is locking your plugin to one element: $.fn.extend({ sdSlider: function(options) { if (options && typeof(options) == 'object') { options = $.extend({}, $.sdSlider.defaults, options); } // be sure the plugin is attached to only one DOMElement if($(this).length == 1) { ...


5

If your intention is to escape certain characters by prefixing them with backslashes, you would be better off with a function that performs that specific task, rather than a generalized function that accepts a dictionary of replacements. In general, performing multiple substitution passes over a string is a bad idea. Certain substitution dictionaries could ...


4

Scanning your code I have a few recommendations to make. If you update your question with a demo I'll make a few more. As I mentioned in comments you have a few obvious issues with globals var native_btn = element; native_type = native_btn.attr('type'); //should be var native_btn = element, native_type = native_btn.attr('type'); And similarly for ...


4

Structure and style looks fine, although I have some general notes: Prefer the strong (in)equality operators (!== and ===) whenever possible Always use braces, even for one-line "blocks". It's nice to save a few keystrokes, but personally I prefer the consistency of always using braces You could skip the defaults for the beforeSend, complete, etc. callbacks ...


4

You could use a closure to create your own proxy. Plugin.prototype.init = function() { var self = this; rows.each(function (groupIndex, group) { self.initGroup(groupIndex, group); }); this.container.on("click", this.options.addBtnId, function (ev) { self.addGroup(ev); }); }; Why are initGroup and addGroup using $.proxy? ...


4

Since your function - as far as I can tell - doesn't rely on a jQuery collection, I'd stick to always using "1 true way" of calling it. I.e. I assume that $("some > selector").formArrayToObject(arr) === $.formArrayToObject(arr); that is, the two ways of calling it give the same exact result. If that's the case, just use $.yourFunction and only that. ...


4

$(window).smartresize(function () { var chosenSettings; // We could map out the defaults first. var defaults = { accordionW: config.wWidth, accordionH: config.wHeight, visibleSlices: 0, expandedHeight: 600, expandedWidth: 1000, contentAnimSpeed: 200 }; // Then, depending on the conditions, create an object of the ...


4

Interesting question, 2 upfront general notes first: Run your code through JsHint.com, there are a ton of little things you could fix Run your code through a beautifier, there are a ton of inconsistencies Also, consider the following You have too many 1 letter variable s,i,o,x,y,e are pretty well understood, but for the rest you should have meaningful ...


4

I started a project with a structure similar to the one you detailed: My first order of business was to move the import plugins out of the discover_dictionaries() function and to the top of the file. My second order of business was to create an iterator which would iterate over the discover_dictionaries() generator and print what it had found: for ...


4

Since you seem to be using Python 3, you could use the new pathlib module from Python 3.4 to have a Path object. Since paths become real objects instead of mere strings, this module improves the safety of path-handling. It is somehow simpler to read than the old os.path and is more expressive. Here is your code rewritten using the pathlib module (not tested ...


4

As @konijn said in the comments, it looks pretty OK, though I do have some comments. I don't know anything about Metalsmith specifically, so it'll just be more general JS/Node comments. Firstly, it looks like you're doing two different things here: Stripping the BOM, of course, which is indeed what the function claims to do Parsing some the file, and ...


4

When you have multiple conditions on the same value, then you should chain the conditions using else if. For example in most in most of the conditions on settings. animation. Also, since your checks on settings.animation are highly repetitive for left, right, top, bottom, it would be better to rewrite with a loop. Instead of this: position = position + ...


4

loadValues() is rather problematic. It checks for each possible data-attribute and if present overrides the value in settings. These repeated checks are just duplicated code which could be eliminated by just calling .data() without parameters to grab all the data-attributes. But the fact that it's modifying settings is a real problem because the function is ...


4

I saw something that could make your code more dry... // be sure the plugin is attached to only one DOMElement if($(this).length == 1) { new $.sdSlider(this, options); } else if($(this).length > 1) { console.error($.messages.severalElements); } else if($(this).length == 0) { console....


4

$(settings.selector).each(function() { var wordsArray = $(this).data('words').split(','); var length = wordsArray.length - 1; var index = 0; var thisWordGroup = $(this);; if (typeof $(this).data('index') == 'undefined') { $(thisWordGroup).data('index', 0); } $(this).html(wordsArray[0]); }); The variables length and index ...


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