14
I scanned through your code and also stepped through it with a debugger, line-by-line. The more I looked, the more things I found that I wanted to point out.
Let's start by looking at some unnecessary comments:
// BASE PRICE
private static String BASE_PRICE = "$1299.99";
// Number of People
private static String NUM_OF_PEOPLE = "3 people";
(Why aren't ...
13
First of all I'd like to congratulate you on the HTML part, that one looks clean and takes almost all best practices into consideration. I say 'almost' since ... yeah well ... these days people argue you should put script tags before the </body> tag. This to avoid http stalling the reflow of the browser. Oh well, for simple applications leave it like ...
13
Well, your post is over a year old, but hopefully this can still be of some use to you!
Javadoc
No need to say the method name again - the method name is automatically part of the Javadoc.
What are the parameter constraints? (To be fair, you don't enforce any constraints in your code, so it makes sense to not mention any in your Javadoc) That said, you ...
13
The typical MVC pattern has a data flow like this:
|--->Model -> View -> Controller ----|
| |
|--------------------------------------------|
So data flows from the Model to the View (When State is updated)
So data flows from the View to the Controller (When the user interacts)
So data flows ...
12
Some general note about your program.
Comments
/**
*
*/
private static final long serialVersionUID = 1L;
Never leave an empty comment. Comment are already something I'm not a fan of, but an empty one is useless.
/*
* @Version: 0.002
* not much in terms of commenting but a lot of this stuff is very obvious
*/
You don'...
12
Disclaimer: if this answer feels like it gets less patient and more "to-the-point" as it goes on, I'm really tired as I am / was writing this and I can't English / logic right now. I promise I mean the very best with this, and I'm having trouble even typing at the moment. This is actually fairly solid code for a C# beginner.
Oh boy, there's a lot to talk ...
11
Is there any convension which says when should I use it for my custom objects? In model layer or in view?
If your question is 'When should I override the default toString() method and use create my own?' then there is one set of answers. If your question is 'when should I call the toString() method (implicitly like "This is my object: " + myobj or ...
11
This sounds exactly like a desire for Dependency Injection (using an IoC container). ASP.NET MVC is perfectly setup for this already with plenty of containers out there for you to use!
You can still have a base constructor take arguments if you wish but I would try and limit how many.
So your controllers would look like:
public StudentController(...
11
I'm not very familiar with JSP and EL (which isn't very good considering I have an exam on it in a few days), but in general Java terms you can simplify it a lot by moving the separate lists to a Map<String, List<MRTBean>> which will do the mapping for you so you won't need all these if-statements.
Initialization
static List<MRTBean> ...
11
In addition to the removed duplicated code, the second version has other improvements too.
Instead of deeply nested if ($isOk) { ... } blocks, it has flattened the logic which is a lot more readable. And instead of $isOK which doesn't give a clue what it's about, he uses more meaningful names like $questionValid and $answerValid.
Note that this kind of ...
10
Layout
Don't hard-code dimensions and positions. On a Mac, everything looks wrong. (I don't have your images, so I'm going to ignore them for the rest of this review.)
Furthermore, the user of the GuiPanel has no clue how large it needs to be, and will also be stuck hard-coding its dimensions.
A better way to do layout is using LayoutManagers. The ...
10
Ok, moving down the list...
I would separate the content of the includes folder into 2 semantically named folders: includes and classes - or if you want to eventually turn this into an MVC, maybe views and classes would be more appropriate.
It's not uncommon to put database credentials in constants, but truth is you should never have more than one ...
9
If you want an honest opinion, I'd say no, this is not a good controller. I won't bother with code styles, but as far as logic goes:
Conditional includes? Not a good idea. You should be writing code that could be included anywhere if necessary (but possibly not used). Using includes like this can quickly turn your project into a complete mess, especially if ...
9
There are several areas that could be improved:
Stop calling it "MVC framework". There is no model, because model is a layer, not an abstraction for DB table. There is no controller, because controller is not responsible for passing data, rendering template and initializing DB abstractions.
Learn about autoloading in PHP. Look up spl_autoload_register() and ...
9
Creating a model just to check the parameters is overkill and not idiomatic in Rails. If you need some extra logic, use the existing model. In this case, I'd make Post#tagged_with accept a nil value which wouldn't filter anything:
def Post
scope :tagged_with, ->(tag) { tag ? <code to filter> : all }
end
class PostsController < ...
9
Naming:
Names like c, r or s don't mean anything, not to you, not to others. Use meaningful names for your variables. c would become controller and so on.
The var keyword:
From the C# Programming Guide:
The var keyword can also be useful when the specific type of the variable is tedious to type on the keyboard, or is obvious, or does not add to the ...
9
MVC thing is hard to be taken right. And yet it is the most popular (my guess) approach in the modern web development.
There are different ways to say what MVC means in particular application. I guess the cleanest part of the trio is the model. I'll call your model the DiceGame.
The DiceGame by itself is the most important part of your application. It ...
8
At face value the code looks OK. I want to focus on one area, though: File Management
You use FileInputStream and FileOutputStream to read and write your files.
Input/Output streams are designed for byte data. You do not have bytes, you have characters.
But, you are trying to shoe-horn these characters in to single byte values, and this will cause all ...
8
Because it's a framework static methods are used as my preferred method to access a class from multiple classes without having to worry about creating variables of instances that I either have to remember globally or re-instantiating the class constantly.
I don't do php, but I'll give this one a try.
Being a framework shouldn't be a reason to prefer static ...
8
If you start thinking with code, you will almost always end up doing bad things. Why? Because it is not natural. OK, no more preaching and let's get to work.
Review of Your Code
I apologize for being blunt.
You are not using C# and .NET coding conventions. You are using Hungarian Notation: m_userInfoViewModel.
You are naming your variables with type info ...
7
Templates
Your HTML code should be separated out into a template file. You should do your best to avoid mixing HTML into your PHP, as your HTML will inevitably grow, and your PHP will get messier as a result.
Post/Get/Redirect
The pattern you are looking for is Post/Get/Redirect. You should always follow this pattern. When a user submits a form using ...
7
PHP is not my area of expertise, so just some generic notes:
4 is a magic number here:
$post = $this->paginate(4);
Why is it 4? What the purpose if this number? A named constant or local variable would be readable with a descriptive name.
public function getMonth($post)
{
$post->month = date('M', strtotime($this->created_at));
$post->...
7
Unfortunately, this is a very large question to ask. So there will be gaps in my answer but i will answer it best i can using code where possible.
You do not want to couple your router with the controllers. They both do different things. If you do combine them, you end up with a mess (as you have found out).
class Router {
private $routes = array();
...
7
First, a few points on case statements. The last three cases all do the same thing. To reduce duplication, you can allow cases to fall through:
case "":
case " ":
default:
rd = getServletContext().getRequestDispatcher(views + "index.jsp");
rd.forward(request, response);
break;
Since, you are falling through to the default, you can just omit the ...
7
I'll skip MVC for now, as I see a whole review for formatting and the such. Perhaps someone else could break down the pattern for you. Right now, let's focus on each individual class.
Model
public!? What's your logic behind that!? Are we programming object-oriented code here? Part of OOP is designing with encapsulation in mind. Encapsulation is using ...
7
Index
It would appear you primarily do three things here. Sort, Filter, and Page. There's not much to say about your paging, but the sorting and filtering logic clutter the action. Try creating an object that does the sorting and filtering for you. Something like this:
Note: I didn't include all the logic you need, however; I did include what I believe to ...
7
MVC
The MVC pattern is described like this: The controller knows the model and the view. The view knows the model. About how components communication with each other: I've seen different opinions on that, but none of them send messages with direct dependencies. Since the View has a dependency to the GameController: Nope, MVC is not correctly applied here.
...
6
There is much wrong with your code..
You are iterating over every record to find a username, what happens if you have lots of users ?
You are not verifying the password !?
It seems like you are exposing the REST services with all the user id's and passwords !?
It seems that if the user knows the /loggedin URL, then the user can skip logging in since you do ...
6
I'm not that familiar with Play, but overall I can't say that there's anything here I actively dislike. The purpose, layout, variable names and all are quite reasonable and easy to follow. Over all, I would say "Good job".
A couple of comments outside of that:
It seems strange that you would have to write all this boiler-plate HTML stuff in code. My ...
6
I would make some changes to getFirstUpper():
public String getFirstUpper(String str)
{
Integer x = str.length();
str = str.substring(0,1).toUpperCase().concat(str.substring(1, x));
return str;
}
The name x is not a very good name as it's only a single character. I'm not even sure if a separate variable is needed for this, but if so, consider ...
Only top voted, non community-wiki answers of a minimum length are eligible
