Hot answers tagged

30

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() >= ...


20

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


17

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


8

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


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

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

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

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


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


4

It looks fine. Some minor improvement idea: The first regexp rule ((?!(.+?)\\1).*) is really hard to understand. I'd create a named local variable for it to explain its intent. Some reference: Chapter 6. Composing Methods, Introduce Explaining Variable in Refactoring: Improving the Design of Existing Code by Martin Fowler: Put the result of the ...


4

Some immediate improvement ideas are possible at first glance: Avoid duplicated code: defaultTimeZone() appears twice. It's easy to factor that out by changing the branching logic: keep checking the requirements (preferences != null, the preference value not null, and so on) until you find a usable value and return. To handle all other (failure) execution ...


4

as it probably hits a database, i would call userExists from outside the validate function. So, registration method first checks the input. Then calls userService.Register(User user, Errors errors). Register detects that username already exists and throws. And validation does not need to know about usernames and that they are unique or non-unique.


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