22
I don't do C# so pardon the syntax, but you should be able to reduce the amount of code by using a for loop over a string array with your column names.
Again I don't do C# but in Java you could do something like this:
for(String col : new String[]{"article", "account", "dim_1", ...}) {
if (e.SourceCol == _detailSpread.GetColNumber(col)){
...
17
This is one of those places where if you ask 10 developers the same question, you will get at least 15 answers, and they are all valid.
My 2 cents:
It's 2014. Use HTML5.
Discover jsfiddle.net for this kind of thing.
Put "required" on the username and password fields, it's free validation.
There's nothing wrong with in-page CSS. I often mix sitewide and in-...
17
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 ...
15
Can people understand my code?
Its certainly not bad code - but I believe there are some things you can do which will improve the speed with which people like me can grasp what it is doing (and why).
Comments
Some people think well-written code needs no comments. I don't fall into that camp. Looking at your code I see the following:
Total number of ...
15
A shorter version would be to create an anonymous array in the if statement combined with a collection initializer where you will add all of the column names and call the extension method .Any to check if any item matches the predicate:
if (new[] {"article", "account", "dim_1", ...}.Any(item => e.SourceCol == _detailSpread.GetColNumber(item)))
{
...
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
Right, I'll be adding to this answer later on, but for a kick-off, here's a few quick remarks/considerations:
On the hidden input fields
As Martijn said in his comment: most bots will probably parse the DOM, and filter out those fields that are marked as required (looking for labels with a * text node in them), and they'll simply ignore hidden fields. You'...
13
Security-wise, you need to make sure you a) never trust the user and b) never trust the user.
You should realise that anyone has access to the GET parameters, lines like
<?php echo @$_GET['msg'];?>
should thus be avoided. Always call htmlspecialchars() on stuff you get from users etc when printing output. Otherwise I could craft an URL like http://...
13
Right, a short and simple answer to each of your questions, for now. I'll be expanding on each of the issues through updates:
Is it efficient
Define efficient. If by that, you mean: is it fast, then yes, it probably is. Not as fast as non OO code, but faster than most form builders out there.
If by efficient, however you mean: portable, easy to use (as in ...
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
The most obvious mistake — which makes your code malformed HTML — is the extra < before TYPE:
<P>ADDRESS<INPUT
<TYPE="TEXT" … />
The next biggest issue, in my opinion, is that your <P> tags aren't consistently being closed. Only in the earliest days of HTML was it acceptable to use an unclosed <P> as a line break. These ...
11
You are missing ending paragraph tags
your tags should be lowercase
I think that Uppercase is normal for straight HTML, but I suggest using something other than straight HTML. I suggest XHTML or HTML5, and in that case everything should be lowercase.
be careful with your indentation.
When you go looking through code you want to be able to find things ...
10
In this response, I assume that you are making a little script for yourself and some friends, i.e. not a large or critical system (e.g. that could delete other files). So you want some security, but not to extreme measures.
Passwords in Plain Text
As pacifist mentioned, storing passwords as clear text is always a bad idea. If you're just making a little page ...
10
Take care that I can read JavaScript but it was a whole time ago that I effectively wrote in that.
I see you've created your own isNumeric function.
Maybe you can use the isNaN. Documentation over here.
You use this twice:
((k > 64 && k < 91) || (k > 96 && k < 123) || k == 8);
If you use such a statement more than once, you ...
10
Naming
The naming of the arrays related to the operators is not so great:
In $operatorsLetter you have the names of functions that do some operation. $operatorFunctionNames or even $operatorFunctions would be a better name.
In $operatorsCharacteres you have the symbols of operations. These are normally not called characters. So $operatorSymbols would be a ...
10
I would:
improve (avoid) the PHP & HTML mixing.
use one associative array instead of two array in order to define the operators
avoid a while. If the problem is the substraction, then check if the result is < 0 and switch the variable. Because in your case the substraction case will be less probable than other operators.
call_user_func_array could be ...
10
Extract the chk parameter value from location.search and use a "lookup table" object:
var CHK_ID_LOOKUP = {
'aiweb-check': '#ai-for-website',
'anivid-check': '#animated-videos',
'appdevuiuxexp-check': '#app-development',
'augreal-check': '#augmented-reality',
'datacol-check': '#database-collection',
'digbrand-check': ...
8
I'm not 100% sure there is a particularly elegant way to write this. If the code did not have a different body for each if statement, one could rewrite it like this
FORM_VALUES = {
"priority": accident.priority,
"status": accident.status,
"duration": accident.duration,
"service_status": accident.service_status
}
for key in FORM_VALUES:
...
8
I'd like to see this as my goal:1
if accident.status == 1:
transfer_from_form(request.form, accident, [
('reason', str),
('note', str),
])
else:
transfer_from_form(request.form, accident, [
('priority', int),
('status', int),
('duration', int),
('service_status', int),
])
Let's see what it ...
8
First of all, I'd make sure that supporting IE7 is actually required. It's a really old browser by now, and there are a lot of things that are much more difficult to do.
Anyway, as for sending multiple form values, there's a common - but informal - way of doing that without JSON. It's supported by some server-side languages/frameworks (notably PHP, where I ...
7
Don't forget that dict.get() can also take a default:
if request.form.get('reason', accident.reason) != accident.reason:
accident.set_reason(request.form['reason'], current_user)
...that makes it a little shorter.
EDIT: or even
def needs_update(name):
return request.form.get(name, getattr(accident, name)) != getattr(accident, name)
if ...
7
Before thinking I didn't read well, I didn't want to rewrite everything: the final edit is at the bottom of the answer, but please read everything to understand it all. :)
If the code inside the CheckedChanged events for all the checkboxes is exactly the same, you should extract that code and place it in a method. Then, call this method from the events:
...
7
You can streamline the code by caching the selectors; this will improve performance as well. Next, you should use prop instead of attr as the jQuery documentation states:
The .prop() method should be used to set disabled and checked instead
of the .attr() method.
Lastly, instead of using an if..else statement, you can write the condition inline, since ...
7
Your code is really clean, but it could be improved in places:
You could consider placeholder attributes in the input fields on your HTML to display the format of the data that should be entered.
Your closing </style> tag isn't indented on the same level as the opening tag, it should be.
Style-wise, It's easier on people if you can align all your <...
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
Validating HTML5 Code
It seems that you are new to HTML5, so it's a good idea to validate your code. You have 14 errors and 5 warnings in your code:
autofocus="on" should be autofocus
Your email regex is invalid (because of the - which needs to be escaped).
You can't have a placeholder for date, month, week, time (it doesn't make that much sense)
You can't ...
7
First of all, it would help tremendously if you named the URL parameter values and the form element IDs consistently. Why do you have chk=webdes-check corresponding to #website-design? How did you even arrive at the weird abbreviation "webdes"? And why do you need the redundant "-check" suffix? The element IDs make much more sense, I think. If you use ...
7
I think everything Sam Onela said is great. I'm gonna add a few more things.
Don't modify your state directly
In general, it's considered bad practice to directly modify your state object. According to the react docs:
Never mutate this.state directly, as calling setState() afterwards may replace the mutation you made. Treat this.state as if it were ...
6
I'd almost always rather use object literals {} than new Object() for this. Much less verbose and easier to read.
var errorMessage = {
required : "This field can not be empty",
email : "Please enter a valid email address",
number : "Please only enter numbers in this field",
min : "This field should be minimum ",
max : "...
6
From a once over:
Function names in JavaScript should only start with a capital if they are constructors, so Number is no good, since it returns a boolean, I would call it isNumber()
I would put a "use strict"; after $(document).ready(function () {
if (!$.trim($element.val()) == "") { should be
if ($.trim($element.val()) != "") { ( put the ! in the ...
Only top voted, non community-wiki answers of a minimum length are eligible
