Initial Review Recommendations

[AndrewBereza]

1. Define separate exception handler for each type of exception
2. Create Controller Advice instead of Controller - it's more appropriate way than controller inheritance
3. SiteIndexService::writeIndexes - index writer must be safely closed, use try-with-resources, same for IndexReader in SiteSearchService::createIndexSearcher
4. Use contructor injection instead of field injection (field injection is not recommended, IDE is highlighting it as a warning)
5. PropertyService environment variable is not used
6. GoogleCloneApplication::setContext doesn't have an effect: it reassigns property that is not used by anything.
7. Tests are using external resources as inputs (spring.io site), they can change/be down/unreachable and tests will fail. All input data must be defined inside tests.

[olesyak655]

Hello,

As for "GoogleCloneApplication::setContext doesn't have an effect: it reassigns property that is not used by anything." - GoogleCloneApplication::setContext is used in the test only

and

as for "Tests are using external resources" as inputs (spring.io site) - it is example of the site url only, and indexes for it was created and located in the local directory of the project before. When test is run - index read from local directory and tests are not using external resources

In the other test service which is used Internet is mocked.

пн, 7 окт. 2019 г. в 10:46, Andrew Bereza notifications@github.com:

1. Define separate exception handler for each type of exception
2. Create Controller Advice instead of Controller - it's more appropriate way than controller inheritance
3. SiteIndexService::writeIndexes - index writer must be safely closed, use try-with-resources, same for IndexReader in SiteSearchService::createIndexSearcher
4. Use contructor injection instead of field injection (field injection is not recommended, IDE is highlighting it as a warning)
5. PropertyService environment variable is not used
6. GoogleCloneApplication::setContext doesn't have an effect: it reassigns property that is not used by anything.
7. Tests are using external resources as inputs (spring.io site), they can change/be down/unreachable and tests will fail. All input data must be defined inside tests.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/olesyak655/testspd/issues/1?email_source=notifications&email_token=ABV3VN2SLB4SNV2C4MF5ZI3QNLSNLA5CNFSM4I6A4CRKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HP7QQ7A, or mute the thread https://github.com/notifications/unsubscribe-auth/ABV3VN2UICJFF5TVHYL4CI3QNLSNLANCNFSM4I6A4CRA .

-- Best Regards, Olesya.

[olesyak655]

I have done My pull request: https://github.com/olesyak655/testspd/pull/2/files

пн, 7 окт. 2019 г. в 11:53, Olesya Koval olesya.k655@gmail.com:

Hello,

As for "GoogleCloneApplication::setContext doesn't have an effect: it reassigns property that is not used by anything." - GoogleCloneApplication::setContext is used in the test only

and

as for "Tests are using external resources" as inputs (spring.io site) - it is example of the site url only, and indexes for it was created and located in the local directory of the project before. When test is run - index read from local directory and tests are not using external resources

In the other test service which is used Internet is mocked.

пн, 7 окт. 2019 г. в 10:46, Andrew Bereza notifications@github.com:

1. Define separate exception handler for each type of exception
2. Create Controller Advice instead of Controller - it's more appropriate way than controller inheritance
3. SiteIndexService::writeIndexes - index writer must be safely closed, use try-with-resources, same for IndexReader in SiteSearchService::createIndexSearcher
4. Use contructor injection instead of field injection (field injection is not recommended, IDE is highlighting it as a warning)
5. PropertyService environment variable is not used
6. GoogleCloneApplication::setContext doesn't have an effect: it reassigns property that is not used by anything.
7. Tests are using external resources as inputs (spring.io site), they can change/be down/unreachable and tests will fail. All input data must be defined inside tests.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/olesyak655/testspd/issues/1?email_source=notifications&email_token=ABV3VN2SLB4SNV2C4MF5ZI3QNLSNLA5CNFSM4I6A4CRKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HP7QQ7A, or mute the thread https://github.com/notifications/unsubscribe-auth/ABV3VN2UICJFF5TVHYL4CI3QNLSNLANCNFSM4I6A4CRA .

-- Best Regards, Olesya.

-- Best Regards, Olesya.