178 - Allow users to input CHT URLs with leading or trailing spaces and or "/"

[mrsarm]

User may enter the URL with blank spaces at the beginning or the end while copy & pasting from a message or from the browser. Same with the "/" char at the end, if the user copy & paste the URL from the the URL bar, it may include the "/" at the end that doesn't change the actual resource it wanted to reach.

Issue: #178

[jkuester]

Looks good. Just a few minor comments. +1 for a full range of unit tests!

[jkuester]

How is this test different from the previous one (testAllMistakesUrl)?

[mrsarm]

Wrong copy & paste, removing...

[jkuester]

When possible I like to avoid having to hack like this with protected methods. Java does not make this easy, but I think in this case we could use Mockito to inject a mock SimpleJsonClient2 into the AppUrlVerifier via a second constructor instead of overwriting the protected methods. The first thing we would have to do is make the json client a class-level variable in AppUrlVerifier (and get rid of getJsonClient()) that is set by the constructors:

	private final SimpleJsonClient2 jsonClient;

	AppUrlVerifier(SimpleJsonClient2 jsonClient) {
		this.jsonClient = jsonClient;
	}

	public AppUrlVerifier() {
		this(new SimpleJsonClient2());
	}

Then, this test code can be updated to look something like this:

	private AppUrlVerifier buildAppUrlVerifier(JSONObject jsonResponse) {
		try {
			SimpleJsonClient2 mockJsonClient = Mockito.mock(SimpleJsonClient2.class);
			Mockito.when(mockJsonClient.get((String) ArgumentMatchers.any())).thenReturn(jsonResponse);
			return new AppUrlVerifier(mockJsonClient);
		} catch (Exception e) {
			throw new RuntimeException(e);
		}
	}

	private AppUrlVerifier buildAppUrlVerifierWithException(Exception e) {
		try {
			SimpleJsonClient2 mockJsonClient = Mockito.mock(SimpleJsonClient2.class);
			Mockito.when(mockJsonClient.get((String) ArgumentMatchers.any())).thenThrow(e);
			return new AppUrlVerifier(mockJsonClient);
		}  catch (Exception exception) {
			throw new RuntimeException(exception);
		}
	}

	private AppUrlVerifier buildAppUrlVerifierOk() {
		try {
			return buildAppUrlVerifier(new JSONObject("{\"ready\":true,\"handler\":\"medic-api\"}"));
		} catch (JSONException e) {
			throw new RuntimeException(e);
		}
	}

[mrsarm]

I like the idea of injecting the Json client (Spring style 😉 ). I don't like Mockito too much, I think it overrides behaviors that the language already offer (like overriding behavior and injecting objects), allowing developers to write code with anti-patterns that the other way is less possible, because that kind of code is less testable unless... mockito.

Anyway, we already have Mockito in the project and it makes the code more compact in many cases (much appreciated when you write in Java), so I'll use it... against my will 😛

[jkuester]

Honestly, Mockito always makes me cringe too. Every time I use it I just get to thinking "there has to be a better way". But, I have never been able to find one (unless your production code is already structured to leverage some kind of dependency injection or service provider framework).

In this case it feels like we are using Mockito the "good" way in that we are just using it to construct objects with mocked behavior that we can inject (and not doing any kind of deep manipulation on the implementation code...).