Converted placeable multidimensional array into object #43

camerongillette · 2017-11-30T02:59:01Z

No description provided.

lessthantrue · 2017-11-30T05:49:36Z

js/models/entity.js

+    setHeight(height){
+        this.height = height;
+    }
+}


If this is just a data class, can we forego the getters/setters? Someone more well versed in javascript standards should chime in, but this seems excessive

At this time. I don't think the getters or setters are needed.

@SpyMaster356 Just for my own knowledge, what is standard practice in js when it comes to getters/setters?

From what I gathered on Mozilla's Javascript reference page, it adheres to generic object oriented standards, and since this is nothing more than a data-holding struct, we don't need to worry much about data hiding.

Fun fact: there's some syntax sugar for this
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/set
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get

Generally, there isn't much need for getters and setters, but there are use cases such as:

When you want do extra things when setting a value

When the value is computed from other properties

When you want to restrict setting a value.

camerongillette · 2017-12-01T15:27:35Z

Note, this will need to updated to accomodate the horizontal mirror rotation updates to the js.

Zeragamba · 2017-12-01T23:17:53Z

Waiting on #47 as that includes eslint settings for es6

camerongillette · 2017-12-03T17:11:45Z

@SpyMaster356 Can review the changes in them most recent commit. They should fix the underground belt and underground pipe rotation issue.

Zeragamba · 2017-12-05T12:19:23Z

the lint errors can be fix automatically, use npm run lint -- --fix to fix them.

Zeragamba · 2017-12-05T12:23:55Z

js/script.js

@@ -608,3 +609,11 @@ function encode(json) {
    var bstring = "0" + base64;
    return bstring;
 }
+
+function factorioEntityObjectArrayFromMultiArray(sourceArray){


that function name is a bit of a mouthful

Zeragamba · 2017-12-05T12:26:44Z

js/script.js

+function factorioEntityObjectArrayFromMultiArray(sourceArray){
+    var objectArray = new Array();
+    for(var i = 0; i < sourceArray.length; i++){
+        objectArray.push(new Entity(sourceArray[i][0],sourceArray[i][1],sourceArray[i][2],sourceArray[i][3],sourceArray[i][4],sourceArray[i][5]));


can you use intermediate variable names here instead of directly passing each property. It's hard to understand what values are being passed around here.

Eg.

var entityData = sourceArray[i]; var name = entityData[0]; var rotation = entityData[1]; // etc...

Zeragamba · 2017-12-05T12:27:40Z

js/script.js

@@ -38,7 +38,7 @@ placeable[4] = height/length
 placeable[5] = mirror flipped horizontal rotation
 */

-var placeable = [
+var placeable = factorioEntityObjectArrayFromMultiArray([


instead of writing a function to turn these arrays into Entity objects, couldn't we just create Entity objects here?

Zeragamba · 2017-12-05T12:29:04Z

js/models/entity.js

+        this.direction = direction;
+        this.width = width;
+        this.height = height;
+        this.mirrorFlippedHorizontal = mirrorFlippedHorizontal;


this property name is a bit long, but i'm not sure if there is a better name for this

Zeragamba · 2017-12-05T12:30:02Z

js/models/entity.js

@@ -0,0 +1,10 @@
+class Entity{


we should probably namespace our stuff at some point instead of overloading the window space. For now this is ok.

Zeragamba · 2017-12-05T12:36:12Z

js/models/entity.js

+        this.height = height;
+        this.mirrorFlippedHorizontal = mirrorFlippedHorizontal;
+    }
+}


if you want to keep using the array of data, you could create a static method that returns a new Entity:

static function fromDataArray(dataArray) { var name = dataArray[0]; var rotation = dataArray[1]; //etc... return new Entity(name, rotation, //etc... }

Ok, I'll add this. Eventually I'd like to get away from the array format and have a config file somewhere storing all of the entity information.

camerongillette · 2017-12-07T03:53:50Z

@SpyMaster356 Ready for re-review

Zeragamba

you'll need to run npm run lint -- --fix to fix the errors that Travis is throwing

camerongillette requested a review from Zeragamba November 30, 2017 02:59

lessthantrue reviewed Nov 30, 2017

View reviewed changes

camerongillette force-pushed the 42-placeable-into-object branch from d282618 to 081e865 Compare December 1, 2017 03:51

camerongillette changed the title ~~Converted placeable multidimensional array into object~~ WIP : Converted placeable multidimensional array into object Dec 1, 2017

camerongillette force-pushed the 42-placeable-into-object branch from 081e865 to 7f2935f Compare December 3, 2017 17:07

camerongillette changed the title ~~WIP : Converted placeable multidimensional array into object~~ Converted placeable multidimensional array into object Dec 3, 2017

camerongillette force-pushed the 42-placeable-into-object branch from 7f2935f to 11a163d Compare December 3, 2017 22:33

Zeragamba reviewed Dec 5, 2017

View reviewed changes

Converted placeable multidimensional array into object

0891b0f

camerongillette force-pushed the 42-placeable-into-object branch from 11a163d to 3d8e3a0 Compare December 7, 2017 03:03

Added underground belts and underground pipes rotation fix.

3bb0232

camerongillette force-pushed the 42-placeable-into-object branch from 3d8e3a0 to 3bb0232 Compare December 7, 2017 03:53

camerongillette mentioned this pull request Dec 26, 2017

BUG:: Rotation some items does not result in a proper image display #55

Open

Zeragamba approved these changes Dec 27, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converted placeable multidimensional array into object #43

Converted placeable multidimensional array into object #43

camerongillette commented Nov 30, 2017

lessthantrue Nov 30, 2017

Zeragamba Nov 30, 2017

camerongillette Dec 1, 2017

lessthantrue Dec 1, 2017

Zeragamba Dec 5, 2017

camerongillette commented Dec 1, 2017

Zeragamba commented Dec 1, 2017

camerongillette commented Dec 3, 2017

Zeragamba commented Dec 5, 2017

Zeragamba Dec 5, 2017

Zeragamba Dec 5, 2017

Zeragamba Dec 5, 2017

Zeragamba Dec 5, 2017

Zeragamba Dec 5, 2017

Zeragamba Dec 5, 2017

camerongillette Dec 5, 2017

camerongillette commented Dec 7, 2017

Zeragamba left a comment •

edited

Loading

Converted placeable multidimensional array into object #43

Are you sure you want to change the base?

Converted placeable multidimensional array into object #43

Conversation

camerongillette commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camerongillette commented Dec 1, 2017

Zeragamba commented Dec 1, 2017

camerongillette commented Dec 3, 2017

Zeragamba commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camerongillette commented Dec 7, 2017

Zeragamba left a comment • edited Loading

Choose a reason for hiding this comment

Zeragamba left a comment •

edited

Loading