James Shore: Challenge #6: Refactoring

Challenge #6: Refactoring

The test in this challenge is similar to Challenge #5, in that you need to handle bad form data and log a warning. Your main focus will be on refactoring your production code.

Instructions

1. Implement the "logs warning when duplicated form field found (and treats request like GET)" test:

Configure the request body to be "text=one&text=two".
Assert that HomePageController.postAsync() returns homePageView.homePage().
Assert that it doesn’t call the ROT-13 service.

Assert that it writes the following log message:

{
	alert: "monitor",
	endpoint: "/",
	method: "POST",
	message: "form parse error",
	error: "should only be one 'text' form field",
	form: {
		text: [ "one", "two", ],
	},
}

2. Revise HomePageController.postAsync() to do the following when there are too many text form fields:

Log a warning.
Return the home page (with nothing in the text field).

3. Refactor your production code to make it cleaner.

The specifics are up to you.

Remember to commit your changes when you’re done.

API Documentation

const length = array.length;

Get the number of entries in an array.

returns length (number) - the number of entries

throw new Error(message);

Throw an exception.

message (string) - the error message

const message = error.message;

Get the error message from an Error exception.

returns message (string) - the error message

JavaScript Primers

No new concepts.

Hints

Implement the test:

The test is identical to the previous challenge, other than a few minor changes.

You can cut and paste the previous test. It’s okay to have some duplication in the tests if it makes them easier to read.

You’ll need to change the request body and the expected log message.

it("logs warning when duplicated form field found (and treats request like GET)", async () => {
	const { response, rot13Requests, logOutput } = await postAsync({ body: "text=one&text=two" });

	assert.deepEqual(logOutput.data, [{
		alert: "monitor",
		endpoint: "/",
		method: "POST",
		message: "form parse error",
		error: "should only be one 'text' form field",
		form: {
			text: [ "one", "two" ],
		},
	}], "should log a warning");

	assert.deepEqual(response, homePageView.homePage(), "should render home page");
	assert.deepEqual(rot13Requests.data, [], "shouldn't call ROT-13 service");
});

The test is ready to run.

When you run it, it should fail because there’s no log message.

This is because the production code isn’t checking the number of text fields.

Make the production code check the number of text fields.

This is just like the previous challenge, except you’re looking at the length of the array.

You can get the length of the array with textFields.length.

async postAsync(request, config) {
	const form = await request.readBodyAsUrlEncodedFormAsync();
	const textFields = form[INPUT_FIELD_NAME];
	if (textFields === undefined) {
		config.log.monitor({
			endpoint: ENDPOINT,
			method: "POST",
			message: "form parse error",
			error: `'${INPUT_FIELD_NAME}' form field not found`,
			form,
		});
		return homePageView.homePage();
	}
	if (textFields.length !== 1) {
		config.log.monitor({
			endpoint: ENDPOINT,
			method: "POST",
			message: "form parse error",
			error: `should only be one '${INPUT_FIELD_NAME}' form field`,
			form,
		});
		return homePageView.homePage();
	}

	const userInput = textFields[0];
	const output = await this._rot13Client.transformAsync(
		config.rot13ServicePort,
		userInput,
		config.correlationId,
	);
	return homePageView.homePage(output);
}

In TypeScript, this finally eliminates the need for the temporary workaround on userInput, but TypeScript still needs to be told it’s a string.

async postAsync(request: HttpServerRequest, config: WwwConfig): Promise<HttpServerResponse> {
	const form = await request.readBodyAsUrlEncodedFormAsync();
	const textFields = form[INPUT_FIELD_NAME];
	if (textFields === undefined) {
		config.log.monitor({
			endpoint: ENDPOINT,
			method: "POST",
			message: "form parse error",
			error: `'${INPUT_FIELD_NAME}' form field not found`,
			form,
		});
		return homePageView.homePage();
	}
	if (textFields.length !== 1) {
		config.log.monitor({
			endpoint: ENDPOINT,
			method: "POST",
			message: "form parse error",
			error: `should only be one '${INPUT_FIELD_NAME}' form field`,
			form,
		});
		return homePageView.homePage();
	}

	const userInput = textFields[0] as string;
	const output = await this._rot13Client.transformAsync(
		config.rot13ServicePort,
		userInput,
		config.correlationId,
	);
	return homePageView.homePage(output);
}

Refactor the production code:

There’s no one right answer for refactoring the production code. If you follow these hints, you’ll extract the parsing logic into a separate function.

The function will have this signature:

async function parseRequestBodyAsync(request, log) {...}

It will return userInput if the parsing was successful and null if it wasn’t.

Start by moving the early returns out of the code you’re going to extract.

Declare userInput at the top of the function, before the code you’re extracting. (The tests should pass.)

Put the userInput assignment in an else block, and change the second if to else if. (The tests should pass.)

Change the early returns into a userInput = null assignment, then check for null after the code you’re going to extract and return there instead. (The tests should pass.)

async postAsync(request, config) {
	let userInput;
	const form = await request.readBodyAsUrlEncodedFormAsync();
	const textFields = form[INPUT_FIELD_NAME];
	if (textFields === undefined) {
		config.log.monitor({
			endpoint: ENDPOINT,
			method: "POST",
			message: "form parse error",
			error: `'${INPUT_FIELD_NAME}' form field not found`,
			form,
		});
		userInput = null;
	}
	else if (textFields.length !== 1) {
		config.log.monitor({
			endpoint: ENDPOINT,
			method: "POST",
			message: "form parse error",
			error: `should only be one '${INPUT_FIELD_NAME}' form field`,
			form,
		});
		userInput = null;
	}
	else {
		userInput = textFields[0];  // add "as string" in TypeScript
	}

	if (userInput === null) return homePageView.homePage();

	const output = await this._rot13Client.transformAsync(
		config.rot13ServicePort,
		userInput,
		config.correlationId,
	);
	return homePageView.homePage(output);
}

The extracted function doesn’t need the entire config object. It only needs the logger.

Extract config.log into a variable and move it above the code you’re going to extract.

async postAsync(request, config) {
	const log = config.log;

	let userInput;
	const form = await request.readBodyAsUrlEncodedFormAsync();
	const textFields = form[INPUT_FIELD_NAME];
	if (textFields === undefined) {
		log.monitor({
			endpoint: ENDPOINT,
			method: "POST",
			message: "form parse error",
			error: `'${INPUT_FIELD_NAME}' form field not found`,
			form,
		});
		userInput = null;
	}
	else if (textFields.length !== 1) {
		log.monitor({
			endpoint: ENDPOINT,
			method: "POST",
			message: "form parse error",
			error: `should only be one '${INPUT_FIELD_NAME}' form field`,
			form,
		});
		userInput = null;
	}
	else {
		userInput = textFields[0];  // add "as string" in TypeScript
	}

	if (userInput === null) return homePageView.homePage();

	const output = await this._rot13Client.transformAsync(
		config.rot13ServicePort,
		userInput,
		config.correlationId,
	);
	return homePageView.homePage(output);
}

You’re ready to extract the function. Don’t forget to await it.

You can turn the userInput assignments back into early returns. (Your editor might do this for you.)

export class HomePageController {
	// ...

	async postAsync(request, config) {
		const log = config.log;
		let userInput = await parseRequestBodyAsync(request, log);
		if (userInput === null) return homePageView.homePage();

		const output = await this._rot13Client.transformAsync(
			config.rot13ServicePort,
			userInput,
			config.correlationId,
		);
		return homePageView.homePage(output);
	}

}

async function parseRequestBodyAsync(request, log) {
	const form = await request.readBodyAsUrlEncodedFormAsync();
	const textFields = form[INPUT_FIELD_NAME];
	if (textFields === undefined) {
		log.monitor({
			endpoint: ENDPOINT,
			method: "POST",
			message: "form parse error",
			error: `'${INPUT_FIELD_NAME}' form field not found`,
			form,
		});
		return null;
	}
	else if (textFields.length !== 1) {
		log.monitor({
			endpoint: ENDPOINT,
			method: "POST",
			message: "form parse error",
			error: `should only be one '${INPUT_FIELD_NAME}' form field`,
			form,
		});
		return null;
	}
	else {
		return textFields[0];
	}
}

TypeScript’s implementation of parseRequestBodyAsync() needs type annotations:

// TypeScript
async function parseRequestBodyAsync(request: HttpServerRequest, log: Log): Promise<string | null> {
	const form = await request.readBodyAsUrlEncodedFormAsync();
	const textFields = form[INPUT_FIELD_NAME];
	if (textFields === undefined) {
		log.monitor({
			endpoint: ENDPOINT,
			method: "POST",
			message: "form parse error",
			error: `'${INPUT_FIELD_NAME}' form field not found`,
			form,
		});
		return null;
	}
	else if (textFields.length !== 1) {
		log.monitor({
			endpoint: ENDPOINT,
			method: "POST",
			message: "form parse error",
			error: `should only be one '${INPUT_FIELD_NAME}' form field`,
			form,
		});
		return null;
	}
	else {
		return textFields[0] as string;
	}
}

You can simplify the parsing logic further by using exceptions.

Throw an Error in the if statements and log the warning in the exception handler.

async function parseRequestBodyAsync(request, log) {
	const form = await request.readBodyAsUrlEncodedFormAsync();
	const textFields = form[INPUT_FIELD_NAME];

	try {
		if (textFields === undefined) {
			throw new Error(`'${INPUT_FIELD_NAME}' form field not found`);
		}
		else if (textFields.length !== 1) {
			throw new Error(`should only be one '${INPUT_FIELD_NAME}' form field`);
		}
		else {
			return textFields[0];  // add "as string" in TypeScript
		}
	}
	catch (err) {
		log.monitor({
			endpoint: ENDPOINT,
			method: "POST",
			message: "form parse error",
			error: err.message,
			form,
		});
		return null;
	}
}

Finish by cleaning up. This is mainly a matter of taste, with no right answer.

export class HomePageController {
	// ...

	async postAsync(request, config) {
		const userInput = await parseRequestBodyAsync(request, config.log);
		if (userInput === null) return homePageView.homePage();

		const output = await this._rot13Client.transformAsync(
			config.rot13ServicePort,
			userInput,
			config.correlationId,
		);
		return homePageView.homePage(output);
	}

}

async function parseRequestBodyAsync(request, log) {
	const form = await request.readBodyAsUrlEncodedFormAsync();
	const textFields = form[INPUT_FIELD_NAME];

	try {
		if (textFields === undefined) throw new Error(`'${INPUT_FIELD_NAME}' form field not found`);
		if (textFields.length !== 1) throw new Error(`should only be one '${INPUT_FIELD_NAME}' form field`);

		return textFields[0];  // add "as string" in TypeScript
	}
	catch (err) {
		log.monitor({
			endpoint: ENDPOINT,
			method: "POST",
			message: "form parse error",
			error: err.message,
			form,
		});
		return null;
	}
}

Complete Solution

Test code:

it("logs warning when duplicated form field found (and treats request like GET)", async () => {
	const { response, rot13Requests, logOutput } = await postAsync({ body: "text=one&text=two" });

	assert.deepEqual(logOutput.data, [{
		alert: "monitor",
		endpoint: "/",
		method: "POST",
		message: "form parse error",
		error: "should only be one 'text' form field",
		form: {
			text: [ "one", "two" ],
		},
	}], "should log a warning");

	assert.deepEqual(response, homePageView.homePage(), "should render home page");
	assert.deepEqual(rot13Requests.data, [], "shouldn't call ROT-13 service");
});

Production code (JavaScript):

export class HomePageController {
	// ...

	async postAsync(request, config) {
		const userInput = await parseRequestBodyAsync(request, config.log);
		if (userInput === null) return homePageView.homePage();

		const output = await this._rot13Client.transformAsync(
			config.rot13ServicePort,
			userInput,
			config.correlationId,
		);
		return homePageView.homePage(output);
	}

}

async function parseRequestBodyAsync(request, log) {
	const form = await request.readBodyAsUrlEncodedFormAsync();
	const textFields = form[INPUT_FIELD_NAME];

	try {
		if (textFields === undefined) throw new Error(`'${INPUT_FIELD_NAME}' form field not found`);
		if (textFields.length !== 1) throw new Error(`should only be one '${INPUT_FIELD_NAME}' form field`);

		return textFields[0];
	}
	catch (err) {
		log.monitor({
			endpoint: ENDPOINT,
			method: "POST",
			message: "form parse error",
			error: err.message,
			form,
		});
		return null;
	}
}

Production code (TypeScript):

export class HomePageController {
	// ...

	async postAsync(request: HttpServerRequest, config: WwwConfig): Promise<HttpServerResponse> {
		const userInput = await parseRequestBodyAsync(request, config.log);
		if (userInput === null) return homePageView.homePage();

		const output = await this._rot13Client.transformAsync(
			config.rot13ServicePort,
			userInput,
			config.correlationId,
		);

		return homePageView.homePage(output);
	}

}

async function parseRequestBodyAsync(request: HttpServerRequest, log: Log): Promise<string | null> {
	const form = await request.readBodyAsUrlEncodedFormAsync();
	const textFields = form[INPUT_FIELD_NAME];

	try {
		if (textFields === undefined) throw new Error(`'${INPUT_FIELD_NAME}' form field not found`);
		if (textFields.length !== 1) throw new Error(`should only be one '${INPUT_FIELD_NAME}' form field`);

		return textFields[0] as string;
	}
	catch (err) {
		log.monitor({
			endpoint: ENDPOINT,
			method: "POST",
			message: "form parse error",
			error: err.message,
			form,
		});
		return null;
	}
}

Next challenge

Return to module overview