Refactor question

[cjbarth]

@markstos I would like your honest opinion on these PRs (#129, #130) before I continue. These PRs are based on the efforts of @forty in #19. Do you think this adds to, or detracts from, the maintainability, debug-ability, comprehensibility, or any other aspect of the code by breaking it up like this?

I note that in VS Code "Find All References..." no longer works as one might expect when the code is refactored into multiple files. I'm not sure if losing that is worth any other gains we might get. After seeing how this looks so far, I'm starting to see how this might not be the 100% win it seemed it might be.

@srd90, @zoellner, @sibelius, @josecolella, @gugu since you also spend a lot of time with this code, I'd value your opinion as well.

[cjbarth]

If I don't get any feedback by this weekend, I'll revert these changes and close the project/PRs to separate functions out of the SAML class. I just can't see the benefits aside from the function to generate metadata, which one may reasonably want to do without newing up the SAML class, and which is already separated out.

Are there any other functions in the SAML class that one may want to use without newing up the SAML class?

[markstos]

I'm looking at this today. Sorry I missed this earlier, I have trouble keeping up with the mail from this very active project.

[markstos]

I see a couple related concerns trying to be addressed here.

File length

I don't think there's great virtue in large files or small files. If a file is very long, it could indicate that perhaps a big problem should be broken into smaller problems. A large file might invite over-use of shared state.

But neither of those things have to be true. A larger file can be well-organized. It might even contain multiple smaller classes. And it may be easier to navigate with all the code in one place. There's also some risk than any big change introduces some undesirable behavior.

On this point, I defer to @cjbarth as the primary maintainer. He mentioned there's some productivity benefits with VS Code to having fewer files.

Reducing State

I think this is the more interesting goal raised by @forty in #19. Here's his description of that motivation:

In those files, I would like to avoid having SAML class (ie this or self) or SamlOption as parameter, as I feel it makes it hard to understand the input of the function. Instead, each function would take as input exactly what it needs to operate. I think this makes review much easier. It also often allow to have stricter types.

I've heard it say about software that "state is the enemy". The more states that are possible, the more complexity, the more bugs, the more testing that is required.

Having a reviewed a lot of code over the years, it's much easier to understand what the former does then the latter:

// minimal state
foo(first,second)

// unknown state
this.foo()

The compromise for OO code is immutable state. Once the object is created, the state is locked and can't be changed.

// compromise: object with immutable state
this = new Object(first, second)
this.foo()

As a security-critical project, having correct code that's clear to write and review is important.

The proposal in #19 didn't just split things up to files, it simplified some function signatures so that the functions required less state. I think that's a win. But the same benefits could be had by keeping the refactored functions in a larger file.

The logout.ts proposed by #19 includes a couple utility functions which are only used by that file, which helps justify breaking out this work into a smaller file.

The metadata.js just moves a single function into it's own file, which doesn't seem to have much benefit.

The work in those files to simplify function signatures seems worth considering, though.

[cjbarth]

Since this project is very much OO with immutable state, it seems reasonable not to break out any functions that depend on that state. I can see your argument for breaking out the functions, but that would mean that each function would need validators for the incoming values.

I'm therefor inclined to keep all the functions that depend on the immutable state to continue to be a part of the SAML class and let someone else break them out if they are needed separately. I think one problem that @forty might have been trying to solve is that we had some functions in saml.ts that weren't part of the SAML class, and yet consumed the state of the SAML class. That I agree was a problem and I've already taken care of it.

I'll continue to wait for more feedback before offering my final recommendation.

[markstos]

@cjbarth given your comment, I've inclined to agree your proposal for how to proceed.