0

I am at currently navigating my way around OOP architecture and looking for best practices rather than code that just works. To highlight my latest conondrum I have written a short example of a problem I run into regularly below. In this example app users can start a learning journey of a programming subject they want to learn and plot their progress throughout. In reality I would need a backend for this type of app as the data will need to be saved, but for learning purposes I have written a short piece of code as if I am making it solely on the frontend.

A user can start a new learning journey by typing in the name of a journey and selecting the subject e.g. react, java, npm etc. This journey is then stored in an array of objects. Inside the app we can do a lot of things. e.g find all the React journeys by filtering, sort the journeys in various ways e.g. date started, add to an existing journey with text, highlight important words in the journey using the selection API in the browser.

I have written some code below which is a small snippet of what the app can do:

class Journey {
  constructor(name, subject, dateStarted = new Date().toLocaleString()) {
    this.name = name;
    this.subject = subject;
    this.dateStarted = dateStarted;
  }
}

class JourneyCollection {
  constructor(journeyArr = []) {
    this.journeysArr = journeyArr;
  }

  addJourney(name, subject) {
    this.journeysArr.push(new Journey(name, subject));
  }

  sortJourneys() { // as the app grows this will receive an argument to establish how we will sort e.g. by date, alphabetic, or number of updates to journey
    this.journeysArr.sort((a, b) => {
      return new Date(b.dateStarted) - new Date(a.dateStarted);
    });
  }

  getJourneysBySubject(subject) {
    const journeys = this.journeysArr.filter((jrny) => {
      return jrny.subject === subject;
  });

    return journeys;
  }
}

const journeys = new JourneyCollection();

document.querySelector("#add").addEventListener("click", () => {
  journeys.addJourney(
    document.querySelector("#name").value,
    document.querySelector("#subject").value
  );
});

document.addEventListener("keypress", (e) => { // if we hit enter the array gets sorted
  if (e.keyCode === 13) {
    journeys.sortJourneys();
  }
});

document.querySelector("#findSubject").addEventListener("click", () => {
  const returned = journeys.getJourneysBySubject(
    document.querySelector("#find").value
  );
});

Now the problem I am starting to have and will continue to have is that I am well on my way to having a god type class which controls the whole application because each method relies on the array which holds the all the Journeys.

Instead I was thinking of doing something where I can pass the array in each time as an argument and make several classes depending on what I want each part of the app to do. Taking just the part where I wan to add a journey, I thought of something like this:

const arrayOfJourneys = [];

class Journey {
  constructor(name, subject, dateStarted = new Date().toLocaleString()) {
    this.name = name;
    this.subject = subject;
    this.dateStarted = dateStarted;
  }
}

class AddJourney {
  constructor(arr, name, subject) {
    this.arr = arr;
    this.journey = new Journey(name, subject);
  }

  addJourney() {
    this.arr.push(this.journey);
  }
}

document.querySelector("#add").addEventListener("click", () => {
  const add = new AddJourney(
    arrayOfJourneys,
    document.querySelector("#name").value,
    document.querySelector("#subject").value
  );

  add.addJourney();
});

The pros and cons I can see so far

  1. code1 Class will become too big and a lot of the Classes don't actually interact with each other or relate to each other, they all just need the journeys array inside the object created through the Class. I have read about God classes controlling apps and am not keen on this style.

  2. code2 looks similar to an anti pattern I have read about called the Anemic Domain pattern where our classes just contain data. (in this case the array being the Data) although its not in a class it could be and I could call a method which returns it each time. Although reading into it I dont think my code2 above quite fits the Anemic criteria but it has frightened me. I like how I can change the state of this Array inside any other Class If I pass it in.

  3. Code 2 whilst I think is better , my classes will all be verbs such as AddJourney or AddToExistingJourney or sortJourney. I have also read Classes should be Nouns and its behaviours should be the Verbs. I am not sure how important this is.

Perhaps there is another way that I am missing?

Thanks

16
  • 1
    Maybe so, but its only fair I mention the research I have already conducted , the code I have written and my thinking as that constitutes a good question here rather than just state what my problem is and hope someone answers it. Commented Sep 11, 2022 at 3:23
  • 3
    I am not sure what you mean by "a lot of the Classes don't actually interact with each other or relate to each other, they all just need the journeys array". There is only so much you can do with the array of journeys stored in your application (or possibly, also an array of journeys currently shown). Having all these methods go inside a single class doesn't seem wrong. Commented Sep 11, 2022 at 13:14
  • 1
    "my classes will all be verbs such as AddJourney or AddToExistingJourney or sortJourney" - yes, that is a problem. Or more, that all of these classes will have only a single method. In that case, you should just not use classes any more - write a plain function. This would become functional programming or procedural programming though, not object-oriented programming. Commented Sep 11, 2022 at 13:17
  • 1
    "The best way would be to have one big class with various methods all changing the state of the array or sorting it/returning filters from it?" - yes. Since that is what these methods have in common: they're all changing the state of the same array. This is why they belong to the same class according to OOP principles. That they don't call each other doesn't matter. Commented Sep 11, 2022 at 16:47
  • 1
    The filtering methods that actually do not change the state, just return a view on it, could be separated out, e.g. using a getFilteredJourneys method that takes the filter predicate as an argument. Whether this is useful or not depends on how related/unrelated those predicates are, where they are used respectively, and how often each is used. Commented Sep 11, 2022 at 16:48

1 Answer 1

1

I very much dislike code #2. I don't like that it takes raw data as an argument rather than taking another class/interface (which would be dependency injection). I really don't like that it mutates the variable which is passed into the constructor by calling push on the arrayOfJourneys.

I'm also seeing issues with your keypress handler. This calls a journeys.sortJourneys() method which mutates the array of journeys, but will it cause your UI to display them in the new order? Probably not.

my classes will all be verbs such as AddJourney or AddToExistingJourney or sortJourney. I have also read Classes should be Nouns and its behaviours should be the Verbs. I am not sure how important this is.

I would not break this convention. If adding a new journey involves a lot of logic then you could consider a JourneyCreator class. But AddJourney is not a good class.


Mimicking backend code on the front-end

When I am designing front-end code that is a placeholder for backend code I like to mimic the sort of functionality that I would expect to eventually have in the backend. I would create a FakeDb class with methods getJourneyById, getJourneysList, addNewJourney, updateJourney, etc. When you think about it in those terms it might become more clear where to put certain logic.

Your goal is to design this is a way where swapping out your front-end data storage for backend storage involves changing as few pieces as possible. You can have a FakeDb and a RealDbWrapper that conform to the same interface, and simply substitute one for the other.

Here's an example of how a stand-in for an API might look. I'm using lodash orderBy which is a super helpful function for sorting arrays of objects.

I first wrote this TypeScript, React-safe (no mutations) version because that's what I normally write:

import { nanoid } from 'nanoid';
import { keyBy, orderBy, omit } from 'lodash';

interface DbAddedFields {
    _id: string;
    dateCreated: string;
    dateModified: string;
}

interface JourneyUpdate {
    text: string;
    date: string;
}

interface JourneyData {
    name: string;
    subject: string;
    description?: string;
    updates: JourneyUpdate[];
}

type DbVersion<T> = T & DbAddedFields;
type NonDbVersion<T> = Omit<T, keyof DbAddedFields>;

class FakeDbTable<Datum> {
    private ids: string[] = [];
    private itemsById: Record<string, DbVersion<Datum>> = {};

    constructor(items: DbVersion<Datum>[] = []) {
        this.ids = items.map(item => item._id);
        this.itemsById = keyBy(items, '_id');
    }
    getIds(): string[] {
        return this.ids;
    }
    getById(id: string): DbVersion<Datum> {
        return this.itemsById[id];
    }
    getList(): DbVersion<Datum>[] {
        return this.getIds().map(id => this.getById(id));
    }
    add(item: Datum): DbVersion<Datum> {
        const _id = nanoid();
        const dateCreated = new Date().toLocaleString();
        const dbVersion: DbVersion<Datum> = {
            ...item,
            _id,
            dateCreated,
            dateModified: dateCreated
        }
        this.ids = this.getIds().concat(_id);
        this.itemsById = {
            ...this.itemsById,
            [_id]: dbVersion
        };
        return dbVersion;
    }
    delete(id: string): void {
        this.ids = this.getIds().filter(itemId => itemId !== id);
        this.itemsById = omit(this.itemsById, id);
    }
    update(id: string, patch: Partial<Datum>): DbVersion<Datum> {
        const existing = this.getById(id);
        const merged = {
            ...existing,
            ...patch,
            dateModified: new Date().toLocaleString()
        };
        this.itemsById = {
            ...this.itemsById,
            [id]: merged
        };
        return merged;
    }
}

class JourneyCollection {
    private data: FakeDbTable<JourneyData>;

  constructor(journeyArr: DbVersion<JourneyData>[] = []) {
    this.data = new FakeDbTable<JourneyData>(journeyArr);
  }

  addJourney(name: string, subject: string) {
    this.data.add({ name, subject, updates: [], description: '' });
  }

  getSortedJourneys(field: keyof DbVersion<JourneyData>, order: 'desc' | 'asc') {
    return orderBy(this.data.getList(), field, order);
  }

  getJourneysBySubject(subject: string) {
    return this.data.getList().filter(journey => journey.subject === subject);
  }
}

const journeys = new JourneyCollection();

TypeScript playground link

But I think this JS version is more how you were thinking:

import { nanoid } from 'nanoid';
import { orderBy } from 'lodash';

class Journey {
    constructor(name, subject, dateCreated = new Date().toLocaleString()) {
        this.name = name;
        this.subject = subject;
        this.description = '';
        this.updates = [];
        this._id = nanoid();
        this.dateCreated = dateCreated;
        this.dateModified = dateCreated;
    }

    private setModifiedDate() {
        this.dateModified = new Date().toLocaleString();
    }

    addUpdate(text) {
        this.updates.push({
            text,
            date: new Date().toLocaleString()
        });
        this.setModifiedDate();
    }
}

class JourneyCollection {
    constructor(journeys = []) {
        this.ids = [];
        this.itemsById = {};
        journeys.forEach(journey => {
            this.ids.push(journey._id);
            this.itemsById[journey._id] = journey;
        });
    }
    getIds() {
        return this.ids;
    }
    getById(id) {
        return this.itemsById[id];
    }
    getList() {
        return this.getIds().map(id => this.getById(id));
    }
    add(name, subject) {
        const journey = new Journey(name, subject);
        this.ids.push(journey._id);
        this.itemsById[journey._id] = journey;
        return journey;
    }
    delete(id) {
        this.ids = this.getIds().filter(itemId => itemId !== id);
        delete this.itemsById[id];
    }
    // Can update the items directly but modifying the Journey instance.
    getSortedList() {
        return orderBy(this.getList(), 'dateModified', 'desc');
    }
    getBySubject(subject) {
        return this.getList().filter(journey => journey.subject === subject);
    }
}

const journeys = new JourneyCollection();
Sign up to request clarification or add additional context in comments.

12 Comments

I always want to mutate data held in one class by calling a method of another class and passing it in e.g I made a game on Monopoly, I wanted to pass in the current player to a chance method which does something to the player. E.g adds money I can pass the player into a chance method and mutate the instance of the player (created in another class) but has one memory reference but instead I find myself calling Methods from my player class such as player.changeMoney(chance.getMoney()) Instead of chance.changeMoney(player) to mutate the player. Why is the later deemed bad?
Good question. I can't articulate this well, but sometimes it breaks encapsulation for one object to mutate another. The player might have some side effect logic beyond just changing the value of a property. Like "if money === 0, declare bankruptcy". So it's better for your chance class to call a public method of the player object to set the money. Some of my avoidance of mutations is just a habit from writing UIs in React (the UI changes when a variable changes, but it won't change if a variable is mutated). But I don't know how you're generating the UI here.
Thanks. I’m using MVC. In the model I have a chance class and a player class. I call a method from the current chance, the current chance is always the node [0] of the chances array. E.g. Const amount = chance.changeMoney(). I will then pass amount into a method of the player object (which is equal to the current players turn) such as currentPlayer.changeMoney(‘+’, amount) which mutates the money directly inside my players class. Do you think this is the cleanest way? The UI is Dom methods rather than react where I pass in any changes to them player to a method of the Ui class.
Your breaking encapsulation comment is very helpful. I have called methods from other classes before but never to mutate them, mainly just to use or read them!! Would you think it’s fair to say mutating objects of one class from another could lead to buggy hard to fix code? Whereas if we encapuslate all mutations , then it’s easier to see where things go wrong ?
Yes, I agree with those statements. What you are describing re: chance sounds good. The chance object tells the player object "I am giving you x dollars" and it's up to the player class to actually increment the total.
|

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.