Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I am using the following code in a Node.js / React project. It works fine but it looks like it could be consolidated a little more using a OO pattern.

import projectsData from '../data/index.js';

function Project(project) {
  this.name = project.name;
  this.order = project.order;
  this.title = project.title;
  this.date = project.date;
  this.tags = project.tags;
  this.logo = project.logo;
  this.html = project.html;
  this.agency = project.agency;
  this.slides = Object.values(project.slides).map(slide => slide);
  this.path = `projects/${this.name}`;
  this.route = `/projects/${this.name}`;
  this.slidesPath = `/projects/${this.name}/slides/`;
  this.hiDefAffix = '@2x';
};

const all = {};
Object.values(projectsData).map(project => all[project.name] = new Project(project));

const projects = {
  all,
  sorted(){ return Object.values(this.all).sort((a, b) => a.order - b.order) },
  get(project) {
    try {
      if (this.all[project]){
        return this.all[project];
      } else {
        throw new Error(`Project "${project}" not found.`);
      }
    } catch (error) {
      return false;
    }
  },
  toJSON() {
    return JSON.stringify(this.all);
  }
};

export default projects;

Basically I have a constructor and an exported object that ends up having a bunch of objects created by the constructor above it. Can these be consolidated in some way?

share|improve this question
    
What's that throw about, why not just return false directly? – Bergi Sep 20 at 21:39
    
.map(slide => slide) doesn't change anything, it should be omitted – Bergi Sep 20 at 21:39
    
toJSON should, despite the name, not return a JSON string but rather an object that will be used for the result when calling JSON.stringify(projects) – Bergi Sep 20 at 21:41
    
I'm not sure what you mean by "more object-oriented"? – Bergi Sep 20 at 21:42
    
@bergi Here's my reasoning: re: "throw" it's because I wanted to see where the program failed without breaking it re: don't use .map(slide => slide). You're right, that is unnecessary. Thanks! re: toJSON. If that's the case then toJSON is the same thing as "all". If you were working on this project which of those names would you prefer? – j0e Sep 20 at 21:43
function Project(project) {
  this.name = project.name;
  this.order = project.order;
  this.title = project.title;
  this.date = project.date;
  this.tags = project.tags;
  this.logo = project.logo;
  this.html = project.html;
  this.agency = project.agency;
  this.slides = Object.values(project.slides).map(slide => slide);
  this.path = `projects/${this.name}`;
  this.route = `/projects/${this.name}`;
  this.slidesPath = `/projects/${this.name}/slides/`;
  this.hiDefAffix = '@2x';
};

If all this does is just set props to an instance, then you probably don't even need a constructor. You can simply use a factory function that accepts a project object and returns an object with properties.

function createProject({name, order, title, date, tags, logo, html, agency, slides, name}){
  return {
    name,
    order,
    title,
    date,
    tags,
    logo,
    html,
    agency,
    slides: Object.values(project.slides),
    path: `projects/${this.name}`,
    route: `/projects/${this.name}`,
    slidesPath: `/projects/${this.name}/slides/`,
    hiDefAffix: '@2x'
  };
}

Object.values(project.slides).map(slide => slide);

Looks like project.slides is an object. Object.values creates an array of values. array.map just creates a copy of that array, which is useless.


const all = {};
Object.values(projectsData).map(project => all[project.name] = new Project(project));

You are misusing array.map. array.map is for transforming one array into another in a 1-to-1 transformation. What you are doing here is creating an object. This is a job for array.reduce.

const all = Object.values(projectsData).reduce((all, project) => {
  return { ...all, [project.name]: createProject(project)};
}, {});

try {
  if (this.all[project]){
    return this.all[project];
  } else {
    throw new Error(`Project "${project}" not found.`);
  }
} catch (error) {
  return false;
}

The try-catch is pointless. You are immediately catching the error and simply returning false. Why not return false in the first place?

return this.all[project] || false;

toJSON() {
  return JSON.stringify(this.all);
}

I believe this is a practice commonly done by Java developers to have an intermediate function to handle special properties. But this is unnecessary in most cases since any simple JS object is immediately serializable by JSON.stringify, and you don't appear to use any of the new data types.


How can I make this Javascript code more OO

This is the wrong way to think in terms of programming, locking yourself to a paradigm. What you should be asking is "How to make this code simpler?" or "How to make this code easier to understand?".

Also, OO isn't about classes, or constructors or methods and properties. It's about working with objects, whether they are instances of classes or simple data structures like object literals or arrays. You can do a lot with just object literals and arrays. You aren't going to need the class and methods fluff in most cases.

share|improve this answer
    
Wow, this is great, thanks. – j0e Sep 21 at 20:57

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.