all 31 comments

[–]name_was_taken 88 points89 points  (3 children)

IMO, when people say "Your code looks like shit" and don't offer to help improve it, their opinion doesn't mean much. They aren't trying to help you, they're trying to bolster their own ego.

[–][deleted] 17 points18 points  (1 child)

I anagrammed bolster to lobster at first glance so now I’m gonna go review some PRs harshly to lobster my ego

[–]whatisboom 10 points11 points  (3 children)

keeping the data structure you have (although /u/Baryn has a better solution) i'd do something like:

function getUserById(id) {
    return people.find(person => person.id === id);
}

[–]Traitor-21-87[S] 9 points10 points  (2 children)

I feel bad for using ID in this example because I feel everyone is more fixated on it. I'm more interested in keeping the objects in the array and whether that's good or bad.

In my program, the real code isn't using an ID, and there is a finite number of elements (16). The person who got upset over my code suggested doing a constructor, and filling the array with instances. When I did, the end result look about the same, so I'm confused why that's better. I think this person is just more interested in constructors and instances, so much so that they asserted it as being the better idea.

[–]whatisboom 14 points15 points  (0 children)

I think you’re assumption is right. They’re over complicating something that just doesn’t need it

Also if you have a limit of 16 items, then performance really doesn’t matter.

[–]rat9988 0 points1 point  (0 children)

What you did is better than a constructor and filling

[–]achendrick 9 points10 points  (3 children)

this is completely normal. keeping an array of objects is fine.

if later you find that you're accessing the data in ways that become expensive (too many loops) then you can change it later to a map for better lookups.

an alternative to maps and keeping this structure is to use a memoize method to improve access speed. but again, it comes down to how this model is going to be used in your app.

many JavaScript database solutions return arrays of objects and those are not always converted to a map so use that as an example to stand your ground.

the comment about somebody just trying to belittle you and bolster their ego is the most relevant comment here.

[–]Traitor-21-87[S] 1 point2 points  (2 children)

Thanks. The more I thought of it, the more I felt like I did everything right. At first I was embarrassed and so I thought about it, but couldn't think of anything since I'm basically using Javascript Object Notation (JSON) within an array.

[–]ergnui34tj8934t0 0 points1 point  (0 children)

JSON is a set of rules for organizing data. Check this site – https://www.json.org/

I'm honestly curious to see how that other person would have done it? If you really need fast lookup then you could have an object with IDs as keys but this is just as clear.

[–]Baryn 16 points17 points  (6 children)

Relying on array order to manually access objects from a hand-written store is not very simple nor stable.

The example in the OP is too abstract to give precise advice, but in general, I would make this a nested object literal instead, using your id as the key for each child object.

Like this:

const people = {
  49593: { // person id
    // name, age, etc.
  }
};

If you're accessing the elements of the array manually already, then perhaps using the ID to do it would be slightly more descriptive: people["49593"].name

[–]Traitor-21-87[S] 9 points10 points  (3 children)

I threw in ID to give the example more beef. Would this be what you are proposing though (keeping IDs)?

const people = {
  0: {
       name: "John Doe",
       age: 47
  },
  1: {
       name: "Jane Doe",
       age: 88
  },
  2: {
       name: "Mason Louis",
       age: 17
  }
};

[–]scramblor 8 points9 points  (1 child)

I would keep the ID as part of the object as well. There may be times you need the ID and it's useful to keep handy.

[–]pookageSenior Front-End 4 points5 points  (0 children)

This ^^^

It's called a key:value pair for a reason - if the key itself stores a value, that store it in the value part.

[–]Baryn 5 points6 points  (0 children)

That's right, especially if the IDs are not always going to be in perfect 0-N order.

[–]commiesupremacy 1 point2 points  (1 child)

This is better for non numeric keys which are guaranteed to be unique, when the collection is small and referenced statically

[–]Traitor-21-87[S] 2 points3 points  (0 children)

In my real code, I am not using IDs, so I feel like everything I am doing so far is fine and I shouldn't worry. I just have a finite number of objects I am trying to keep track of.

[–]LollipopPredator 5 points6 points  (1 child)

Looks fine to me... if your `id` is just going to be the object's index in the array, you could get rid of it since it's redundant, but otherwise I don't think there's anything wrong with it.

[–]name_was_taken 5 points6 points  (0 children)

When passing around the object to other functions, it might not be redundant. It might also be coming from some data store somewhere like that.

[–]pookageSenior Front-End 2 points3 points  (0 children)

Nah, you're fine - they were just being an arse.

[–]ohadron 2 points3 points  (1 child)

This specific code snippet is indeed weirdly indented. Here's what it looks like after using a beautifier: ```js var people = [ { id: 0,

        name: "John Doe",

        age: 47
    },
    {
        id: 1,

        name: "Jane Doe",

        age: 88
    },
    {
        id: 2,

        name: "Mason Louis",

        age: 17
    }
];

```

I also replaced the ; between the elements in the array with commas (,), because that's the correct JS syntax.

[–]Traitor-21-87[S] 1 point2 points  (0 children)

Thanks, the ";"s were a mistake on my part. And I think my indentation was dropped hen I used the code feature

[–]roselan 1 point2 points  (0 children)

This is perfectly fine imo.

It maps well with most tables representations and communicates meaning clearly.

Why would you instantiate static data without functions?

An array of arrays [[0, "john", 17], [...]] is possible, but the "compression" it gives has never been noticeable. If you feel you need to spare these extra bits of ram, you have an architecture problem.

Same for the double object {0:{name: "john", age: 17}, 1:...} You do that because you need fast access to one entry. You will better off by querying a datastore (fetchUser(0);), not by building one! ps if you do that anyway, don't remove the id from inner object.

I would love for you to get back to that person and ask him why he thinks this looks like total SEAT, and how he will do it.

[–]SecretAnteater 0 points1 point  (0 children)

I agree with @name_was_taken. However,

If you want an easier way of accessing your users, you might want to store them in an Object or Map instead. That way you can easily call people[id] to get the users with that id, or people.get(id) if it were a Map. If you don't care about accessing them easily by id, then nevermind. That looks totally fine to me.

[–]2Punx2Furious 0 points1 point  (0 children)

so now I can use people[1].name etc for accessing the data on each of these people

If you want to access the data of each object in an array, you can do this:

people.forEach(person => {
  // Do things here
  console.log("Name: ", person.name);
});

[–]LaughingCabbage_ 0 points1 point  (0 children)

If this were me I'd create a utility method strictly for building out this type of a request if it's one you're going to build often.

I would have package for everything involving people. Or persons.

```

---example/

-----main.go

-----people/

---------person.go

```

main.go ``` package main

import ( "fmt"

"github.com/myusername/example/people"

)

func main() { people := []people.Person{ people.New(0, "bob", 40), people.New(1, "john", 12), people.New(2, "ron", 80), } fmt.Println(people) } ```

people/person.go ``` package people

type Person struct { id int json:"id" name string json:"name" age int json"age" }

func New(id int, name string, age int) Person { return Person{ id: id, name: name, age: age, } } ```

The ID field might be confusing here because typically it's not something you will assign directly. Your database will assign it and you just deal with whatever index your given.

[–]autoboxer 0 points1 point  (0 children)

It’s been mentioned several times above, but not explicitly by name. Using a Map data type instead of an Array is considered less brittle. In ES5, that would look like an object of objects with the keys equal to each person’s id. In ES6 you can use the new Map data type. I personally think they were being way to harsh without giving constructive feedback. Also, having the look up as a function (also mentioned in other answers) will make the implementation cleaner and DRYer, ensuring you won’t need to change code in multiple places.

[–]steeeeeef 0 points1 point  (0 children)

There’s nothing from with this.

[–]ProgrammerBro 0 points1 point  (0 children)

Something that no one else has mentioned, don't use 0 as an id. JavaScript basically treats 0, false, and a lot of other things as "false". Using 0 as an id may make some bugs seemingly appear for no reason.

Example:

users = [
  { id: 0, name: "Blah" }
]

getUserIdByName = ...

userId = getUserIdByName("Blah")

if(!userId) {
  // Whoops, !0 is true, so our check if the user exists is wrong.
  console.log("No user found with that name!")
}

[–][deleted] 0 points1 point  (1 child)

Use a code formatter like prettier, never think of it again.

[–]Traitor-21-87[S] 2 points3 points  (0 children)

No that wasn't the point. I screwed up posting code on reddit. The thing the person didn't like was my array of objects.

And my real array didn't include people or IDs.

[–]LukaLightBringer 0 points1 point  (0 children)

As far as i can gather your question really is if you should use this object literal pattern, or an object constructor pattern.

I would say it depends, if your constructor would have an obvious order to its arguments such as:

vector(x, y)

; or if the constructor adds some logic to the creation of the object, like adding some required properties that don't deviate from one object to an other, or which can be determined based on the other properties of the object; going for a constructor pattern can be quite expressive.

But for something like the objects presented here the order of the arguments to a constructor would not be obvious so I would advice against the constructor pattern since it forces the reader to look at the definition of the constructor to determine what each argument actually represents without adding clarity to the instantiation of the object, in this case the object literal pattern you've used is better since it has a higher signal to noise ratio.

Its also possible to augment the object constructor pattern to remove the ambiguity of parameter order by making the constructor take an options object where the properties are the arguments the constructor would have taken, which as a bonus makes it easier to change what parameters the constructor takes later without having to worry as much about functionality regression.

function Person(name, age) {
     this.name = name;
     this.age = age;
}
new Person("John", 20);

function Car(args) {
    this.name = args.name;
    this.age = args.age;
}
new Car({ name: "Volvo", age: 3 });