4
\$\begingroup\$

The goal is to draw random tarot cards from the suit, number of cards is determined by the amount of HTML elements with 'card-container' class on given page.

The deck is structured as follows:

  • major arcana (trumps), numbered from 0 to 21
  • minor arcana have 4 suits, each numbered 1 to 14.

The result cannot contain duplicates.

After the cards are drawed the script retrieves card description form an external HTML file and replaces img src to according img path.

I also draw background image on canvas as an additional functionality.

The script does what it's supposed to do (at least that's it seems so); I would appreciate any suggestions on its structure, efficiency and how can I generally improve it.

var suits = ['cups', 'disks', 'swords', 'wands', 'trumps']; var minor_names = ['ace', 'two', 'three', 'four', 'five', 'six', 'seven', 'eight', 'nine', 'ten', 'princess', 'prince', 'queen', 'knight']; var major_names = ['The Fool', ' The Magician', 'The Priestess', 'The Empress', 'The Emperor', 'The Hierophant', ' The Lovers', 'The Chariot', 'Adjustment', 'The Hermit', 'Fortune', 'Lust', 'The Hanged Man', 'Death', 'Art', 'The Devil', 'The Tower', 'The Star', 'The Moon', 'The Sun', 'The Aeon', 'The Universe']; var picked = []; var img_src; //console.log(suits); function getRandomInt(min, max) { return Math.floor(Math.random() * (max - min + 1)) + min; } function card_name(suit, card_no) { var name = minor_names[card_no - 1]; if (suit === 'trumps') { name = major_names[card_no]; } } function draw_cards() { var suit = suits[Math.round(Math.random() * (suits.length - 1))]; var min = 1; var max = 14; if (suit === 'trumps') { min = 0; max = 21; } var card_no = getRandomInt(min, max); card_name(suit, card_no); if (card_no < 10) { card_no = '0' + card_no; } var set_name = suit + '-' + card_no; picked.push(set_name); } function draw(list) { $('.card-container').each(function() { draw_cards(); }); for (i = 0; i < picked.length; i++) { for (j = 0; j < picked.length; j++) { if (i != j) { if (picked[i] == picked[j]) { picked.splice(i, 1); draw_cards(); } } } } $('body').addClass('show-cards'); } function uncover(list) { var a = 0; $('.card-container').each(function(i) { var item = list[a]; $(this).find('article').load( 'content/' + item + '.html'); img_src = 'cards/'+ item + '.jpg'; $(this).find('.front img').attr('src', img_src); a++; var li = $(this); setTimeout(function() { li.addClass('active'); }, i*500); // delay 500 ms }); var img = new Image(); img.onload = function() { var ctx = document.getElementById('ctx').getContext('2d'); ctx.drawImage(img, 0, 0, 400, 400 * img.height / img.width); }; img.src = img_src; } $('.draw').click(function() { picked.length = 0; $(this).remove(); $('.show').fadeIn(); draw(); }); $('.show').click(function() { $(this).remove(); uncover(picked); });
\$\endgroup\$

1 Answer 1

Reset to default
4
\$\begingroup\$

Here are a few thoughts:

I would recommend wrapping your code in an IIFE. This allows you to have a private scope for your code and will help lessen the chance of a collision with other code.

I would also create this as a JS Object with properties and methods. Again this will help with keeping your code private and helps by not polluting the global namespace.

Caching some of your values into variables would help.

One thing I am not sure of is why you broke your "deck" of cards out into multiple arrays. Doesn't this make the code more difficult? Couldn't all the cards be added into one large(r) array for the entire deck? That way you wouldn't have to adjust min/max values, figure out if it's a trump card, figure out the card name, etc. For example var deck = ['One of Cups','Two of Cups',....'One of Disks',....'The Fool','The Magician'.....];.

(function( $ ){ //start with an IIFE to create private scope var original = ['One of Cups','Two of Cups','One of Disks','The Fool','The Magician']; var deck = []; var picked = []; var container = $('.card-container'); function getRandomInt() { return Math.floor(Math.random() * deck.length); } function card_name(num) { return deck[num]; } function draw_cards() { var card_no = getRandomInt(); picked.push(card_no); deck.slice(card_no, 1); } function draw() { // the param `list` was never used so removed container.each(function() { draw_cards(); }); $('body').addClass('show-cards'); } function uncover(list) { // This shows card but I am not sure where some of this data is coming // from so leaving pretty much as is except caching vars. var a = 0; var img_src =''; container.each(function(i) { var item = list[a]; var $t = $(this); $t.find('article').load( 'content/' + item + '.html'); img_src = 'cards/'+ item + '.jpg'; $t.find('.front img').attr('src', img_src); a++; setTimeout(function() { $t.addClass('active'); }, i*500); // delay 500 ms }); var img = new Image(); img.onload = function() { var ctx = document.getElementById('ctx').getContext('2d'); ctx.drawImage(img, 0, 0, 400, 400 * img.height / img.width); }; img.src = img_src; } function init() { // do things to help set up the "board" for first time use // for now just call "drawFunction" and add click events $('.draw').on('click', drawFunction); $('.show').on('click', showFunction); drawFunction(); } function drawFunction() { picked.length = 0; deck = original; // reset to original deck $(this).hide(); // hide it in case we want to show again $('.show').fadeIn(); draw(); } function showFunction() { $(this).hide(); // hide it in case we want to show again uncover(picked); } //now create all this as an object var tarot ={ init : init }; window.tarot = tarot; })( jQuery, undefined ); $(function(){ // on document.ready call init function tarot.init(); });

I haven't had a chance to test any of this code but it's more for explanation. Hope that helps.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your feedback. I figured it would be nicer to have smaller arrays and reuse them rather than having to type the name of each card, but after all you're probably right about this. Also if I only had one array I could just remove the item which was already drawn instead of checking for duplicates and running that function inside 2 ugly for loops :) \$\endgroup\$ Commented May 25, 2015 at 17:06

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.