-
Notifications
You must be signed in to change notification settings - Fork 5
Coding Guidelines
Here are some vary common coding guidelines, which you can find many places on the web. They are given in books like Code Complete, 2nd Ed and Clean Code.
Also known as: No Magic Numbers
In a controller class, one project has:
@Security.Authenticated(Secured.class)
public static Result showRateResult() {
int currentUserID = Integer.parseInt(session().get("userID"));
User currentUser = User.find.byId( currentUserID );
if(currentUser.projectId == 99 ) {
List<Project> projects = Project.find.all();
List<RatingCriteria> criteria = RatingCriteria.find.all();
MultiKeyMap result = RatingRecord.summarize();You can improve this code by instroducing named constants:
public interface ExceedConstants {
public static final int ADMIN_GROUP = 99;
public static final String SESSION_KEY = "userID";and then in the code:
import static util.ExceedConstants;
...
@Security.Authenticated(Secured.class)
public static Result showRateResult() {
int currentUserID = Integer.parseInt(session().get(SESSION_KEY));
User currentUser = User.find.byId( currentUserID );
if(currentUser.projectId == ADMIN_GROUP ) {
List<Project> projects = Project.find.all();You should remove the constant literals everywhere, not just this class.
Use the IDE refactoring command to find all occurrences. Then use grep to verify
that you really eliminated them all.
Using projectId in this way is hacky. Better to apply the Player-Role Pattern
that is used in the JAAS framework (and every other authorization framework)
and introduced earlier:
if( currentUser.hasRole(Roles.ADMIN) ) {
List<Project> projects = Project.find.all();There is a hidden assumption in this code from TeamMalee:
@Security.Authenticated(Secured.class)
public static Result votingResult() {
long maxindexProject = 0;
long maxindexVote = 0;
long maxindexCriteria = 0;
List<resultVote> results = new ArrayList<resultVote>();
List<Project> projectList= Project.find.all();
if(projectList.size() > 0) {
maxindexProject = projectList.get(projectList.size() - 1).id;
}
...
for (int i=0;i< maxindexProject;i++){
Project tempProject = Project.find.byId((long)i+1);
if ( tempProject != null ) {
resultVote temp = new resultVote(tempProject.name, 0);
results.remove(i+1);
results.add(i+1, temp);
}
}The hidden (and unnecessary) assumptions are: (1) projects are numbered sequentially from 0, (2) the last project added to list has the largest project id.
The assumptions are unnecessary. They could write:
List<Project> projectList = Project.find.all(); // poor design: accessing Finder directly.
...
for (Project p: projectList){
resultVote temp = new resultVote(p.name, 0);
Long id = p.id;
// use a map from project id to resultVote
if (! results.containsKey(id) ) results.put(id, temp);
}In TeamMalee's code again:
-
resultVoterefers to a class name (see above code). Most class names begin with uppercase, so this one should, too. - Elsewhere in the same controller class:
@Security.Authenticated(Secured.class)
public static Result GotoAddProjectPage() {
if(session().get("type").equals("Admin")) {GotoAddProjectPage is a method. It should start with lowercase letter, for consistency with other methods. "type" and "Admin" are constant literals that would be better as named constants.