HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Form for registering a user

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
formregisteringforuser

Problem

You have one form and you can register a user. It can be god or mortal, but it also requires all the information to let you save the user or the change. When you have all the registers that you want you can edit one or delete it.

I'm new using Angular and I want tips to use it better. I also want to see other ways to do what I did or how I can make my code better / shorter, etc.



var app = angular.module('miApp',[]);

app.controller('tableController', function($scope)
{

var OPC = -1;

$scope.profiles =
{
options: [
{ id: '1', profile: "Dios"},
{ id: '2', profile: "Mortal"},
],
selected: {id:'1', profile: "Dios"}
};

$scope.users = [
{clave:'1', user:'Adrian', profile:'Dios', pass:'adri', confPass:'adri'},
{clave:'2', user:'González', profile:'Mortal', pass:'go', confPass:'go'},
{clave:'3', user:'Ceja', profile:'Mortal', pass:'ce', confPass:'ce'},
{clave:'4', user:'Maytro', profile:'Dios', pass:'ma', confPass:'ma'},
];

$scope.IDS = $scope.users.length + 1;

console.log("ID siguiente:" + $scope.IDS);

$scope.submit = function()
{
if (OPC
input,select {
width: 200px;
margin-bottom: 10px;
height: 27px;
font-size: 13px;
}

button {
width: 100px;
height: 36px;
margin: 10px;
font-size: 14px;
}
table, th , td {
border: 1px solid black ;
border-collapse: collapse;
padding: 5px;
text-align: center;
}
table tr:nth-child(odd) {
background-color: #f1f1f1 ;
}
table tr:nth-child(even) {
background-color: #ffffff ;
}
form span {
color: red ;
font-style: italic;
}
.controles:hover {
cursor: pointer;
}

.controles.ed:hover {
color: blue !important;
}
.controles.el:hover {
color: red !important;
}
.tabla_usuarios {
margin-top: 32px;
}
.tabla_usuarios table{
margin-top: 10px;
}
form {
margi

Solution

Let's start with your HTML



type is optional. By default, browsers execute scripts as JS. The only time I see type being used is when you want to tell the browser it is not a script by putting a type unknown to the browser. Also, Angular 1.2? Really? The latest version is 1.5.

Usuario


You are using
for layouting, putting the elements after in a newline. Use ` instead. is a general-purpose layouting element, when no better element is applicable. It is also block-level by default. This means anything before it goes on top of it, while anything after it goes below it.

The password is required


elements are for labeling inputs. They are not suited for warnings and headers. For headers there is thru . For warning messages, is usually used and commonly wrapped in to layout.

Editar


Markup attributes use the spinal-case convention of naming. Although case is irrelevant, it is usually advised to follow the convention for consistency.

For more detail about elements and their uses, there's HTML5Doctor.

For your CSS...

I suggest you take a look at Normalize.css. It normalizes defaults across browsers, giving you an even playing ground to start your CSS. I also suggest you take a shot at Bootstrap and similar libraries. That way, you can focus more on getting functionality out the door instead of spending too much time styling. Bootstrap covers most of the stuff you are doing, like table borders, error message styling, headers and so on.

.controles.ed:hover {
    color: blue !important;
}


I usually discourage the use of nested selectors. The problem is that when your app grows huge, you tend to use the nest CSS to scope the styles and avoid collision. However, this increases the style's specificity, making it harder to override. I suggest you read about BEM. It's a naming convention that allows you to write styles using usually just one uniquely crafted class name.

Now to your JS

console.log("ID siguiente:" + $scope.IDS);


Try avoiding the use of
console for debugging. Use the browser's debugger to inspect the code. Use breakpoints to take a look at the execution at some point in the code. Refer to browser-specific documentation on how to use a specific browser's debugger. Chrome's debugger is a good start.

$scope.users.push({
    clave: $scope.IDS,
    user: this.user,
    profile: this.profiles.selected.profile,
    pass: this.pass,
    confPass: this.passConfirmation
});


Instead of creating an object and manually copying over the properties, you can use
angular.copy to copy the object holding the form data and push it to users. Editing would work in the same manner, except you copy from users and into the form data.

function clear_form() {
    $scope.user = "";
    $scope.pass = "";
    $scope.passConfirmation = "";
    $scope.profiles.selected = $scope.profiles.options[0];
}


I notice that your form data are immediately in the
$scope. I suggest you nest them under a property like $scope.userForm. That way, you don't collide with new properties as you grow the app.

Also, instead of wiping each property, why not create a function that generates a new object when called and assign it to your data.

function createUserData(){
  return {
    user: null,
    pass: null,
    profile: null,
  }
}

function clearForm(){
  $scope.userForm = createUserData();
}


I also notice
clear_form. JS naming convention uses camelCase for variables. For constants, SNAKE_CASE is usually used like any other language.

profile: 'Mortal',


Suggesting you store the profile as IDs (the numbers) instead of the text. The text is just the UI. That way, you don't have to be comparing strings to get the right value for the dropdown.

For your password confirm, I suggest you create a directive that renders two inputs. For your app, a password confirm directive would look like a custom element:



This way, the app isn't aware of the two inputs. Also, the logic remains inside the directive. All the app knows is that
value binds to userForm.password on the controller. If both inputs are valid, userForm.password will have a value. Otherwise, the directive supplies something like null to userForm.password`.

Code Snippets

<script type="text/javascript" src="https://ajax.googleapis.com/ajax/libs/angularjs/1.2.23/angular.min.js"></script>
<label>Usuario</label>
<br>
<input type="text" name="useri" ng-model="user" placeholder="Usuario" required>
<br>
<label class="warning" ng-show="!formu.pass.$pristine && formu.pass.$error.required">The password is required</label>
<button ng-Click="edit($index, user.profile)">Editar</button>
.controles.ed:hover {
    color: blue !important;
}

Context

StackExchange Code Review Q#117914, answer score: 4

Revisions (0)

No revisions yet.