carlosmdq44 / carRental

0 stars 0 forks source link

Feedback 2 #5

Open gperelstein-ms opened 3 years ago

gperelstein-ms commented 3 years ago

Good work Carlos, you are doing it very well! I will leave some comments that will help you improve the code.

  1. First, let's start with the styling

See this class

image

It's a bit confusing. You can see that the definition of the class Car in line 7 is at the same level that the definition of the properties and the constructors. This is difficult to read. Think that maybe in the future you are going work on a project with thousands of lines of code and you will have to read a lot of code to understand what is doing. If the project doesn't follow any convention, this reading could be very difficult.

Also, see the curly brackets. For the namespace, you open the curly bracket below the namespace declaration but for the class declaration, you open the curly brackets next to the declaration. Try to follow a convention. This try-catch block is another example of this

image

Also, check the parameters in this constructor

image

Some of them start with upper case and some of them start with lower case. Remember, try to follow the conventions

  1. Recommendation for enums

Try to explicit the value of the enum, for example

public enum Brand
{
    Fiat = 0,
    Ford = 1,
    Chevrolet = 2,
    ....
}

If you don't explicit the values and you store these values in the database and somebody changes the order of the brands you are going to have all the brands mixed. Check that the correct word in English is Brand and not Mark.

  1. Remember to use the using statement to manage files, otherwise, you are not calling the Dispose method and you are not closing the file.

image

Here is the documentation about this https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/using-statement

  1. Some methods could be simplified by better use of Linq. For example, this could be the method for removing a car in the CarCrud
public static void Remove(int id)
{

     List<Car> listCars = GetALL().Where(x => x.Id != id).ToList();
     SaveList(listCars);
}
  1. I have a question. Why do you prefer to do static all the Crud methods?

  2. Try to think of another way to get the next Id. You are assuming the last car is going to have the last Id but what is going to happened if somebody orders the car's file by color?

image

  1. FirstOrDefault method accepts a predicate. So this method:

image

public Car GetById(int id)
{
     return GetALL().FirstOrDefault(x => x.Id == id);
}

That's it for my end, for now. Check this https://git-scm.com/book/en/v2/Git-Branching-Basic-Branching-and-Merging for creating a new branch. Also, I recommend you to check a GUI client for git like git extensions (http://gitextensions.github.io/). It simplifies my life