Simple search engine code review

[umbral fulcrum]

Hi! I finished the Simple Search Engine project with an inverted index and the strategy pattern on JetBrains Academy. I would like to know if the code is good enough. The repo: https://github.com/metazor/simple-search-engine/tree/master/Simple Search Engine (Java)/task/src/search

[dense fulcrum]

<@&987246883653156906> please have a look, thanks.

[dense fulcrum]

While you are waiting for getting help, here are some tips to improve your experience:

Code is much easier to read if posted with syntax highlighting and proper formatting.

If nobody is calling back, that usually means that your question was not well asked and hence nobody feels confident enough answering. Try to use your time to elaborate, provide details, context, more code, examples and maybe some screenshots. With enough info, someone knows the answer for sure.

Don't forget to close your thread using the command </help-thread close:1027500463647621170> when your question has been answered, thanks.

[half mortar]

Over all codes are good but try to run it if is there any errors are there or not ?

[umbral fulcrum]

There aren't any errors and it passes the tests of the exercise.

[upbeat sinew]

In the future you should probably add a README that explains the project, bonus points if you have pictures

[viral edge]

Also dont use a extra folder inside of your main branch, thats ugly. You already get a main zip when you download that so you will not be overflown with files

[plush jackal]

Naming conventions are hard to follow and ambigious, project structure is all over the place and could do some with organisation of what is actually happening. mixture of design principles and use of static methods which i'd recommend to avoid.

[viral edge]

And use public modifier in your classes, dont do everything package private

[plush jackal]

your 3 classes any,none and all can exist as methods inside of Strategy and have 1 class named StrategyImpl

[plush jackal]

to add to this comment, there isn't an effort to encapsulate methods and implement access modifiers correctly.

nice job @umbral fulcrum maybe implement some unit tests for your code through mockito? this will actually tackle a lot of the design decisions that jetbrains academy have given you.

[red dome]

also, avoid javadoc comments such as

/**
 * This is a cat.
 */
class Cat {
    ...
}

[quaint perch]

Yeah documentation has two rules:

• the code explains how
• the doc explains why

[umbral fulcrum]

I'd add that for a real project, this is just an exercise from my course.

[umbral fulcrum]

That wasn't intentional, I created this repo from the relevant part of the course's testing structure which I don't want to modify to keep the tests working.

[umbral fulcrum]

I will make the naming and structure clearer in the next project. I made the verifyWord method of the All class static, because it doesn't reference non-static methods or fields.

[umbral fulcrum]

Why not use the strictest access modifier possible?

[umbral fulcrum]

That's possible and might be better, but the exercise asked to use the strategy pattern which requires this structure. EntryFinder as the context, Strategy as the interface of the common methods and the strategy classes as the concrete implementations of the strategies.

[umbral fulcrum]

What do you mean by that?

[umbral fulcrum]

I haven't learned about making unit tests yet, I will get to that later in the course.

[umbral fulcrum]

I removed some of the obvious comments. This was my first project where I used Javadoc and I tried to comment everything. 😄

[ionic tendon]

There is a big design problem in your project: if I want to take your project and use the engine for a rest api, for a new gui, or anything else, I just can't, because your engine is tied to the ui class, this also mean that you need to assume that the ui is text based, but also your engine needs logic to handle how the ui works like for example the engine has a startMenu method.

So it's the ui which should holds the engine, not the reverse

[ionic tendon]

Javadoc != comments

[ionic tendon]

Doesn't matter if it is a real project or not, add a readme

[ionic tendon]

Also in readFile

• there is a better alternative to Scanner/File (Files/Path)
• if you use nextLine, then use hasNextLine, don't mix them
• there is no way to configure which file to read
• it's an engine, why does it print stuff ? You are just hiding the exception here, and there is no way for the caller to know if the file was read, that's a bug

[umbral fulcrum]

That's an eloquent way to put it.

[umbral fulcrum]

Portability wasn't a concern, how should it be designed then?

[umbral fulcrum]

That's true.

[umbral fulcrum]

I will keep this in mind.

[umbral fulcrum]

• Good to know that there's a better way, the course taught reading files with Scanner/File.
• Fixed hasNext.
• The exercise requires that the 2nd element of args should be used as the filename.
• I added a call to the UI's printFileNotFoundExceptionMessage method to the readFile catch block, which prints the error message and the stack trace.

[ionic tendon]

As I said, ui hold the engine, not the reverse

[ionic tendon]

• .
• .
• ah my bad, I didn't see it
• no it doesn't solve the problem, readFile should throw an exception or at least return a boolean in such case

[ionic tendon]

This really about portability but about maintenance, by doing that you prevent most improvements to be made to your project, so if you need in the future to add something or change something, you may need to rewrite it because of how engine is tied to ui, this is very bad

[umbral fulcrum]

I will remove the try/catch block and make readFile throw the FileNotFoundException then.

[umbral fulcrum]

I will refactor it such that the UI calls the methods of the engine then.

[ionic tendon]

And remove any code in engine that is based on text ui since the engine shouldn't know that
An easy way to know if what you did works is to create another ui, ie a gui and see how easy it is to link it yo the engine

[umbral fulcrum]

Alright, I will do those.

[upbeat sinew]

sure, but if you ask people to review your project you increase the chance of someone doing it if you have these things. Going in completely blind takes more effort for the reviewer

[umbral fulcrum]

Good point, I will add it to the next one.

[umbral fulcrum]

I refactored it such that the UI calls the engine and added a warning log message to the catch block of readLine. readLine returns a boolean depending on the success of reading the file and main uses that to decide to start the menu.

[dense fulcrum]

Closed the thread due to inactivity.

If your question was not resolved yet, feel free to just post a message to reopen it, or create a new thread. But try to improve the quality of your question to make it easier to help you 👍

[umbral fulcrum]

Thanks for the help everyone, the code improved and I learned a lot.

[ionic tendon]

It's normal that you do nothing in main if it doesn't find the file ? No error nothing?
In the Engine, in the read file, don't use an anonymous logger, use a correct logger, and also, why did you get rid of the approach of just letting the exception go ?
Also use public instead of nothing in engine, and put it, and its dependencies in a engine package (and so move strategy package inside it
In strategy, entryToFind is a string but you always split it in an array, it seems odd to me, why no by default pass an array or list ?
In All, you create an array and then a list, but the list seems useless here, only the array is needed
Also, you are leaking the invertedIndexes from the engine to the strategies, pass them an immutable view instead
And finally, I noticed that the only reason entry finder exist is to call a single method, this class seems useless to me... and by extension the strategy pattern doesn't make sense here (also it can't really work at the first place, because since it's limited to private usage in engine, not only you are limited and don't have the freedom to create implementations because of the enum, which basically break all the advantages of the strategy pattern)
A better approach should be that it is the ui which passes the strategy directly to the engine instead of the engine using the strategy, and also get rid of the enum

[umbral fulcrum]

• readfile logs a warning message if it can't find the file. What should be in its catch block? The course didn't teach what should be in catch blocks or how to handle exceptions properly yet, so I imitated what I've seen in others' code. I don't want to use a proper logger, because I haven't learned about that yet. SonarLint says rethrowing FileNotFoundException in the catch block is wrong, because it's a checked exception.
• Moved Engine and its dependencies into the engine package.
• Removed the conversion of entryToFind to a list and changed it to an array everywhere.
• Changed search to send an unmodifiable view of the inverted index to getFOundEntries.
• The exercise requires using the strategy pattern and I implemented it as they taught it. The context is EntryFinder.
• I created the ChosenStrategy enum to group related constants, I can add more constants to it if needed. The UI already passes the chosen strategy to the engine in startMenu.
• Changed search to be public.

[ionic tendon]

• just dont catch it
• the contexte here should be the engine itself imo, you ask the engine to execute the strategy, entryfinder is currently useless
• the problem is that if you or someone else wants to add a strategy, they need to modify the enum, they can't just add a class

[dense fulcrum]

Closed the thread due to inactivity.

If your question was not resolved yet, feel free to just post a message to reopen it, or create a new thread. But try to improve the quality of your question to make it easier to help you 👍

[umbral fulcrum]

• Removed the catch block.
• Made Engine the context and removed EntryFinder.
• Removed the ChosenStrategy enum.