Skip to main content
Code Review

Return to Answer

deleted 2 characters in body
Source Link
janos
janos
  • 111.7k
  • 15
  • 152
  • 391

Game

In the code you shared, you never use the setters of the Game class. How about making its fields final and removing the setters? Immutable things are simple, robust, and great. It would be greatIt's best to make everything immutable thatas much as you can.

Also, do you really need the empty constructor?

GameRepository

In getGameById and updateGame you iterate over a list of games. How about using a Map of id -> Game instead? It would seem a lot more natural, and at the same time more efficient.

Formatting

Overall the code is well readable. The one thing where you violate the standard is spaces around braces and parentheses:

for(Game game : games){
    if(game.getId() == id){

The standard (Eclipse does it with Control-Shift-f) would be:

for (Game game : games) {
    if (game.getId() == id) {

Game

In the code you shared, you never use the setters of the Game class. How about making its fields final and removing the setters? Immutable things are simple, robust, and great. It would be great to make everything immutable that you can.

Also, do you really need the empty constructor?

GameRepository

In getGameById and updateGame you iterate over a list of games. How about using a Map of id -> Game instead? It would seem a lot more natural, and at the same time more efficient.

Formatting

Overall the code is well readable. The one thing where you violate the standard is spaces around braces and parentheses:

for(Game game : games){
    if(game.getId() == id){

The standard (Eclipse does it with Control-Shift-f) would be:

for (Game game : games) {
    if (game.getId() == id) {

Game

In the code you shared, you never use the setters of the Game class. How about making its fields final and removing the setters? Immutable things are simple, robust, and great. It's best to make everything immutable as much as you can.

Also, do you really need the empty constructor?

GameRepository

In getGameById and updateGame you iterate over a list of games. How about using a Map of id -> Game instead? It would seem a lot more natural, and at the same time more efficient.

Formatting

Overall the code is well readable. The one thing where you violate the standard is spaces around braces and parentheses:

for(Game game : games){
    if(game.getId() == id){

The standard (Eclipse does it with Control-Shift-f) would be:

for (Game game : games) {
    if (game.getId() == id) {
added 16 characters in body
Source Link
janos
janos
  • 111.7k
  • 15
  • 152
  • 391

Game

In the code you shared, you never use the setters of the Game class. How about making its fields final and removing the setters? Immutable things are simple, robust, and great. It would be great to make everything immutable that you can.

Also, do you really need the empty constructor?

GameRepository

In getGameById and updateGame you iterate over a list of games. How about using a Map of id -> Game instead? It would seem a lot more natural, and at the same time more efficient.

Formatting

Overall the code is well readable. The one thing where you violate the standard is spaces around braces and parentheses:

for(Game game : games){
    if(game.getId() == id){

The standard (Eclipse does it with Control-Shift-f) would be:

for (Game game : games) {
    if (game.getId() == id) {

Game

In the code you shared, you never use the setters of the Game class. How about making its fields final and removing the setters? Immutable things are simple, robust, and great. It would be great to make everything immutable that you can.

Also, do you really need the empty constructor?

GameRepository

In getGameById and updateGame you iterate over a list of games. How about using a Map instead? It would seem a lot more natural, and at the same time more efficient.

Formatting

Overall the code is well readable. The one thing where you violate the standard is spaces around braces and parentheses:

for(Game game : games){
    if(game.getId() == id){

The standard (Eclipse does it with Control-Shift-f) would be:

for (Game game : games) {
    if (game.getId() == id) {

Game

In the code you shared, you never use the setters of the Game class. How about making its fields final and removing the setters? Immutable things are simple, robust, and great. It would be great to make everything immutable that you can.

Also, do you really need the empty constructor?

GameRepository

In getGameById and updateGame you iterate over a list of games. How about using a Map of id -> Game instead? It would seem a lot more natural, and at the same time more efficient.

Formatting

Overall the code is well readable. The one thing where you violate the standard is spaces around braces and parentheses:

for(Game game : games){
    if(game.getId() == id){

The standard (Eclipse does it with Control-Shift-f) would be:

for (Game game : games) {
    if (game.getId() == id) {
Source Link
janos
janos
  • 111.7k
  • 15
  • 152
  • 391

Game

In the code you shared, you never use the setters of the Game class. How about making its fields final and removing the setters? Immutable things are simple, robust, and great. It would be great to make everything immutable that you can.

Also, do you really need the empty constructor?

GameRepository

In getGameById and updateGame you iterate over a list of games. How about using a Map instead? It would seem a lot more natural, and at the same time more efficient.

Formatting

Overall the code is well readable. The one thing where you violate the standard is spaces around braces and parentheses:

for(Game game : games){
    if(game.getId() == id){

The standard (Eclipse does it with Control-Shift-f) would be:

for (Game game : games) {
    if (game.getId() == id) {