icssc / AntAlmanac

A course exploration and scheduling tool for UCI Anteaters
https://antalmanac.com
MIT License
58 stars 72 forks source link

Extract helpers.ts #764

Closed MinhxNguyen7 closed 1 year ago

MinhxNguyen7 commented 1 year ago

Summary

Extract all course/WebSOC-related functionality to the WebSOC class in $lib/websoc.ts. There should be no change in behavior

I do not sleep with OOP, but my God is this better than the garbage can that was the old helpers.ts.

Test Plan

Check that everything works. I can't get too much more specific than that.

This PR mostly touches on WebSOC data request, caching, and manipulation functionality, so checking if things load properly is the priority.

Issues

Closes #760

ap0nia commented 1 year ago

I don't think it makes any sense to make one class to encapsulate private functions. You could accomplish the exact same thing by not exporting the private functions from the module. There's no benefit to OOP because instances of the websoc query class don't inherently manage any individual data, this is a perfect scenario to apply procedural programming because all you're doing is processing data in -> data out with not many side effects. (There aren't even multiple instances, it's just one).

Am I missing something?

EricPedley commented 1 year ago

The websoc class also has its cache, which makes sense as a private field and changes as a side effect when the public methods are called. I don't really see the downside of this approach vs a module with functions we don't export

ap0nia commented 1 year ago

If that's the only benefit of having this class, then I think it would be clearer if everything was static. For example:

export class Websoc {
  private static cache = {}

  public static async query() {
    // ...
  }
}

// Usage:

Websoc.query(...)

In my opinion, I just dislike this illusion of encapsulation. The constructor doesn't actually do anything unique per instantiation, and the class itself isn't used in a way that actually takes advantage of OOP; it's literally just a couple of methods that are invoked like regular functions, but now you need to say websoc.query(...) instead of queryWebsoc(...), which seems like you should be aware of the extra class logic in class Websoc (which, counterintuitively, there isn't any --aside from the cache).

I think static would be a valid compromise because it's "closer" to the level of module instantiation in that it doesn't readily support unique behavior per instance, and can exist just to encapsulate variables and mutate them freely (since imports/exports are immutable by the ES6 standard).

Also, we should migrate to Tanstack query eventually :tm:, which handles rendering and caching. So the most ideal scenario in the future is that these are just procedural functions that fetch data for the library's hooks, and the library handles caching, mutations, and other server-side state altogether. (i.e. the endgoal is to not have a class).

MinhxNguyen7 commented 1 year ago

This is equivalent to using a static class. The interface would be the same. Again, I'm not married to OOP; there's no reason a singleton is better than static here.

It should be a class because it has state and side effects. Every function accesses it. It's not really a valid point to say, "well it doesn't have that much state", because having state just sitting around in the module is unclear in the opposite direction. It's not just data in -> data out, so that shouldn't be indicated.

Also, I strongly prefer websoc.query() because of the namespace. It's much quicker to read. It makes it clear that all interfacing with WebSOC is done here and nothing else.

This is not about encapsulation or safety in some way. It's cleaner and clearer.

I'm not familiar with Tanstack. What's the advantage there?