5
\$\begingroup\$

I wrote a program to evenly divide pizzas. And I tried to stylize it as a console. I will be grateful for tips on improving my code.

Here is a sample image of the program after a user has entered values: sample image of output

$(function(){
	$('.secondQuestion').hide();
	$('#peopleInput').focus();
			
	//Three functions for outputting the result
	const firstOutput = function(a, b){
		if(b>1){
			$('#firstStr').text(a + " people with " + b + " pizzas")
		} else {
			$('#firstStr').text(a + " people with " + b + " pizza")
		}
	} 
	const piece = (a, b) => Math.floor((b * 8) / a);
	const secondOutput = function(a){
		if(a > 1){
			$('#secondStr').text("Each person gets " + a + " pieces of pizza")
		} else {
			$('#secondStr').text("Each person gets " + a + " piece of pizza")
		}
	} 
	const leftover = (a, b) => (b * 8) % a; 
	const thirdOutput = function(a){
		if(a > 1){
			$('#thirdStr').text("There are " + a + " leftover pieces")
		} else {
			$('#thirdStr').text("There is " + a + " leftover pieces")
		}
	}

	//Action after pressing the enter key on the first line
	$('#peopleInput').bind("enterKey", function(e){
		$('.secondQuestion').show();
		$('#pizzaInput').focus();
	});
	$('#peopleInput').keyup(function(e){
		if(e.keyCode == 13){$(this).trigger("enterKey")}
	});

	//Action after pressing the enter key on the second line
	$('#pizzaInput').bind("enterKey", function(e){
		$('#peopleInput').focus();
		const people = $('#peopleInput').val();
		const pizza = $('#pizzaInput').val();
		firstOutput(people, pizza);
		setTimeout(function(){secondOutput(piece(people, pizza)); }, 300);
		setTimeout(function(){thirdOutput(leftover(people, pizza)); }, 600);
	});
	$('#pizzaInput').keyup(function(e){
		if(e.keyCode == 13){$(this).trigger("enterKey")}
	});
});
body{
	background: #000;
	color: #fff;
	font-family: 'Inconsolata', monospace;
}

h2{
	font-weight: normal;
}

.output{
	margin-top: 10px;
}

p{
	margin: 0 0 5px 0;
}
input {
	border: none;
    background-color: #000;
    width: 50px;
    color: #fff;
    padding-left: 5px;
    margin-bottom: 5px;
}
input:focus{
	outline-color: #000;
}

main{
	position: absolute;
    left: 200px;
}
input::-webkit-outer-spin-button,
input::-webkit-inner-spin-button {
    -webkit-appearance: none;
    margin: 0;
}
<link href="https://fonts.googleapis.com/css?family=Inconsolata:400,700" rel="stylesheet">
<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<body>
<header>
	<h2>Pizza Party ></h2>
</header>
<main>
	<div class="firstQuestion">
		<label>How many people?</label>
		<input type="number" class="form-control" id="peopleInput" value="" name="" min="0" oninput="validity.valid||(value='');">
	</div>
	<div class="secondQuestion">
		<label >How many pizzas do you have?</label>
		<input type="number" class="form-control" id="pizzaInput" value="" name="" min="0" oninput="validity.valid||(value='');">
	</div>
	<div class="output">
		<p id="firstStr"></p>
		<p id="secondStr"></p>
		<p id="thirdStr"></p>
	</div>
</main>	
</body>

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Feedback

I like the style, timing of events, and how the number input spinners are hidden. It really looks like a console application! Plus, pizza is my favorite food!*

Suggestions

Cache DOM references

As was mentioned in answers to your other question (like the retirement calculator), any DOM element lookups that happen multiple time should be reduced to a single lookup that is stored in a variable (or a constant using const). So in your code, it would be wise to cache $('#peopleInput');, $('#pizzaInput'), etc...

$(function(){
    $('.secondQuestion').hide();
    const peopleInput = $('#peopleInput');
    peopleInput.focus();
    const pizzaInput = $('#pizzaInput');

Don't repeat yourself

This principle comes to play here with the following blocks:

$('#peopleInput').keyup(function(e){
    if(e.keyCode == 13){$(this).trigger("enterKey")}
});

and

$('#pizzaInput').keyup(function(e){
    if(e.keyCode == 13){$(this).trigger("enterKey")}
});

These could be combined to a single CSS Selector:

$('#peopleInput, #pizzaInput').keyup(function(e){
    if(e.keyCode == 13){$(this).trigger("enterKey")}
});

Since those are the only input elements on the page, it could be simplified to simply $('input'), but that might not be a good habit to get into, especially if you go on to larger applications.

Naming

Some of the names are misleading. For example: piece is actually a function that returns the number of pieces, so something like pieceDivision or getNumerOfPieces would be more appropriate.

Templating

Like I mentioned in my answer to your question about the retirement calculator, using templates might allow the code that sets the text to be simplified...

Altered Code

With the suggestions above, the code could be modified to be more like this:

$(function() {
  $('.secondQuestion').hide();
  const peopleInput = $('#peopleInput');
  peopleInput.focus(); //$('#peopleInput').focus();
  const pizzaInput = $('#pizzaInput');

  //Three functions for outputting the result
  const firstOutput = function(a, b) {
    if (b > 1) {
      $('#firstStr').text(a + " people with " + b + " pizzas")
    } else {
      $('#firstStr').text(a + " people with " + b + " pizza")
    }
  }
  const getNumberOfPiecesPerPerson = (a, b) => Math.floor((b * 8) / a);
  const secondOutput = function(a) {
    if (a > 1) {
      $('#secondStr').text("Each person gets " + a + " pieces of pizza")
    } else {
      $('#secondStr').text("Each person gets " + a + " piece of pizza")
    }
  }
  const getNumberOfLeftoverPieces = (a, b) => (b * 8) % a;
  const thirdOutput = function(a) {
    if (a > 1) {
      $('#thirdStr').text("There are " + a + " leftover pieces")
    } else {
      $('#thirdStr').text("There is " + a + " leftover pieces")
    }
  }

  //Action after pressing the enter key on the first line
  peopleInput.bind("enterKey", function(e) {
    $('.secondQuestion').show();
    pizzaInput.focus();
  });
  //combine $('#peopleInput') and $('#pizzaInput') - could also be $('#peopleInput, #pizzaInput')
  $('input').keyup(function(e) {
    if (e.keyCode == 13) {
      $(this).trigger("enterKey")
    }
  });

  //Action after pressing the enter key on the second line
  pizzaInput.bind("enterKey", function(e) {
    peopleInput.focus();
    const numPeople = peopleInput.val();
    const numPizzas = pizzaInput.val();
    firstOutput(numPeople, numPizzas);
    setTimeout(function() {
      secondOutput(getNumberOfPiecesPerPerson(numPeople, numPizzas));
    }, 300);
    setTimeout(function() {
      thirdOutput(getNumberOfLeftoverPieces(numPeople, numPizzas));
    }, 600);
  });
});
body {
  background: #000;
  color: #fff;
  font-family: 'Inconsolata', monospace;
}

h2 {
  font-weight: normal;
}

.output {
  margin-top: 10px;
}

p {
  margin: 0 0 5px 0;
}

input {
  border: none;
  background-color: #000;
  width: 50px;
  color: #fff;
  padding-left: 5px;
  margin-bottom: 5px;
}

input:focus {
  outline-color: #000;
}

main {
  position: absolute;
  left: 200px;
}

input::-webkit-outer-spin-button,
input::-webkit-inner-spin-button {
  -webkit-appearance: none;
  margin: 0;
}
<link href="https://fonts.googleapis.com/css?family=Inconsolata:400,700" rel="stylesheet">
<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>

<body>
  <header>
    <h2>Pizza Party ></h2>
  </header>
  <main>
    <div class="firstQuestion">
      <label>How many people?</label>
      <input type="number" class="form-control" id="peopleInput" value="" name="" min="0" oninput="validity.valid||(value='');">
    </div>
    <div class="secondQuestion">
      <label>How many pizzas do you have?</label>
      <input type="number" class="form-control" id="pizzaInput" value="" name="" min="0" oninput="validity.valid||(value='');">
    </div>
    <div class="output">
      <p id="firstStr"></p>
      <p id="secondStr"></p>
      <p id="thirdStr"></p>
    </div>
  </main>
</body>

*not that it means the code is worth more, but at least it is interesting!

\$\endgroup\$

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.