In an overall sense, it's pretty ovious that the person/people who wrote this wrote it under the assumption that there was sort of one way of writing it, so a lot of this seems shoehorned into a "dictated style." I'm a bit wary of that to begin with - you shouldn't be figuring out how to make your tools do the job, you should be figuring out the best tools for the job. (It's also obvious that there's no real technical lead in their group who oversees their coding and applies some sort of standards to what they're doing.)
For example:
-
Their code comments seem to be literally only commented-out code that didn't work for whatever reason. Why is this? This is a HUGE red flag. If I'm building code that I know will be passed off to someone else to maintain, I comment the shit out of that code. This way it's readily apparent what the code does. I don't just leave it up to "they'll figure it out." That doesn't speak well to my own understanding of my responsibility as a vendor.
-
Why are there any backbone models or collections in this at all? It looks like that all they really want out of it is events, so why not just import and extend only backbone events, or hook into some other pub/sub-type system?
-
It appears they don't really understand the purpose of React's
state- they use it in limited places and instead go with jQuery-driven dom manipulation. This is a huge red flag. React handles their own DOM manipulation in a 10x better way than jQuery does. They also seem to want to create aconstructormethod in every object, for the sole purpose of adding the minimalstatethat they do use. There's already agetInitialStatemethod for that purpose. Which means they don't really know React, they're just using it because either someone told them they should, or they read how it's really cool. -
They have template code wrapped up inside the view code. While it's not unheard of to do this, it definitely makes it harder to read a file (and makes me wonder if they know how to use external templates). It's basic separation of concerns.
-
They're adding object defaults for one class inside other classes, which itself is weird (e.g. https://github.com/articulate/articulate/blob/master/src/scripts/AuthorComponents/lib/Editor.jsx#L17 - why is there not a MediumEditor defaults object inside medium-editor?)
-
Why are they hard-coding sample content? Any sample content, or really any content which is going in to a view, unless it is really small, should be in a separate file. (e.g. https://github.com/articulate/articulate/blob/master/src/scripts/Components/lib/Sidebar.jsx#L87)
-
There is so much un-DRY code here. There's not really much more to say about it. In some places they are taking 100 lines of code to accomplish what they could do in 15.
-
I hate to say it, but please note that this isn't all of the stuff I found. It's a list of some of the more glaring high-level examples.
In a larger sense - why are we paying for a Rise team? Is it to have someone to maintain their code? If all we're looking to do is maintain their stuff after they're done writing it, let's can the rise team and get some contractors - because that's not a full-time job. I understand that Ueno is more comfortable working that way, that's "just how they do things," or whatever - but the ultimate argument we have against that is that while that might be fine for others, we expect a higher standard of code and they're not delivering it. I get that they have a lot to do in a little time, and as such they're looking to get it done fast - but if they're also looking to deliver code, then they need to step back and work on getting that code right. And since we have a team of people sitting around looking for something to do, then maybe we should be figuring out how to work with Ueno to have the best of both worlds - they get their designer to work closely with a developer, and we get to develop with the quality of code that we expect. Hell, I'd go as far as to say we might even want to put up a dev out in SF for a few days a week to work in the office with them, if what they're looking for is an in-house dev that their designers can work with.
Also, and this is important - don't think that this is only about code quality for code quality's sake. The quality of code is directly related to technical debt and how much more effort we want to spend later, once we've decided to make some changes to Rise. In the current situation, we're going to spend as much as 200% the effort we would if we just get the code right now. So if you're willing to spend (just throwing out bullshit numbers here) half a million getting updates right, ask yourself - would it be worth a million? Because you could be looking at that.