4
\$\begingroup\$

I created a program that calculates the area of a room. And I added the form after first calculation which automatically updates the values when any value changes. But the code for this form duplicates some of the functions. I will be happy for the tip on improving the code.

$(function(){
	$('.card').hide();
	const e = 0.09290304;
	var newLength = function(a){return $('#lengthChange').val(a)};
	var newWidth = function(a){return $('#widthChange').val(a)};
	var lengthA = "";
	var widthA = "";
	var areaFeet ="";
	var convert = function(a){return parseFloat(a.val());};
	var inFeet = function(a, b){return a * b};
	var inMeters = function(a){return Math.pow(a, 2) * e};
	$('#inputBtn').click(function(){
		$('#startForm').hide();
		$('.card').show();
		lengthA = convert($('#lengthInput'));
		widthA = convert($('#widthInput'));
		newLength(lengthA);
		newWidth(widthA);
		areaFeet = inFeet(lengthA, widthA);
		$('#squareFeet').text(areaFeet);
		$('#squareMeters').text(inMeters(areaFeet));
		$(this).hide();
	});
	var changeArea = function(){
		lengthA = convert($('#lengthChange'));
		widthA = convert($('#widthChange'));
		areaFeet = inFeet(lengthA, widthA);
		$('#squareFeet').text(areaFeet);
		$('#squareMeters').text(inMeters(areaFeet));
	};

	 $('#lengthChange').on('input', function(){
	 	return changeArea();
	  });
	 $('#widthChange').on('input', function(){
	 	return changeArea();
	  });
	
});
.input{width: 80px; padding-left: 5px}
.card{width: 600px}
<link href="https://fonts.googleapis.com/css?family=Open+Sans:400,700" rel="stylesheet">
	<link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/4.0.0-alpha.6/css/bootstrap.min.css" integrity="sha384-rwoIResjU2yc3z8GV/NPeZWAv56rSmLldC3R/AZzGRnGxQQKnKkoFVhFQhNUwEyJ" crossorigin="anonymous">
	<script src="https://code.jquery.com/jquery-3.1.1.slim.min.js" integrity="sha384-A7FZj7v+d/sdmMqp/nOQwliLvUsJfDHW+k9Omg/a/EheAdgtzNs3hpfag6Ed950n" crossorigin="anonymous"></script>
	<script src="https://cdnjs.cloudflare.com/ajax/libs/tether/1.4.0/js/tether.min.js" integrity="sha384-DztdAPBWPRXSA/3eYEEUWrWCy7G5KFbe8fFjk5JAIxUYHKkDx6Qin1DkWx51bBrb" crossorigin="anonymous"></script>
	<script src="https://maxcdn.bootstrapcdn.com/bootstrap/4.0.0-alpha.6/js/bootstrap.min.js" integrity="sha384-vBWWzlZJ8ea9aCX4pEW3rVHjgjt7zpkNpZk+02D9phzyeVkE+jo0ieGizqPLForn" crossorigin="anonymous"></script>
<div class="container mt-5 text-center">
	<h2 class="mb-4">Area of a Rectangular Room</h2>
	<form id="startForm">
		<div class="form-group">
			<label>What is the length of the room in feet?</label>
			<input type="number" class="input" id="lengthInput" name="" step="any" min="0" oninput="validity.valid||(value='');">
		</div>
		<div class="form-group">
			<label>What is the width of the room in feet?</label>
			<input type="number" class="input" id="widthInput" name="" step="any" min="0" oninput="validity.valid||(value='');">
		</div>	
	</form>
	<button class="btn btn-primary" id="inputBtn">Quick Calculations</button>
	<div class="card text-center mx-auto">
		<div class="card-header">
			<form class="inline-form">
				<label class="mr-2 font-weight-bold">Length:</label>
				<input type="number" class="input" id="lengthChange" value="" name="" step="any" min="0" oninput="validity.valid||(value='');">
				<span class="ml-2">feet</span>
				<span class="ml-2 mr-2 font-weight-bold">X</span>
				<label class="mr-2 font-weight-bold">Width:</label>
				<input type="number" class="input" id="widthChange" name="" step="any" min="0" oninput="validity.valid||(value='');">
				<span class="ml-2">feet</span>
			</form>
		</div>
		<div class="card-body text-left mt-3">
			<dl class="row">
				<dt class="col-sm-5 pl-5">The area is</dt>
				<dd class="col-sm-7"><span id="squareFeet" class="font-weight-bold"></span> square feet</dd>
                <dd class="col-sm-7 offset-sm-5"><span id="squareMeters" class="font-weight-bold"></span> square meters</dd>
			</dl>
		</div>
	</div>
</div>

\$\endgroup\$
  • \$\begingroup\$ which formula do you use? \$\endgroup\$ – kharandziuk Mar 2 '18 at 11:35
  • \$\begingroup\$ ????100 square feet = 929.0304 square meters, I THINK NOT!! One square foot = ~0.1 square meters ( 0.092903 square meters per square foot) and you MUST not increase precision when converting measurements. The function inMeters = function(a){return Math.pow(a, 2) * e}; should be inMeters = sqrF => Number((sqrF * e).toFixed(1)); \$\endgroup\$ – Blindman67 Mar 2 '18 at 22:38
0
\$\begingroup\$

The UI looks very pleasant, and I was surprised that it was only so few lines of code. Well done.

When I first tried it, I was a confused when the UI changed radically after clicking the "Quick Calculations" button. What about removing the first part of the UI altogether?

const e = 0.09290304;

The name e is bad because it doesn't give any clue about what the number means. A better name would be squareMeterPerSquareFoot.

var newLength = function(a){return $('#lengthChange').val(a)};
var newWidth = function(a){return $('#widthChange').val(a)};

You don't need these two functions since you never use their return values.

var lengthA = "";
var widthA = "";
var areaFeet ="";

These variables should not be declared here since they are not used in this outer function. They are only used in the inner functions below, and there they should be defined using const instead of var. This tells the human reader that they won't be modified.

var convert = function(a){return parseFloat(a.val());};

The name of this function is a little too broad. Sure, it converts something, but when this function is called as output = convert(input), I have no idea what happens here. A better name is floatValue.

var inFeet = function(a, b){return a * b};

This function has nothing to do with feet. It should be called area. Maybe even rectArea, but since this program is only concerned about rectangles that might be too long.

var inMeters = function(a){return Math.pow(a, 2) * e};

This function must not square its argument. The parameter a has an awful name. To avoid bugs in your program, all variables should include the measurement unit. Instead of a, a better name is lengthInFeet. This makes the bug obvious since later you call this function with areaFeed, which obviously has a measurement unit different from lengthInFeet.

var changeArea = function(){
    lengthA = convert($('#lengthChange'));
    widthA = convert($('#widthChange'));
    areaFeet = inFeet(lengthA, widthA);
    $('#squareFeet').text(areaFeet);
    $('#squareMeters').text(inMeters(areaFeet));
};

So the area in meters is areaFeet * areaFeet * e? That's a bug.

Instead of var changeArea = function() { ... };, you should write function changeArea() { ... }. This is shorter and saves a var and a semicolon. It also captures the intention more clearly since you are defining a function, not a variable here.

 $('#lengthChange').on('input', function(){
    return changeArea();
  });
 $('#widthChange').on('input', function(){
    return changeArea();
  });

As Occam's Razor already mentioned, when you call a function without parameters in a callback like these, you can pass the function directly:

 $('#widthChange').on('input', changeArea);
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ "only so few lines of code." That is if you don't count the additional 140K of minified JavaScript and 150K of minified CSS it requires to make an area calculation look good. \$\endgroup\$ – Blindman67 Mar 3 '18 at 11:19
3
\$\begingroup\$

These one-liner functions are a great opportunity to use the more concise arrow functions.

so this..

var newLength = function(a){return $('#lengthChange').val(a)};

becomes this...

var newLength = a=>$('#lengthChange').val(a);

Some people would disagree with me, but I personally prefer to define my vars all at once, and to keep the functions separate from the variables. Also, I would use const for the functions.

var lengthA = "", 
    widthA = "",
    areaFeet ="",
const newLength = a=>$('#lengthChange').val(a);
const newWidth = a=>$('#widthChange').val(a);
const convert = a=>parseFloat(a.val());
const inFeet = (a, b)=>a * b;
const inMeters = a=>Math.pow(a, 2) * e;

There's really no reason to have an anonymous function that does nothing but call a named function.

this...

$('#widthChange').on('input', function(){
    return changeArea();
});

might as well just be...

$('#widthChange').on('input', changeArea);

Aside from that, well done. It looks nice.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Be careful though when replacing function() { return changeArea() } with a simple changeArea. In the latter case, the function is called with an argument of type Event. \$\endgroup\$ – Roland Illig Mar 2 '18 at 18:29
  • \$\begingroup\$ @RolandIllig - Why does that matter when the function doesn't use any arguments? \$\endgroup\$ – I wrestled a bear once. Mar 2 '18 at 19:03
  • \$\begingroup\$ For functions having no arguments it doesn't matter. But in any other case the behavior changes. Therefore this restriction should be mentioned, I think. \$\endgroup\$ – Roland Illig Mar 3 '18 at 8:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.