Applied 'Extract Method' to read()

[david-sackstein]

This change extracts methods from basic_file_base::read(std::size_t size).
In my opinion this breaks down the logic into independent pieces that are easier to reason about.

1. It explicitly deals with the case when size==0. The logic is different and should be separate.
2. It explicitly deals with the case when size==0 and file.size() = 0. The logic is different.
3. There is no code duplication (read_non_zero() calls read(byte_view).
4. Encapsulated the logic of get_bigger_size(). Nit. Handled the case where curr_size < 4

True, the file is now longer by 28 lines.
Indeed, in my opinion, some of this logic should be extracted to other headers which should be included by this one.

[eyalz800]

@david-sackstein This is an awesome work, I made some comments here and there but feel free to leave the fixing to me, I will make the necessary changes soon (because I just started to study for my University exam).

[eyalz800]

I am taking this idea and have two read overloads, one for reading the entire file and one for reading a specific size. (That is read() and read(std::size_t)).

[david-sackstein]

Good. But sometimes I think it is better to be explicit.
I propose read_all().
This is similar to the train of thought that encourages us to write explicit static factories instead of relying on overloaded constructors. Sometimes you can get away with it, but often the variety of argument types is not enough to describe what the method does.

[eyalz800]

This is not dealing the case where the file is growing along the way. Most likely it can't do any harm but the fact that it is not waiting for the real end of file is troubling me.

[david-sackstein]

Can you be more explicit? What is the scenario and what should the method be doing in that case that it is not?

[eyalz800]

When the file is growing larger while you read it, this would only read a fixed amount of bytes (the amount of bytes before the grow) and not read until the EOF.

[eyalz800]

Calling tell twice seems risky, in any case, can optimize the two tell calls into one.

[david-sackstein]

Of course. My bad.

[eyalz800]

I would say enlarge_bytes rather than get and then do it from the outside.

[david-sackstein]

Do you mean - to replace:
data.resize(get_bigger_size(data.size())); (1)
with
enlarge_bytes(data); (2)
I agree. But personally I still prefer separating the logic of get_bigger_size() and data.resize().
So I might add a method called enlarge_bytes whose implementation is (1) rather than inline get_bigger_size into enlarge_bytes().
I am a SRP guy ; )

[eyalz800]

I would really like to avoid this other read call.

[david-sackstein]

OK. But a comment might be in order.

[eyalz800]

@david-sackstein

1. I added your change about read_zero and read_nonzero as separate overloads, then I realized there is no need to rename them, so both are named just "read", one is read(std::size_t) and the other is just read(). I wonder about the other changes:
2. The fix to reading 4GB on a 32 bit platform requires another read which makes the code less pretty where we both know it is not going to work as the process address space is exactly 4GB.
3. Given point 1 I think the code for read(std::size_t) is now ok, and the read() function remains a little long but handles less cases. In my opinion, separating the read() function further into private functions is going to hurt the readability. It will reduce the readability significantly because the functions will handle a niche logic that will have the reader jump between them to understand the flow.