Hot answers tagged

31

I have one in the "any other comments" category :) Localization? I know, that's not a concern. Or is it? Localization always seems to get left behind! Ironically, it's exactly for that reason that localizing an application is more often than not, a pain in the neck. But then, how much harder is it, really, to write this: if (items.size() >= ...


21

Like Mat's Mug already pointed out, you have some magic numbers in your code. private final WebhookParameters params = WebhookParameters.toRoom("8595"); private final WebhookParameters debug = WebhookParameters.toRoom("20298"); get rid of them. You shouldn't hardcode the stackexchange api url. Instead put it inside a configuration file. If the api ...


18

Welcome to code review and thanks for sharing your code. When writing UnitTests keep in mind that they are documentation. Test code has less restrictions for identifier names. This means that method names are usually longer and you should not "shorten" your unit test methods. The test name should describe the expected behavior in detail @Test public void ...


12

Don't use RegEx to prevent XSS attacks! Why? Regex is not a tool that can be used to correctly parse HTML. There are much better ways to prevent XSS attacks. An example, using your code, modified to use Spring HtmlUtils. Please note that I changed names s to input and ret to isHtml, as these names indicate what the variable is intended for, rather than ...


11

Putting this as an anwer, because it took a while to find it: I took your code, and put it through some tests..... I made the tests up based on the 'rules'. This is the sort of thing any interviewer will do: public static void main(String[] args) { String[] positive = {"12345abc", "1a2b3c4d", "1234567890ab", "1234a"}; String[] negative = {"", "1", "...


11

Sorry to present the antithesis to an existing answer once more: if the string is neither reusable nor public, a constant makes no sense in my book. (And yes, this question is opinion-based ;-)) I see why you use a string-variable inside the medthod (to keep it readable for longer strings) which I find totally OK. In general: aim to write the code for the ...


10

First off, it would've been nice to see the entire method, or preferrably even the entire class. Having less context makes reviewing harder. That said, I feel there was enough for a review, so here goes. You call file.listFiles 3 times per file. It would probably be more efficient to store the result in a variable the first time you call it so you don't have ...


9

Should be changed: Request mappings are not that RESTful due to the verbs: POST /api/cars/add-car => POST /api/cars GET /api/cars/cars => GET /api/cars GET /api/cars/get-car/{id} => GET /api/cars/{id} PUT /api/cars/update-car/{id} => PUT /api/cars/{id} DELETE /api/cars/delete-car/{id} => DELETE /api/cars/{id} POST can return HTTP 201 ...


8

The idea of the test class is ok: you want to test the service, mock the repository and you check on the outcome. In the test, it would be nice to use the AAA (arrange/act/assert) pattern. In my tests, i call these setup, execute and verify. There is no logic in the service layer, and your method returns the database objects. Also you only have one test, ...


7

This class should instead use an AtomicReference to ensure the state is kept valid. Alternatively, you should incorporate thread-safe handling of the String. Consider: private final AtomicReference<String> stateref = new AtomicReference<String>(); public void setState(final String state) { // only one initializer will succeed (assuming ...


7

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 ...


7

General Feedback the package 'StackCanary' can follow Java standard naming. Ref: https://www.oracle.com/java/technologies/javase/codeconventions-namingconventions.html You do not need to type Tag on both side. Update all instances. HashSet<Tag> tags = new HashSet<Tag>(); // not so good HashSet<Tag> tags = new HashSet<>(); /...


6

I think you shouldn't declare your beans in the XML configuration if you can eeasily avoid it. Just annotate GameRepository and GameValidator with @Repository and @Component respectively, just like you have annotated GameController with @Controller. (See about the different annotations.) Then, I definitely second janos's comment about using a Map. Much more ...


6

Handler is too generic name for a class. Try to find something more descriptive (which doesn't force readers to check the implementation if they want to know what the class does), SerializationHandler, for example. In Java classes could have only one superclass. If you make it an abstract superclass you can't have another one which might be cumbersome in ...


6

Smelly code The parent method smells: public void setDataSource(DataSource dataSource) { this.simpleJdbcTemplate = new SimpleJdbcTemplate(dataSource); } You ask a DataSource, but in fact you need a SimpleJdbcTemplate. this smells... Let's assume I need to test your code. I run my unit tests and pass in a DataSource, It somehow throws this really weird ...


6

Disclaimer: I don't know sqat about Grails createUser I have a few concerns in this method. Using User.countByEmail seems inefficient. I don't know Grails, but the name of this function suggests that it will scan all the users in the database, when it should stop after the first match, effectively doing an EXISTS query. The string parameter in all the ...


6

This is not necessarily an in-depth review, but I see one issue and one improvement: The CRUD method for delete in your service should combine operators Currently you have @Override public Mono<Movie> delete(String id) { Mono<Movie> movie = movieRepository.findOne(id); movieRepository.delete(id); return movie; } This is problematic ...


6

1 Use Logger, do not print all your app info, in production it will mess up server logs. You can use different log level (debug, info). For example use info level in prod, and debug level in dev environment. 2 Do no catch all exceptions in service layers, you can throw them up to controller (if you use spring mvc) and catch all that there. https://spring....


6

Your code is good, but some improvements could be made. Here are my suggestions: Use lowercase for first letter of variable. Example: BrandService BrandService; rename to: BrandService brandService; Use Lombok annotations like @Getter, @Setter, @Slf4j and various @*Constructors and so on. It will make code clean and readable. Use ControllerAdvice for ...


6

fizzBuzzOutputsRepo is an in-memory map and there is no data persistence. So if the web server crashes, then there will be data loss. Even if data persistence is out of scope for the interview question, by default a web server works in a multi-user environment. However, there is no synchronization method to protect the data integrity of fizzBuzzOutputsRepo. ...


6

From an object-oriented point of view there is a better way you can handle this scenario. Feature Envy Currently the code snipped has a Code Smell that is called Feature Envy: The whole point of objects is that they are a technique to package data with the processes used on that data. A classic [code] smell is a method that seems more interested in a class ...


6

Some Concepts I'm not much experienced in the framework used but Unit Testing, Test Driven Development and Behavioral Driven Development are concepts that need to be understood well and then implemented properly. Unit Test Definition by Michael Feathers I've used these rules with a large number of teams. They encourage good design and rapid feedback and they ...


5

I agree it’s a false positive. Sonarqube is way too strict on this issue in my view and creates many false positives with that rule. Often, when catching a specific exception, we know exactly where it came from. For this rule, I gave in and logged the exception on TRACE in addition to what I already logged at whatever different level. Sonarqube is satisfied,...


5

This is a utility method. There's nothing wrong with it being static. Its readability would benefit greatly from (a) using guard clauses instead of having multiple nesting levels, (b) using java bracket style, and (c) using java spacing. You might also want to add logging to indicate why there's no signed-in user, which is presumably a WARN or ERROR, but ...


5

If you want to make the compiler spit out errors when the exceptions aren't caught, make your classes that derive from Exception, but not RuntimeException. Note that unless you have control over the source from whence they came, since you can't change the definition of classes, you have to create your own custom ones. You've got a bunch of random newlines ...


5

SOLID Principles Single responsibility principle A class should have one, and only one, reason to change. This class should take a PlayerRequest and update the state of the corresponding Players. The public void play method does more than that, by changing the state of the current game, creating a new round or pushing the latest turn. It would be ...


5

Optionals do not improve your code. You're using if statements and null checks just like they are intended to be used. There is no need to improve anything there. Edit: If-statements and null-checks are not evil. There is no need to try to get rid of either one and Optionals were not intended to replace them. The evil is having an API made by someone else ...


5

From API design perspective let me pitch in Have a base path and then have the resource name in the corresponding request mapping. Base path /api version /v1 request mapping /fizzBuzz Add HTTP status code in response. As simple as 200 for OK and 400 Bad request should be fine for a sample API. Further reading: HTTP status LIST Add Swagger Docs it's ...


5

PUT vs. POST @PutMapping("/fizzbuzz") public void createFizzBuzzItem(...) PUT and POST can be used to create new resources. The difference is that PUT is idempotent which means: calling it once or several times successively has the same effect But your fizzbuzz implementation has a time stemp that gets updated: FizzBuzzOutput fizzBuzzOutput = new ...


5

Code review UserIdFactoryImpl Why is this called Impl if it does not extend an interface? Why isn't it final or do you want to allow subclassing? It doesn't seem designed for that... public Long buildUserId() { Any reason why this returns a boxed Long instead of a long? Is this something to do with Spring maybe? Always prefer long if an object is not ...


Only top voted, non community-wiki answers of a minimum length are eligible