3
\$\begingroup\$

This is a JavaScript calculator that uses JQuery and bootstrap. How can I improve the CSS, HTML, JS and the general design?

/// <reference path="jquery-1.9.1.js" />
var firstnum = '';
var first = true;
var secondnum = '';

// Resets firstnum, secondnum and first
function reset() {
  firstnum = '';
  secondnum = '';
  first = true;
}

// Sums up firstnum and secondnum
function sum() {
  return parseInt(firstnum) + parseInt(secondnum);
}

// Moves the content of the buttons to the screen
$('.col-sm-3').click(function typing() {
  if (first) {
    firstnum += $(this).text();
    $('.screen').text(firstnum);
  } else {
    secondnum += $(this).text();
    $('.screen').text(secondnum);
  }
})

// Resets the calculator
$('#ac').click(function ac() {
  reset();
  $('.screen').empty();
})

// Deletes the last character of the screen text
$('#del').click(function del() {
  lasttext = $('.screen').text();
  // This condition happens when the user clicks 'del' after clicking '='
  if (lasttext === 'del') {
    $('.screen').empty();
  } else {
    $('.screen').text(lasttext.replace(lasttext.substr(lasttext.length - 4, lasttext.length /* - 1*/ ), '')) // We used a substr here since "del" is written on the screen
  }
  if (first) {
    firstnum = $('.screen').text();
  } else {
    secondnum = $('.screen').text();
  }
})

// Switches typing to secondnum
$('#add').click(function add() {
  if (first) {
    firstnum = $('.screen').text(); // This is to ensure that the user can add again after pressing '='
  }
  // If the user is trying to add more than two numbers
  else {
    firstnum = sum().toString();
    secondnum = '';
  }
  $('.screen').text('+')
  first = false;
})

// Shows the final result or firstnumber if there wasn't an operation
$('#equal').click(function result() {
  $('.screen').text(sum().toString());
  if (isNaN($('.screen').text())) {
    $('.screen').text(firstnum);
  }
  reset();
})
.screen {
  background: black;
  color: #ffef9e;
  font-size: 25px;
  width: 100%;
  height: 100px;
  text-align: left;
  word-wrap: break-word;
  overflow: auto
}

.container {
  border: 3px solid black;
  padding: 0%;
  margin-top: 1%;
  box-shadow: 10px 10px 85px 12px black;
}

.col-sm-3,
.col-sm-1 {
  padding: 3%;
  text-align: center;
  box-sizing: border-box;
  border: 3px solid black;
  margin-left: 3%;
  margin-top: 0.5%;
  margin-bottom: 0.5%;
  background: #ffc79e63;
  box-shadow: 10px 10px 67px -9px rgba(0, 0, 0, 0.75);
  font-size: 25px;
  user-select: none;
}

.col-sm-1 {
  float: left;
}

.col-sm-3:hover,
.col-sm-1:hover {
  box-shadow: none;
  background: #ffc79e;
  cursor: pointer;
}

.col-sm-3:active,
.col-sm-1:active {
  background: #ffef9e;
}

@media only screen and (max-width: 780px) {
  .col-sm-3 {
    width: 25%;
    float: left;
  }
  .col-sm-1 {
    width: 10%;
    float: left;
  }
  .col-sm-3:hover,
  .col-sm-1:hover {
    box-shadow: 10px 10px 67px -9px rgba(0, 0, 0, 0.75);
  }
}

@media only screen and (max-width: 300px) {
  .col-sm-3,
  .col-sm-1 {
    font-size: 18px;
  }
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.js"></script>
<script src="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/js/bootstrap.min.js"></script>
<link href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap.min.css" rel="stylesheet"/>

<!DOCTYPE html>
<html>

<head>
  <meta charset="utf-8" />
  <title>Calculator</title>
  <script src="../scripts/jquery-1.9.1.js"></script>
  <script src="../scripts/bootstrap.js"></script>
  <link href="../Content/bootstrap.css" rel="stylesheet" />
  <link href="../CSS/Calculator.css" rel="stylesheet" />
</head>

<body>
  <div class="container">
    <div class="screen">
    </div>
    <div class="col-sm-3">1</div>
    <div class="col-sm-3">2</div>
    <div class="col-sm-3">3</div>
    <div class="col-sm-1" id="add">+</div>
    <div class="col-sm-3">4</div>
    <div class="col-sm-3">5</div>
    <div class="col-sm-3">6</div>
    <div class="col-sm-1" id="equal">=</div>
    <div class="col-sm-3">7</div>
    <div class="col-sm-3">8</div>
    <div class="col-sm-3">9</div>
    <div class="col-sm-3">0</div>
    <div class="col-sm-3" id="del">del</div>
    <div class="col-sm-3" id="ac">AC</div>
  </div>
  <script src="../scripts/Calculator.js"></script>
</body>

</html>

\$\endgroup\$
4
\$\begingroup\$

From the perspective of using Bootstrap, you want to refrain from modifying the standard Bootstrap classes. Modifying classes such as col-sm-6 causes many issues down the line. Always leave that standard framework classes alone and add additional classes yourself.

In Bootstrap 3.7 you have a col-xs- class that you should be using from mobile devices up. col-sm- is for 768px and up. Build your columns up from a mobile-first perspective.

https://getbootstrap.com/docs/3.3/css/#grid-options

Containers must be followed by <div class="row"></div>, then your columns are placed within that row. This ensures the margin and padding system that Bootstrap has in place marries up correctly.

In general, you seem to be using the Bootstrap grid framework incorrectly. You may want to look at some of the examples again.

The columns are there as an aid to design. If you are not going to use them you should probably refrain from using the grid framework and develop it from scratch. Also, each column row should total a maximum of 12 columns. If you have more than 12 columns within a row it will spill onto the next line.

Refer to the Bootstrap documentation for a thorough explanation.

https://getbootstrap.com/docs/3.3/css/#grid-intro

You seem to be using margins incorrectly, too. Margins can affect the flow of the document. Particularly in Bootstrap. If you do require a white padding area around each button I would personally use a containing div with padding (others may do it differently), but again refer to the documentation on the CSS box model to gain a better understanding of how elements are rendered. There are many ways to skin a cat in CSS, but a good way to start is to always vision what you are trying to achieve either in your mind or on paper using the box model.

https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Box_Model/Introduction_to_the_CSS_box_model

You could use the Bootstrap btn class for your buttons, as well as many other areas where the framework can do the work for you. But I am not going to put this in for you.

To be honest I think it could be structured in a better way but without completely re-writing what you have here I thought it would be better to point out areas where you will gain the most knowledge.

Bootstrap is a 200kb+ resource so if you are going to use it, use it effectively.

Using Bootstrap and my understanding of the framework, this is how I would lay out the HTML and CSS so it is similar in structure to what you have.

I have not touched the jQuery and would be interested to see someone with more experience make improvements.

/// <reference path="jquery-1.9.1.js" />
var firstnum = '';
var first = true;
var secondnum = '';

// Resets firstnum, secondnum and first
function reset() {
  firstnum = '';
  secondnum = '';
  first = true;
}

// Sums up firstnum and secondnum
function sum() {
  return parseInt(firstnum) + parseInt(secondnum);
}

// Moves the content of the buttons to the screen
$('.calc-button-number').click(function typing() {
  if (first) {
    firstnum += $(this).text();
    $('.screen').text(firstnum);
  } else {
    secondnum += $(this).text();
    $('.screen').text(secondnum);
  }
})

// Resets the calculator
$('#ac').click(function ac() {
  reset();
  $('.screen').empty();
})

// Deletes the last character of the screen text
$('#del').click(function del() {
  lasttext = $('.screen').text();
  // This condition happens when the user clicks 'del' after clicking '='
  if (lasttext === 'del') {
    $('.screen').empty();
  } else {
    $('.screen').text(lasttext.replace(lasttext.substr(lasttext.length - 4, lasttext.length /* - 1*/ ), '')) // We used a substr here since "del" is written on the screen
  }
  if (first) {
    firstnum = $('.screen').text();
  } else {
    secondnum = $('.screen').text();
  }
})

// Switches typing to secondnum
$('#add').click(function add() {
  if (first) {
    firstnum = $('.screen').text(); // This is to ensure that the user can add again after pressing '='
  }
  // If the user is trying to add more than two numbers
  else {
    firstnum = sum().toString();
    secondnum = '';
  }
  $('.screen').text('+')
  first = false;
})

// Shows the final result or firstnumber if there wasn't an operation
$('#equal').click(function result() {
  $('.screen').text(sum().toString());
  if (isNaN($('.screen').text())) {
    $('.screen').text(firstnum);
  }
  reset();
})
body {
  padding-top: 1%;
}

.screen {
  background: #000;
  color: #ffef9e;
  font-size: 25px;
  height: 100px;
  word-wrap: break-word;
  overflow: auto
}

.calc-container {
  border: 3px solid #000;
  box-shadow: 10px 10px 85px 12px #000;
}

.calc-button {
  padding: 3%;
  border: 3px solid black;
  background: rgba(255,199,158,0.3);
  box-shadow: 10px 10px 67px -9px rgba(0, 0, 0, 0.75);
  font-size: 25px;
  user-select: none;
}

.calc-button:hover,
.calc-button:hover {
  box-shadow: none;
  background: #ffc79e;
  cursor: pointer;
}

.calc-button:active,
.calc-button:active {
  background: #ffef9e;
}

@media only screen and (max-width: 780px) {
  .calc-button:hover,
  .calc-button:hover {
    box-shadow: 10px 10px 67px -9px rgba(0, 0, 0, 0.75);
  }
}

@media only screen and (max-width: 300px) {
  .calc-button,
  .calc-button {
    font-size: 18px;
  }
}
<!DOCTYPE html>
<html>

<head>
  <meta charset="utf-8" />
  <title>Calculator</title>
  <!-- Latest compiled and minified CSS -->
  <link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap.min.css" integrity="sha384-BVYiiSIFeK1dGmJRAkycuHAHRg32OmUcww7on3RYdg4Va+PmSTsz/K68vbdEjh4u" crossorigin="anonymous">
  <link href="../CSS/Calculator.css" rel="stylesheet" />
</head>

<body>
    <div class="container calc-container">
      <div class="row">
        <div class="col-xs-12 screen"></div>
      </div>
      <div class="row">
        <div class="col-xs-3 calc-button calc-button-number text-center">1</div>
        <div class="col-xs-3 calc-button calc-button-number text-center">2</div>
        <div class="col-xs-3 calc-button calc-button-number text-center">3</div>
        <div class="col-xs-3 calc-button text-center" id="add">+</div>
      </div>
      <div class="row">
        <div class="col-xs-3 calc-button calc-button-number text-center">4</div>
        <div class="col-xs-3 calc-button calc-button-number text-center">5</div>
        <div class="col-xs-3 calc-button calc-button-number text-center">6</div>
        <div class="col-xs-3 calc-button text-center" id="equal">=</div>
      </div>
      <div class="row">
        <div class="col-xs-3 calc-button calc-button-number text-center">7</div>
        <div class="col-xs-3 calc-button calc-button-number text-center">8</div>
        <div class="col-xs-3 calc-button calc-button-number text-center">9</div>
      </div>
      <div class="row">
        <div class="col-xs-3 calc-button calc-button-number text-center">0</div>
        <div class="col-xs-3 calc-button calc-button-number text-center" id="del">del</div>
        <div class="col-xs-3 calc-button calc-button-number text-center" id="ac">AC</div>
      </div> 
    </div>
    <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.12.4/jquery.min.js"></script>
    <!-- Latest compiled and minified JavaScript -->
    <script src="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/js/bootstrap.min.js" integrity="sha384-Tc5IQib027qvyjSMfHjOMaLkfuWVxZxUPnCJA7l2mCWNIpG9mGCD8wGNIcPD7Txa" crossorigin="anonymous"></script>
  <script src="../scripts/Calculator.js"></script>
</body>

</html>

\$\endgroup\$
3
\$\begingroup\$

WRT your JavaScript code, I don't like the firstnum, secondnum, and first variables. I believe they cripple your code into a series of corner cases that make it difficult to read, and possibly further modify.

You already allow the user to sum more than two numbers consecutively. I would then suggest using an array to store the numbers, and reducing the array to a single number when the result of the sum of those numbers is actually requested. You could also store the final sum in a new array (replacing the old one) so that it is the first number to use for the next operation.

WRT jQuery, I would at least try to extract frequently accessed "elements" (e.g. $('.screen')) into their own global variable (e.g. $screen, with the $ prefix, so as to make it clear that it contains a jQuery collection).

\$\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.