gistfile1.txt

– ShortLinkTest – prefer many small @Tests to a single public test that delegates to private methods. Smaller test immediately give you insight what is failing. Basically, each test case is specification of behaviour:

E.g.:
    
    public static final String SOME_URL = "https://address.to";
    
    @Test
    public void acceptsValidPathAndDestination() {
        ShortLink shortLink = new ShortLink("/path", SOME_URL);
        assertThat(shortLink.destination()).isEqualTo(SOME_URL);
        assertThat(shortLink.path()).isEqualTo("/path");
    }

    @Test
    public void throwsIfEmptyPathIsProvided() {
        ShortLink shortLink = new ShortLink("", SOME_URL);
        thrown.expectMessage("Sorry bro...");
    }

	@Test
    public void throwsIfSwearwordIsUsedInAPath() {
        new ShortLink(SWEARWORD, SOME_URL);
        thrown.expectMessage("...");
    }    


    @Test
    public void throwsIfDuplicateLinkIsInserted() {
        ShortLink shortLink = new ShortLink(SOME_PATH, SOME_LINK);
        shortLinkRepository.insert(shortLink);        
        
        ShortLink shortLink = new ShortLink(SOME_PATH, SOME_LINK);        
    }

– ShortLink can be constructed not only out of Strings, but out of Destination value object and, more importantly, UniquePath. That would communicate in the code that ShortLink's path is always unique. Having Path would help getting rid of RandomStringUtils.random(8, true, true) duplicates. E.g. Path.random();
– ShortLinkTest – you can get rid of assertValidationCorrect and associated boolean parameters (we know that boolean params are evil) with the help of JUnit's ExpectedException class.
– ShortLinkTest – we can easily run this test w/o launching Spring context, by mocking out repositories with Mockito. Tests cases would run much faster and would become independent from each other.

– ShortLink – construction look overcomplicated to me. Ideally, I would prefer creating objects via plain old constructor (POCO). It's important to make sure that Cassandra gets its own constuctor to avoid excessive validations during reads, though.


– SwearwordFilter – it can be just SwearwordDictionary. if (swearwordDictionary.contain(someWord)) { ... }. It can also be just Swearwords. 
– To give increase functional cohesion of system, it makes sense to keep SwearwordRepository in the same package with Swearwords (check out Jakub Nabrdalik's talk @ DevTernity 2017). This will let us make SwearwordRepository package-private.
– SwearwordFilter is not just some utility (lv.devternity.shortify.utils). It's part of the core domain, so let's keep it under model.
–  ShortLinkController – it's possible to get rid of manual Json parsing. Data structured returned from reactions to controller, can be auto-marshalled to JSON by Spring. 
– ShortLinkController – technically, you can also accept Commands as a parameter:

    @RequestMapping(method = RequestMethod.POST)
    String generateShortLink(@RequestBody GenerateShortLinkCommand  command) {

– ShortLink.Unchecked – unnecessary namespace "unchecked" in private fields
– ShortLink.Unchecked – buildChecked could have been replaced with "checked" to conform to CQS.

– Visit.vtime – perhaps timestamp, because it's not clear what "v" stands for.

– Reaction / Commands – if a command knows about the reaction class, we can't benefit from a command being handled by different reactions, depending on the context. It's better if the reaction knows about the command.

– naming is inconsistent throghout the project. E.g. String inputUrl, String customPath in GenerateShortLinkCommand vs. destination and path in ShortLink. It's cool to have a domain dictionary somewhere in Wiki, to keep the naming consistent.

– GetAllShortLinksReaction – just return a collection of ShortLink data structures that will be auto-marshalled to JSON. Keep the data structures a sublclass of Command, to avoid naming conflicts with domain model (if any).

– TrackVisitCommand – clientIPAddress -> clientIpAddress, according to the modern Java conventions.

– GenerateShortLinkReaction – compare:

        1) String shortPath = StringUtils.isEmpty(command.customPath()) ? randomPath() : command.customPath();

        2) command.path().orElseGet(this::randomPath);

– according to best practices, consider using constructor dependency injection, instead of field injection

– TrackVisitReaction – in the following snippet, record is not a record – it's a shortlink. Inline command.shortlink() to avoid naming conflicts.

        String shortLink = command.shortLink();
        ShortLink record = shortLinkRecord(shortLink);

– TrackVisitReaction – storeVisitFact stores Visit, not a VisitFact. Or maybe Visit is actually a VisitFact? 
– TrackVisitReaction – the class is definitely not responsible for constructing low-level geo ip query. It's better to encapsulate geo encoding in a separate class. Also, dealing with geo encoding on URL level is too detailed – use of some lightweight http client library is preffered (like Feign)

– SafeRemoteImmediateImpl is not overriding ImmediateImpl's behaviour, so composition instead of inheritance is preferred here.

– Preconditions – new Preconditions(showImmediate). If we think about it, then Preconditions class has nothing to do with showing something in a particular way. So the field is either not properly named, or has a wrong placement. In overall, the whole "showImmediate" concept looks very confusing. It's not the code that is flawed, but the approach itself.

– ValidationException – rename isShowImmediate() to shouldBeShownImmediately() (but it doesn't make the whole idea of "showImmediate" better)
– CustomRestExceptionHandler has some magic HTML formatting inside. Return JSON.