Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DRYed GeoFeatureModelSerializer Chapter 2 #81

Closed
wants to merge 1 commit into from

Conversation

pglotov
Copy link
Contributor

@pglotov pglotov commented Sep 13, 2015

This PR is an attempt to DRY out the GeoFeatureModelSerializer: main field loop is done in the DRF base class ModelSerializer, GeoFeatureModelSerializer only re-shuffles and adds geo specific fields. Also provides hooks to modify properties field. Continuing from #76

@nemesifier
Copy link
Member

@pglotov I worked a few hours last week to merge #71 (which included usage documentation), that means I rejected #76 in favour of #71 and I optimized it to gain some performance. I don't want to go back working on this unless you do 2 things:

  • explain properly why this solution is better
  • run a few benchmarks and compare it to current master

@pglotov
Copy link
Contributor Author

pglotov commented Sep 14, 2015

@nemesisdesign Current implementation of gis serializer goes over model fields and computes their representations. This functionality is already present in the base class ModelSeializer. It is considered a good practice in software design to not repeat functionality already present elsewhere, but instead reuse this functionality (DRY principle - Dont Repeat Yourself). This goal can be achieved with help of existing design principles and methodologies, one of them being inheritance from Object Oriented Programming. This PR reuses functionality of base class to loop over model fields, instead of re-implementing it. This leads to shorter, easier to maintain code base (total number of lines in serializer is reduced by 37 lines, or about 25%).

As for performance, when running testsuite it prints out time of performing serialization of 10K or so instances. The time values for current master are the same as for this PR.

@nemesifier
Copy link
Member

37 lines? I added quite a lot of comments that you are removing.
You also changed the name of a public method that is already documented and ready to be released.

If you want to improve something start again from master, do not remove any comment, do not rename public methods, and ensure that what is documented works (get_properties must be able to access the instance object in order to provide the flexibility that was intended in #71).

@nemesifier nemesifier closed this Sep 16, 2015
@openwisp openwisp locked and limited conversation to collaborators Sep 16, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants