3
\$\begingroup\$

Restaurant Menu It works how I want it to, but the way I've written the jQuery looks like it could be simplified. How can I rewrite the jQuery so it does exactly the same thing but in much less code? I need only the first square of JavaScript rest is only an understanding of the code 

$(document).ready(function() {
  //Variables
  var selectedStarter = {
    dish: "(None)",
    price: 0
  };
  var selectedMain = {
    dish: "(None)",
    price: 0
  };
  var selectedDessert = {
    dish: "(None)",
    price: 0
  };
  var starter = {
    firstDish: "Salad",
    firstDishPrice: 15,
    secondDish: "Soup",
    secondDishPrice: 7,
    thirdDish: "Fish rolls",
    thirdDishPrice: 12
  };

  var main = {
    firstDish: "Steak",
    firstDishPrice: 17,
    secondDish: "Salmon",
    secondDishPrice: 12,
    thirdDish: "Rissotto",
    thirdDishPrice: 9
  };

  var dessert = {
    firstDish: "Sorbet",
    firstDishPrice: 4,
    secondDish: "Fruit salad",
    secondDishPrice: 6,
    thirdDish: "Apple pie",
    thirdDishPrice: 5
  };

  function total() {
    return selectedStarter.price + selectedMain.price + selectedDessert.price;
  }

  function selectedStarterFnc(dish, price) {
    selectedStarter.price = price;
    selectedStarter.dish = dish;
    $("#total").html(total());
    return dish + "(" + price + ")";
  }

  function selectedMainFnc(dish, price) {
    selectedMain.price = price;
    selectedMain.dish = dish;
    $("#total").html(total());
    return dish + "(" + price + ")";
  }

  function selectedDessertFnc(dish, price) {
    selectedDessert.price = price;
    selectedDessert.dish = dish;
    $("#total").html(total());
    return dish + "(" + price + ")";
  }

  // Instantiating HTML Button Elements
  // Starter Elements
  document.getElementById("btStarter1").value =
    starter.firstDish + ": " + starter.firstDishPrice;

  document.getElementById("btStarter2").value =
    starter.secondDish + ": " + starter.secondDishPrice;

  document.getElementById("btStarter3").value =
    starter.thirdDish + ": " + starter.thirdDishPrice;

  // Main Elements
  document.getElementById("btMain1").value =
    main.firstDish + ": " + main.firstDishPrice;

  document.getElementById("btMain2").value =
    main.secondDish + ": " + main.secondDishPrice;

  document.getElementById("btMain3").value =
    main.thirdDish + ": " + main.thirdDishPrice;

  // Dessert Elements
  document.getElementById("btDessert1").value =
    dessert.firstDish + ": " + dessert.firstDishPrice;

  document.getElementById("btDessert2").value =
    dessert.secondDish + ": " + dessert.secondDishPrice;

  document.getElementById("btDessert3").value =
    dessert.thirdDish + ": " + dessert.thirdDishPrice;

  // Your Order: Elements
  document.getElementById("selectedStarter").innerHTML =
    selectedStarter.dish + " (" + selectedStarter.price + ")";

  document.getElementById("selectedMain").innerHTML =
    selectedMain.dish + " (" + selectedMain.price + ")";

  document.getElementById("selectedDessert").innerHTML =
    selectedDessert.dish + " (" + selectedDessert.price + ")";

  // Functions (JQuery)
  // Main menu onClicks handler
  $("#btMenu").click(function() {
    $("#liMainMenu").toggle("slow");
  });

  $("#btStarter").click(function() {
    $("#liStarter").toggle("slow", function() {
      if ($(this).css("display") == "none") {
        $("#btStarter").css("background-color", "#008080");
      } else {
        $("#btStarter").css("background-color", "red");
      }
    });
  });

  $("#btMain").click(function() {
    $("#liMain").toggle("slow", function() {
      if ($(this).css("display") == "none") {
        $("#btMain").css("background-color", "#008080");
      } else {
        $("#btMain").css("background-color", "red");
      }
    });
  });

  $("#btDessert").click(function() {
    $("#liDessert").toggle("slow", function() {
      if ($(this).css("display") == "none") {
        $("#btDessert").css("background-color", "#008080");
      } else {
        $("#btDessert").css("background-color", "red");
      }
    });
  });

  // Starter onClicks
  $("#btStarter1").click(function() {
    $("#liStarter").children("li").children("input").css("background-color", "rgb(0, 230, 40)");
    $(this).css("background-color", "red");
    $("#selectedStarter").html(selectedStarterFnc(starter.firstDish, starter.firstDishPrice));
  });

  $("#btStarter2").click(function() {
    $("#liStarter").children("li").children("input").css("background-color", "rgb(0, 230, 40)");
    $(this).css("background-color", "red");
    $("#selectedStarter").html(selectedStarterFnc(starter.secondDish, starter.secondDishPrice));
  });

  $("#btStarter3").click(function() {
    $("#liStarter").children("li").children("input").css("background-color", "rgb(0, 230, 40)");
    $(this).css("background-color", "red");
    $("#selectedStarter").html(selectedStarterFnc(starter.thirdDish, starter.thirdDishPrice));
  });

  // Main onClicks
  $("#btMain1").click(function() {
    $("#liMain").children("li").children("input").css("background-color", "rgb(0, 230, 40)");
    $(this).css("background-color", "red");
    $("#selectedMain").html(selectedMainFnc(main.firstDish, main.firstDishPrice));
  });

  $("#btMain2").click(function() {
    $("#liMain").children("li").children("input").css("background-color", "rgb(0, 230, 40)");
    $(this).css("background-color", "red");
    $("#selectedMain").html(selectedMainFnc(main.secondDish, main.secondDishPrice));
  });

  $("#btMain3").click(function() {
    $("#liMain").children("li").children("input").css("background-color", "rgb(0, 230, 40)");
    $(this).css("background-color", "red");
    $("#selectedMain").html(selectedMainFnc(main.thirdDish, main.thirdDishPrice));
  });

  // Dessert onClicks
  $("#btDessert1").click(function() {
    $("#liDessert").children("li").children("input").css("background-color", "rgb(0, 230, 40)");
    $(this).css("background-color", "red");
    $("#selectedDessert").html(selectedDessertFnc(dessert.firstDish, dessert.firstDishPrice));
  });

  $("#btDessert2").click(function() {
    $("#liDessert").children("li").children("input").css("background-color", "rgb(0, 230, 40)");
    $(this).css("background-color", "red");
    $("#selectedDessert").html(selectedDessertFnc(dessert.secondDish, dessert.secondDishPrice));
  });

  $("#btDessert3").click(function() {
    $("#liDessert").children("li").children("input").css("background-color", "rgb(0, 230, 40)");
    $(this).css("background-color", "red");
    $("#selectedDessert").html(selectedDessertFnc(dessert.thirdDish, dessert.thirdDishPrice));
  });
});
.button {
  background: rgb(0, 230, 40);
  outline: solid 2px #353535;
  border: solid 2px white;
  color: white;
  padding: 10px 15px;
  text-decoration: none;
  width: 200px;
  margin-top: 50px;
}

.ul li {
  display: inline;
  margin-right: 8px;
}
.ul {
  display: none;
}
.table {
  margin-top: 50px;
  border: 10px solid blue;
  width: 43%;
}
.table th {
  padding-top: 20px;
  text-align: left;
  font-size: 24px;
  margin: 30px;
}
.table tr,
td {
  text-align: left;
  padding-left: 50px;
  padding-right: 20px;
  padding-top: 20px;
  padding-bottom: 20px
}
<!doctype html>
<html lang="en">

<head>
  <meta charset="utf-8">
  <title>Restaurant Menu With JQuery</title>
  <link rel="stylesheet" type="text/css" href="style.css">
  <script src="https://code.jquery.com/jquery-1.12.4.js"></script>
</head>

<body>
  <input type="button" class="button" id="btMenu" value="Menu" style="background-color: #353535">
  <ul id="liMainMenu" class="ul">
    <li>
      <input type="button" class="button" id="btStarter" value="Starter" style="background-color: #008080">
    </li>
    <li>
      <input type="button" class="button" id="btMain" value="Main" style="background-color: #008080">
    </li>
    <li>
      <input type="button" class="button" id="btDessert" value="Dessert" style="background-color: #008080">
    </li>
  </ul>
  <ul id="liStarter" class="ul">
    <li>
      <input type="button" class="button" id="btStarter1" value="">
    </li>
    <li>
      <input type="button" class="button" id="btStarter2" value="">
    </li>
    <li>
      <input type="button" class="button" id="btStarter3" value="">
    </li>
  </ul>
  <ul id="liMain" class="ul">
    <li>
      <input type="button" class="button" id="btMain1" value="">
    </li>
    <li>
      <input type="button" class="button" id="btMain2" value="">
    </li>
    <li>
      <input type="button" class="button" id="btMain3" value="">
    </li>
  </ul>
  <ul id="liDessert" class="ul">
    <li>
      <input type="button" class="button" id="btDessert1" value="">
    </li>
    <li>
      <input type="button" class="button" id="btDessert2" value="">
    </li>
    <li>
      <input type="button" class="button" id="btDessert3" value="">
    </li>
  </ul>
  <table class="table">
    <th>Your Order:</th>
    <tr>
      <td>First :</td>
      <td id="selectedStarter"></td>
    </tr>
    <tr>
      <td>Main :</td>
      <td id="selectedMain"></td>
    </tr>
    <tr>
      <td>Dessert :</td>
      <td id="selectedDessert"></td>
    </tr>
    <tr>
      <td>Total :</td>
      <td id="total"></td>
    </tr>
  </table>
  </body>
  <html>

enter image description here

\$\endgroup\$
3
  • \$\begingroup\$ What does it do? \$\endgroup\$ Commented May 4, 2018 at 12:38
  • \$\begingroup\$ Restaurant menu that update orders and change color button \$\endgroup\$ Commented May 4, 2018 at 12:47
  • 3
    \$\begingroup\$ The site standard is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$ Commented May 4, 2018 at 13:13

4 Answers 4

Reset to default
4
\$\begingroup\$

Use better data structures.

When you have a problem you should try to find which data structure have more advantages for you, in this case i think that it should be an array.

Using an array.

The bigger advantage of using this approach is that is easier to remove or add new dishes to your menu, and you dont need to specificy the n-th of the dish, you can see that using 'first'/'second' would start to become difficult for a menu with 10 options or so.

Instead of:

var starter = {
    firstDish: "Salad",
    firstDishPrice: 15,
    secondDish: "Soup",
    secondDishPrice: 7,
    thirdDish: "Fish rolls",
    thirdDishPrice: 12        
  };

You can use it:

var starter = [{
    dish: "Salad",
    price: 15,
} { 
    dish: "Soup",
    price: 7,
}, {
    dish: "Fish rolls",
    price: 12
}];

And for free you now can make your events more abstract in a way to avoid repetition: There is one thing in HTML called data-*, its purpose is to hold some information that you need in order to identify the element; We can use it here:

<input type="button" class="starter-button" id="btStarter1" data-id="0" value="">

So as the array starts with 0, identification for the first dish is 0 as well. Now instead of creatring one event handler for each button we can abstract it to:

$(".starter-button").click(function() {
    // ...
    $(this).css("background-color", "red");
    $("#selectedDessert").html(selectedDessertFnc(starter[$(this).data("id")], starter[$(this).data("id")].price));
});

But this its just the start, you can use it better creating a even better data structure:

var dishes = [{
    "starter": [{
        name: "Salad",
        price: 15,
    }, { 
        name: "Soup",
        price: 7,
    }, {
        name: "Fish rolls",
        price: 12
    }],
    "desert": [...]
}]

Now using the same logic that before your button can be something like:

<input type="button" class="button" id="btStarter1" data-id="0" data-type="starter" data-target-id="selectedDessert" value="">

And your event:

$(".button").click(function() {
    // ...    
    var targetId = $(this).data("target-id");
    var dishType = $(this).data("type");
    var dishId = $(this).data("id");

    $(targetId).html(selectedDish(dishes[dishType][dishId].name, dishes[dishType][dishId].price);
});

The tl;dr version of my code review would be, Try to use the best data structures for your information, and think about how would the maintenance of the code, "how can i make it so its easier to add more features?".

After improving this bit your code should be pretty good, keep coding!

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

Like @Josenberg mentioned your data I believe would be better structured like:

var main = [{
    dish: "Steak",
    price: 17
},{
    dish: "Salmon",
    price: 12
},{
    dish: "Rissotto",
    price: 9
}];

Then to fill your elements you could use a single for loop.

for(var i=0;i<main.length;i++){
    document.getElementById("btMain"+(i+1).toString()).value =
      main[i].dish + ": " + main[i].price;
}

I would then add a class starter, main, and dessert to each of the respective elements. And then you could have a class on click function:

$('.starter').click(function(){
    $("#liStarter").children("li").children("input").css("background-color", "rgb(0, 230, 40)");
    $(this).css("background-color", "red");
    var index = $(".starter").index(this);
    $("#selectedStarter").html(selectedStarterFnc(starter[index].dish, starter[index].price));
});


$(document).ready(function() {
  //Variables
  var selectedStarter = {
    dish: "(None)",
    price: 0
  };
  var selectedMain = {
    dish: "(None)",
    price: 0
  };
  var selectedDessert = {
    dish: "(None)",
    price: 0
  };
  var starter = [{
    dish: "Salad",
    price: 15
    },{
    dish: "Soup",
    price: 7
    },{
    dish: "Fish rolls",
    price: 12
  }];

  var main = [{
    dish: "Steak",
    price: 17
    },{
    dish: "Salmon",
    price: 12
    },{
    dish: "Rissotto",
    price: 9
  }];

  var dessert = [{
    dish: "Sorbet",
    price: 4
    },{
    dish: "Fruit salad",
    price: 6
    },{
    dish: "Apple pie",
    price: 5
  }];

  function total() {
    return selectedStarter.price + selectedMain.price + selectedDessert.price;
  }

  function selectedStarterFnc(dish, price) {
    selectedStarter.price = price;
    selectedStarter.dish = dish;
    $("#total").html(total());
    return dish + "(" + price + ")";
  }

  function selectedMainFnc(dish, price) {
    selectedMain.price = price;
    selectedMain.dish = dish;
    $("#total").html(total());
    return dish + "(" + price + ")";
  }

  function selectedDessertFnc(dish, price) {
    selectedDessert.price = price;
    selectedDessert.dish = dish;
    $("#total").html(total());
    return dish + "(" + price + ")";
  }

  // Instantiating HTML Button Elements
  // Starter Elements
  for(var i=0;i<starter.length;i++){
    document.getElementById("btStarter"+(i+1).toString()).value =
      starter[i].dish + ": " + starter[i].price;
  }

  // Main Elements
  for(var i=0;i<main.length;i++){
    document.getElementById("btMain"+(i+1).toString()).value =
      main[i].dish + ": " + main[i].price;
  }

  // Dessert Elements
  for(var i=0;i<dessert.length;i++){
    document.getElementById("btDessert"+(i+1).toString()).value =
      dessert[i].dish + ": " + dessert[i].price;
  }

  // Your Order: Elements
  document.getElementById("selectedStarter").innerHTML =
    selectedStarter.dish + " (" + selectedStarter.price + ")";

  document.getElementById("selectedMain").innerHTML =
    selectedMain.dish + " (" + selectedMain.price + ")";

  document.getElementById("selectedDessert").innerHTML =
    selectedDessert.dish + " (" + selectedDessert.price + ")";

  // Functions (JQuery)
  // Main menu onClicks handler
  $("#btMenu").click(function() {
    $("#liMainMenu").toggle("slow");
  });

  $("#btStarter").click(function() {
    $("#liStarter").toggle("slow", function() {
      if ($(this).css("display") == "none") {
        $("#btStarter").css("background-color", "#008080");
      } else {
        $("#btStarter").css("background-color", "red");
      }
    });
  });

  $("#btMain").click(function() {
    $("#liMain").toggle("slow", function() {
      if ($(this).css("display") == "none") {
        $("#btMain").css("background-color", "#008080");
      } else {
        $("#btMain").css("background-color", "red");
      }
    });
  });

  $("#btDessert").click(function() {
    $("#liDessert").toggle("slow", function() {
      if ($(this).css("display") == "none") {
        $("#btDessert").css("background-color", "#008080");
      } else {
        $("#btDessert").css("background-color", "red");
      }
    });
  });

  // Starter onClicks
  $('.starter').click(function(){
    $("#liStarter").children("li").children("input").css("background-color", "rgb(0, 230, 40)");
    $(this).css("background-color", "red");
    var index = $(".starter").index(this);
    $("#selectedStarter").html(selectedStarterFnc(starter[index].dish, starter[index].price));
  });

  // Main onClicks
  $('.main').click(function(){
    $("#liMain").children("li").children("input").css("background-color", "rgb(0, 230, 40)");
    $(this).css("background-color", "red");
    var index = $(".main").index(this);
    $("#selectedMain").html(selectedMainFnc(main[index].dish, main[index].price));
  });

  // Dessert onClicks
  $('.dessert').click(function(){
    $("#liDessert").children("li").children("input").css("background-color", "rgb(0, 230, 40)");
    $(this).css("background-color", "red");
    var index = $(".dessert").index(this);
    $("#selectedDessert").html(selectedDessertFnc(dessert[index].dish, dessert[index].price));
  });
});
.button {
  background: rgb(0, 230, 40);
  outline: solid 2px #353535;
  border: solid 2px white;
  color: white;
  padding: 10px 15px;
  text-decoration: none;
  width: 200px;
  margin-top: 50px;
}

.ul li {
  display: inline;
  margin-right: 8px;
}
.ul {
  display: none;
}
.table {
  margin-top: 50px;
  border: 10px solid blue;
  width: 43%;
}
.table th {
  padding-top: 20px;
  text-align: left;
  font-size: 24px;
  margin: 30px;
}
.table tr,
td {
  text-align: left;
  padding-left: 50px;
  padding-right: 20px;
  padding-top: 20px;
  padding-bottom: 20px
}
<!doctype html>
<html lang="en">

<head>
  <meta charset="utf-8">
  <title>Restaurant Menu With JQuery</title>
  <link rel="stylesheet" type="text/css" href="style.css">
  <script src="https://code.jquery.com/jquery-1.12.4.js"></script>
</head>

<body>
  <input type="button" class="button" id="btMenu" value="Menu" style="background-color: #353535">
  <ul id="liMainMenu" class="ul">
    <li>
      <input type="button" class="button" id="btStarter" value="Starter" style="background-color: #008080">
    </li>
    <li>
      <input type="button" class="button" id="btMain" value="Main" style="background-color: #008080">
    </li>
    <li>
      <input type="button" class="button" id="btDessert" value="Dessert" style="background-color: #008080">
    </li>
  </ul>
  <ul id="liStarter" class="ul">
    <li>
      <input type="button" class="button starter" id="btStarter1" value="">
    </li>
    <li>
      <input type="button" class="button starter" id="btStarter2" value="">
    </li>
    <li>
      <input type="button" class="button starter" id="btStarter3" value="">
    </li>
  </ul>
  <ul id="liMain" class="ul">
    <li>
      <input type="button" class="button main" id="btMain1" value="">
    </li>
    <li>
      <input type="button" class="button main" id="btMain2" value="">
    </li>
    <li>
      <input type="button" class="button main" id="btMain3" value="">
    </li>
  </ul>
  <ul id="liDessert" class="ul">
    <li>
      <input type="button" class="button dessert" id="btDessert1" value="">
    </li>
    <li>
      <input type="button" class="button dessert" id="btDessert2" value="">
    </li>
    <li>
      <input type="button" class="button dessert" id="btDessert3" value="">
    </li>
  </ul>
  <table class="table">
    <th>Your Order:</th>
    <tr>
      <td>First :</td>
      <td id="selectedStarter"></td>
    </tr>
    <tr>
      <td>Main :</td>
      <td id="selectedMain"></td>
    </tr>
    <tr>
      <td>Dessert :</td>
      <td id="selectedDessert"></td>
    </tr>
    <tr>
      <td>Total :</td>
      <td id="total"></td>
    </tr>
  </table>
  </body>
  <html>

\$\endgroup\$
1
  • \$\begingroup\$ I had not thought of that but rendering the elements from the objects would be a good improvement as well. \$\endgroup\$ Commented May 4, 2018 at 14:34
1
\$\begingroup\$

Since you are using jQuery and have it tagged, I want to mention that you are missing an opportunity to use it. Every time you do document.getElementById(id), you could be using $('#id). This is more concise, allows you use jQuery's other methods like .html(), .val() and .data(), and makes it easier to change the selector without needing to fundamentally change the code.

For instance, instead of

document.getElementById("btStarter1").value = starter.firstDish + ": " + starter.firstDishPrice;

you could simply do

$('#btStarter1').val(starter.firstDish + ": " + starter.firstDishPrice);

If you then needed to access nodes by class instead of id, it would be straightforward to simply change the selector:

$('.starterButtons').val(starter.firstDish + ": " + starter.firstDishPrice);
\$\endgroup\$
1
  • \$\begingroup\$ Can the downvoter explain why they don't agree? \$\endgroup\$ Commented May 10, 2018 at 16:38
-1
\$\begingroup\$

As other answers have mentioned. use better data structures to organise your data

Check this out i have made a small fiddle.

https://jsfiddle.net/czpy2ze3/

$(document).ready(function(){


var dishes = {
    'starters': [
        {name: 'Salad', price: 15},
        {name: 'Fish rolls', price: 33},
    ],
    'main' : [
        {name: 'Steak', price: 30},
    ],
    'desserts': [
        {name: 'Apple Pie', price: 12},
    ]
};

var types = Object.keys(dishes);

var selected = {};

$("#xMenu").click(function(){
    $("#xContent").toggle();
});

var calculate = function() {
    $("#xBill").html("");

    var total = 0;
    $xBill = $("<div>", {});
    types.forEach(function(type, index) {
        var dishindex = selected[type];
        console.log(typeof dishindex);

        if(typeof dishindex == 'number'){
            var dish = dishes[type][dishindex];
            console.log(dish);
            total+= dish.price;
            $xBill.append($("<p>",{
                html: `${type} : ${dish.name}(${dish.price})`
            }));
        } else {

        }
    });

    $xBill.append($("<h3>", {
        html: `Total: ${total}`
    }))

    $("#xBill").append($xBill);
    console.log(total);
}
var render = function() {

    var $xTypes = $("<div>", {"class": "xType"});
    var $xDishes = $("<div>", {"class": "xDishes"});

    $("#xContent")
        .html("")
        .append($xTypes)
        .append($xDishes);

    types.forEach(function(type){

        var $x = $("<button>", { "class": "xTypes", "text": type});
        $xTypes.append($x);

        var $row = $("<div>", {});
        dishes[type].forEach(function(dish, index){

            var $dish = $("<button>", {
                "class": "dish",
                "text": dish.name,
                "click": function() {
                    selected[type] = index;
                    render();
                    console.log(selected);
                }
            });


            if(selected[type] == index) {
                $dish.addClass("xchosen");
            }

            $row.append($dish);
        });

        $xDishes.append($row);

        $x.click(function(){
            $row.toggle();
        })

    });

    calculate();
};




render();


});

Instead of reducing code you should focus on not hardcoding the data part into your website.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.