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

Standardize Layer code in the LEL repository #198

Closed
ananyaarun opened this issue Jun 6, 2019 · 16 comments · Fixed by #207, #233 or #237
Closed

Standardize Layer code in the LEL repository #198

ananyaarun opened this issue Jun 6, 2019 · 16 comments · Fixed by #207, #233 or #237

Comments

@ananyaarun
Copy link
Member

Description

LEL is a growing library with a set of layers. Currently the process of adding new layers is not easily described and repo contains redundant code for different layers.

What can be done

  • find similarities in code btw layers
  • remove redundant code and pass layer type as parameter to a standard layer display code
    (common function which could be used to reduce redundant code in layers)
  • refactoring based on common functions
@ananyaarun
Copy link
Member Author

@sagarpreet-chadha , while some layers have very similar code (almost same ) I noticed there are a few eg fractrackermobile layer, wisconsin layer etc whose functions are pretty different.

@ananyaarun
Copy link
Member Author

ananyaarun commented Jun 6, 2019

@sagarpreet-chadha I guess one kind of custom code can be used for layers of a similar kind ie Mapknitter Layer SkyTruth Layer PurpleLayer Layer Toxic Layer etc, where we can have one function to add layer that takes its URL as a parameter ? Let me know if there are better ways to implement this. I feel this enhancement will take me a while , so I want to get started with it as soon as possible.

@ananyaarun
Copy link
Member Author

@sagarpreet-chadha ,do you have any ideas/ guidelines in mind for the standardization ?

@sagarpreet-chadha
Copy link
Collaborator

Let's start the discussion keeping Jeff's idea here 👍 #168 (comment) in mind .

One possible approach is to make one custom class/function and pass parameters like URL , API parsing info. to it and call it for each layer ?
Also making UI code separable and making functions for UI like position of layer menu , ability to show number of active layers on this button , function to give popup styling maybe , when more than 1 default layer is added then layer button should come automatically , etc.

So let's get started by first making this custom class (leaving the layers that have different code) . What do you think ?

@sagarpreet-chadha
Copy link
Collaborator

As we are extending Leaflet class to make layer currently , right? Let's implement this custom class by putting it inside a function which will recieve various parameters (as described in above) .
Then removing the redundant code from each layer and use this new custom universal function .

@ananyaarun
Copy link
Member Author

ananyaarun commented Jun 10, 2019

makes sense.
Right now there are many files with the same type of code (for the similar layers)
So after the refactoring , according to my understanding there would be one file along with other files with different layer code and from index.html we can call its functions by passing parameters here L.layerGroup.commonfunction(URL,API ....etc) ;
Again this would be applicable for most layers and we can reduce the number of files with redundant code hereby.

@sagarpreet-chadha
Copy link
Collaborator

Yes that is correct 😄
@jywarren what do you think ?

@jywarren
Copy link
Member

This sounds SUPER

@ananyaarun
Copy link
Member Author

Reopening because other layers are yet to be standardized.
I'll try to look for the next most similar layers and follow a similar method for their standardization.
Thanks!!

@ananyaarun
Copy link
Member Author

Hey @sagarpreet-chadha @rexagod @jywarren

we now have 2 standardized file

I noticed that some more layers for example

  • luftdatedlayer
  • openaqlayer
  • opensense layer
  • pfas layer
  • purpleairmarkericon
  • toxicreleaselayer

can be standardized with the same file used to standardize these : mapknitter, skytruth, odorreport and fractracker.

Shall I go ahead with this, or will it make that one file too complicated ?
Another option would be to standardize layer code for about 4-5 layers in one file ...
Let me know whichever is more suitable
Thanks!!

@sagarpreet-chadha
Copy link
Collaborator

Hi @ananyaarun ,

If we try to understand the things that are unique in all these layers are :

  1. URL of API
  2. Parsing function of API
  3. Marker code

right?

Can we make one file that accepts these 3 things as arguments ?
Then for each layer we just have to call this file's function and pass 3 arguments . Makes sense?

Thanks!

@ananyaarun
Copy link
Member Author

ananyaarun commented Jul 13, 2019

Hey @sagarpreet-chadha , true this can be done and this was my initial idea as well.
But a few problems with this method are -

  1. Marker code is too long (20-30 lines in some cases ) to be passed from the layer.js file.
  2. Some layers have certain extra functions that use/modify the same variables. For other layers these should not be modified. So we will have to use a if else structure no matter what to ensure these functions only act on required layers
  3. The layer.js file in example directory would become too messy if we pass all these information about each layer there.
  4. For many layers the code for adding marker, parsing data, getting the popup location etc is different as well. These differences are small though

@ananyaarun
Copy link
Member Author

ananyaarun commented Jul 13, 2019

I was thinking of like extending my previous PR for standardization to the similar remaining layers as well.
Like we pass just the layer name and everything else is taken care of in the file ie layercode.js file.

Let me know what would be better 😃

@rexagod
Copy link
Member

rexagod commented Jul 13, 2019

@ananyaarun Would that solve your problem? Loving the fact that you're considering making things dynamic, very cool! Let me know any specifics you'd like me to look at, though, I guess you've already got this figured.

Also, whenever in doubt about how to code things, feel free to refer this, I find this suprisingly comprehensive yet brief (The Zen of Python by Tim Peters, in case you're wondering). Happy coding! 👍


Beautiful is better than ugly.
Explicit is better than implicit.
Simple is better than complex.
Complex is better than complicated.
Flat is better than nested.
Sparse is better than dense.
Readability counts.
Special cases aren't special enough to break the rules.
Although practicality beats purity.
Errors should never pass silently.
Unless explicitly silenced.
In the face of ambiguity, refuse the temptation to guess.
There should be one-- and preferably only one --obvious way to do it.
Although that way may not be obvious at first unless you're Dutch.
Now is better than never.
Although never is often better than right now.
If the implementation is hard to explain, it's a bad idea.
If the implementation is easy to explain, it may be a good idea.
Namespaces are one honking great idea -- let's do more of those!

@sagarpreet-chadha
Copy link
Collaborator

This is super cool ❤️ !

@ananyaarun
Copy link
Member Author

Thanks @rexagod !!
@sagarpreet-chadha also do you think we can extend my previous PR and follow the same method for the rest of the layers ?
Like this one -
#233

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment