6
\$\begingroup\$

I am fairly new to OOP and am working on a little website project to practice. I wanted to post after getting the initial setup done in case I am making any major errors or not following best practices.

Feedback Requested Looking for feedback mostly on the OOP aspect. Since I am new I don't have a full understanding I'll take any suggestions of what I need to learn about. You can give me terms to search for, article a bonus. Of course I'm more than happy to see any code reviews on this, just thought it would be more beneficial to focus on getting a better understanding.

Code Purpose Create a soccer field on the map by clicking the "Add Field" button and then clicking on the map. This is not a final version, so will be working on UI and more features such as size & rotation of the fields, and interacting with ones already placed (moving, resizing, removing, etc).

CodePen Live Example: https://codepen.io/BadMoodTaylor/pen/KyQoea

// setup Map object
var Map = {};
Map.clickFunction = '';
Map.left = $('.map-container').position().left;
Map.top = $('.map-container').position().top;
Map.fields = [];
Map.id = $('.map-container');


class Field{
  constructor(num, x, y){
    // assigning each field a unique class name
    this.id = 'field' + num;
    // copying the HTML from the template
    this.html = '<div class="svg-field ' + this.id + '">' + $('#template').html() + '</div>';
    // Adding the HTML to the map
    Map.id.append(this.html);
    // Storing Height and Width
    this.height =   $('.' + this.id).height()
    this.width =   $('.' + this.id).width();
    // Setting location based on where user clicked.  Setting middle of field to click point
    this.x = x - this.width / 2;
    this.y = y - this.height / 2;
    $('.' + this.id).css('left', this.x);
    $('.' + this.id).css('top', this.y);
  }

// not used yet, will be to reposition field
  position(x, y){
    this.x = x;
    this.y = y;

  }
}



  // Add Field Button Clicked
  $('.add-field').click(function(){
    Map.clickFunction = 'addField';
    console.log("Add Field button pressed and Map.clickFunction set to: " + Map.clickFunction)
  })

  // Click on Map
  $('.map-container').click(function(e){
    // Finding desired action based on Map.clickFunction
    switch (Map.clickFunction) {
      case 'addField':
        // Reset parameter
        Map.clickFunction = '';

        // Create new field
        let newField = new Field(Map.fields.length, e.pageX - Map.left, e.pageY - Map.top);
        Map.fields.push(newField);

        break;
      default:
        console.log('default')

    }
  })
  body{
    display: flex;
    flex-flow: column;
    justify-content: center;
    align-items: center;
  }

  .lines{
    fill:none;
    stroke-width: 1;
    stroke: white;
    animation: 1.4s 0.1s both drawLines;
    stroke-dasharray: 100 0;
  stroke-dashoffset: 0;
  }
  @keyframes drawLines {
    from{
          stroke-dasharray: 1 100;
      stroke-dashoffset: 10;
    }
  }
  .map-container{
    position: relative;
    width: 1000px;
    height: 700px;
    background: #3d9156;
  }
  .svg-field{
    position: absolute;
    top: 60%;
    left: 6%;
    z-index: 5;
    width: 70px;
    height: 120px;
  }
  .svg-field svg{
    position: absolute;
    top:0;
    left:0;
    height: 120px;
    width: 70px;
  }
  .hidden{
    display: none;
  }
  .spot{
    fill:white;
  }
  .penalty-arc{
    stroke-dasharray: 18 50;

  }
  .penalty-arc.top-half{
        stroke-dashoffset: -6.5;
  }
  .penalty-arc.bottom-half{
    stroke-dashoffset: -38;
  }


  .div-button:hover {
    transform: translateY(5px);
    box-shadow: 0 0 0 #e67409;
    text-shadow: 2px 2px 0 #42964c;
    background-color: #69c160;
}

.div-button {
    background-color: #69c160;
    color: white;
    border-radius: 10px;
    padding: 0.5em;
    font-size: 1.5rem;
    cursor: pointer;
    box-shadow: 0 5px 0 #42964c;
    transition-duration: 0.2s;
    text-shadow: 2px 2px 0 #42964c;
    animation: 0.6s 5.8s both button1;
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.2.1/jquery.min.js"></script>
<div class="map-container">
  <div id="template" class="svg-field">
      <svg version="1.1"
           baseProfile="full"
           width="100%" height="100%"
           viewBox="0 0 70 120"
           xmlns="http://www.w3.org/2000/svg">

          <!--Perimeter (Touch Lines and Goal Lines) -->
          <rect class="lines" x="0" y="0" width="100%" height="100%"/>
          <!--Center Circle -->
          <circle class="lines" cx="50%" cy="50%" r="10" />
          <!--Halfway line -->
          <line class="lines" x1="0" y1="50%" x2="100%" y2="50%" />
          <!--Penalty Spots -->
          <circle class="spot" cx="50%" cy="12" r="0.5" />
          <circle class="spot" cx="50%" cy="108" r="0.5" />
          <!--Goal Area -->
          <path class="lines" d="M 35 120 m -14 0 l 0 -6 l 28 0 l 0 14" />
          <path class="lines" d="M 35 0 m -14 0 l 0 6 l 28 0 l 0 -14" />
          <!--Penalty Areay -->
          <path class="lines" d="M 35 120 m -22 0 l 0 -18 l 44 0 l 0 18" />
          <path class="lines" d="M 35 0 m -22 0 l 0 18 l 44 0 l 0 -18" />
          <!--Penalty Arc -->
          <circle class="lines penalty-arc top-half" cx="50%" cy="12" r="10" />
          <circle class="lines penalty-arc bottom-half" cx="50%" cy="108" r="10" />
        </svg>
  </div>
</div>




<div class="div-button add-field">
  Add Field
</div>

\$\endgroup\$
5
\$\begingroup\$

That animations looks pretty cool. I'm no expert in OOP, but there is one thing I want to mention. This

this.html = '<div class="svg-field ' + this.id + '">' + $('#template').html() + '</div>';

is a bad idea. You might not always be sure what's passed to the function, and if it gets called like this

new Field('"><script>alert("HAX")</script>',100,100)

someone can inject any code, which is a major security risk. Instead you can do it like this

this.html = document.createElement('div');
this.html.classList.add("svg-field");
this.html.classList.add(this.id);
let templateClone = document.getElementById('template').cloneNode(true);
this.html.appendChild(templateClone);

I didn't use jQuery, but you could, since you already have it included. You also need a few tweaks, since the position changed a bit.

\$\endgroup\$
1
\$\begingroup\$

OOP feedback

The class only has two methods, and one isn't used (you even said so yourself: // not used yet, will be to reposition field). That position method could be called by the constructor, and other parts of the constructor could be moved out into other methods - e.g. one to create the element, set the styles, etc.

General feedback

If there is only one element that will contain the map, then perhaps using the id attribute instead of a class name would be more appropriate.


DOM queries aren't cheap:

”...DOM access is actually pretty costly - I think of it like if I have a bridge - like two pieces of land with a toll bridge, and the JavaScript engine is on one side, and the DOM is on the other, and every time I want to access the DOM from the JavaScript engine, I have to pay that toll”
    - John Hrvatin, Microsoft, MIX09, in this talk Building High Performance Web Applications and Sites at 29:38, also cited in the O'Reilly Javascript book by Nicholas C Zakas Pg 36, as well as mentioned in this post

Bearing in mind he said that ~10 years ago and browsers have come a long way, it is still wise to consider. It is best to store those references in a variable (e.g. const mapContainer = $('.map-container')) and utilize those whenever needed later.


const can be used for any variable that doesn't get re-assigned - e.g. newField. That way there won't be any accidental re-assignment.


The switch statement seems excessive for two cases (including the default) - it could easily be replaced by a simple if/else chain

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