Skip to content

Re-integrate _check_array() into _read_or_write()#138

Closed
mgeier wants to merge 1 commit intomasterfrom
reintegrate-array-check
Closed

Re-integrate _check_array() into _read_or_write()#138
mgeier wants to merge 1 commit intomasterfrom
reintegrate-array-check

Conversation

@mgeier
Copy link
Contributor

@mgeier mgeier commented May 18, 2015

This reverts a part of #36, namely ab33e66.

@bastibe
Copy link
Owner

bastibe commented May 18, 2015

Any justification for this? (Not that I'm against it, I'd just like to know your thoughts on this)

@mgeier
Copy link
Contributor Author

mgeier commented May 20, 2015

The justification is in #36, mainly https://github.com/bastibe/PySoundFile/pull/36#issuecomment-44213305 and https://github.com/bastibe/PySoundFile/pull/36#issuecomment-44533741.

At the time I wasn't quite sure, so I followed your wish to split the function into two parts.
In the meantime I got more and more sure, and now I know that splitting the function was wrong.
The reasons stayed the same, though.

@bastibe
Copy link
Owner

bastibe commented May 20, 2015

I am not opposed to this.

But let's take this opportunity to talk about _read_or_write, then. This is something I wanted to change for a long time. It is absolutely terrible design to have one function that either reads or writes depending on a string parameter.

There has to be a better way. Maybe we can separate the snd-call from the data conversion/normalization somehow. Maybe we can pass in functions. But reading and writing are such fundamentally different operations, they should not live in the same function.

@mgeier
Copy link
Contributor Author

mgeier commented May 21, 2015

But let's take this opportunity to talk about _read_or_write, then.

This has nothing to do with the current PR, but ... whatever ...

It is absolutely terrible design [...]

There is no design without boundary conditions.

The result may be ugly, but if it is the least bad solution to a given problem, its actually good design!

But I'm not saying that it is ...

There has to be a better way.

Quite likely ... there nearly always is ...

[...] reading and writing are such fundamentally different operations, they should not live in the same function.

They may be conceptually different, but pragmatically, they are the same:

sf_count_t  sf_readf_double  (SNDFILE *sndfile, double *ptr, sf_count_t frames) ;
sf_count_t  sf_writef_double (SNDFILE *sndfile, double *ptr, sf_count_t frames) ;

The only difference is a substring in the function name, which we pass as string argument into our wrapper function.
Seems perfectly straightforward to me.

As I mentioned in https://github.com/bastibe/PySoundFile/pull/36#issuecomment-43413445, we could change 'sf_readf_' to 'read' when calling the wrapper function.
This might look nicer, but only marginally.

Alternatively, we could have two different dicts (one for "read" and one for "write") which map the FFI type to one of the function objects in _snd.
Then, instead of the function substring, we could pass one of those dicts into the wrapper function.

I doubt this will turn out less terrible, though.
It will definitely be more code which would verbosely do the same thing that the string concatenation does concisely.

Either way, this won't be a solution to your problem, because there would still be one function that does both reading and writing ...

Currently, I don't see a nice way to use separate functions for reading and writing without code duplication.

But I'm looking forward to a better solution!

@bastibe bastibe mentioned this pull request May 22, 2015
@mgeier mgeier added this to the 0.8.0 milestone Sep 27, 2015
@mgeier
Copy link
Contributor Author

mgeier commented Sep 28, 2015

After merging #72, this PR is obsolete, I'm closing it ...

@mgeier mgeier closed this Sep 28, 2015
@mgeier mgeier deleted the reintegrate-array-check branch September 29, 2015 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants