Why not more object oriented?

[scd31]

I noticed a lot of methods are used like this:

fast_paths::a_method(&graph, a, b, ...)

Whereas a more OO method call would look like this:

graph.a_method(a, b, ...)

Is there any reason the first is used so often? I would like to send in a PR to move over to the second way, but want to make sure it would be accepted.

[easbar]

Can you give an example for such a method?

[scd31]

For instance, old:

// prepare the graph for fast shortest path calculations. note that you have to do this again if you want to change the
// graph topology or any of the edge weights
let fast_graph = fast_paths::prepare(&input_graph);

// calculate the shortest path between nodes with ID 8 and 6 
let shortest_path = fast_paths::calc_path(&fast_graph, 8, 6);

New:

let fast_graph = input_graph.prepare_fast_graph();

let shortest_path = fast_graph.calc_path(8, 6);

[easbar]

let fast_graph = input_graph.prepare_fast_graph();

That makes little sense to me. The input graph is just a data structure and has nothing to do with the fast graph preparation or at least I see no reason to couple the input graph to the fast graph preparation like this.

let shortest_path = fast_graph.calc_path(8, 6);

This might be reasonable, but what about the usage pattern with PathCalculator? The calc_path calculator really belongs to the PathCalculator class, why should it be available via fast_graph as well?

I don't know I do not see a big improvement in changing these the way you propose. But maybe you can explain a bit how this would help or make things easier for your use case? And maybe have a second look at lib.rs. These fast_paths::xyz functions are really just high level util functions that use the underlying classes of this crate. Maybe you can just use the library classes directly?

[scd31]

Changing the prepare_fast_graph method to be an instance method of input_graph does not make it coupled. It is already coupled because it requires taking in input_graph. Rather, it makes this coupling more obvious, and thus easier to read.

The PathCalculator should ideally be redone as well. The fast_graph is passed in both when the calculator is created, and also when the path is calculated. Imo it would make a lot more sense for it to only be passed in once. It also makes it harder for the programmer to make a mistake - what if they pass one fast_graph in when creating the calculator, and pass in a different fast_graph when calculating the path?

[easbar]

Changing the prepare_fast_graph method to be an instance method of input_graph does not make it coupled. It is already coupled because it requires taking in input_graph. Rather, it makes this coupling more obvious, and thus easier to read.

Hm, prepare_fast_graph() is already coupled to the input graph yes, but not the other way around. Its just a data structure and it does not depend on its users or client code. Imagine there was more code related to input graph. Maybe there would be some routines to do some pre-processing of the graph, or some code to store it on disk, serialize it etc. With this logic input graph would have all these methods: input_graph.pre_process, input_graph.save_to_disk etc. So everything would be coupled to it. It would be like attaching all kinds of methods to Vec just because you can do many things with a Vec.

The fast_graph is passed in both when the calculator is created, and also when the path is calculated.

The path calculator constructor only takes an integer input (the number of nodes of the graph). But its true that the number of nodes passed into the constructor must match the actual number of nodes of the fast graph used in calc_paths later on. This is not ideal, yes. But this brings up the question who "owns" the fast graph instance and since there are typically multiple path calculator instances (e.g. one per thread) I did not think path calculator should own the fast graph. Or would you keep just some kind of reference to the fast graph (passed via the constructor)? I think I tried this briefly but felt like adding the complexity of lifetimes and borrowing objects was not worth it.

what if they pass one fast_graph in when creating the calculator, and pass in a different fast_graph when calculating the path?

This case is covered by this assert. So there will be an error indicating the wrong usage pattern:

fast_paths/src/path_calculator.rs

Lines 61 to 65 in f00b621

	assert_eq!(
	graph.get_num_nodes(),
	self.num_nodes,
	"given graph has invalid node count"
	);