andreicruceru95 / CO452-BlueJ-Apps

0 stars 0 forks source link

Feedback on App03 Course #2

Closed DerekPeacock closed 4 years ago

DerekPeacock commented 4 years ago

Hi Andrei

Your code layout in the Course class is largely fine. However there should definitely be a blank line between your list of constants and your list of variables.

Your list of variables is less readable (too long with a line break, and you have added some unnecessary single line comments such as // Course name

I am not sure why you need a global module variable as well as a list of modules??

I am also not sure why your period variables could not be declared locally?

Also your calculateForPeriod() method is too long and does not have a single purpose. For example you could separate it into at least two methods one of which goes through the full module list and returns a list of modules for a specified period.

andreicruceru95 commented 4 years ago

Hi Derek,

Thank you for your feedback.

I am not sure why you need a global module variable as well as a list of modules?? Sorry, that was something I forgot to delete.

I am also not sure why your period variables could not be declared locally? When I declare the variables locally I get this message : "local variables referenced from a lambda expression must be final or effectively final"

Also your calculateForPeriod() method is too long and does not have a single purpose. For example you could separate it into at least two methods one of which goes through the full module list and returns a list of modules for a specified period. I already have a method that searches for a code, but the result is stored in a clone list that is declared inside the searchByCode method. Is there a way to store the result of searchByCode() in a list from calculateForPeriod()?