Skip to content

Conversation

@dmitrizagidulin
Copy link
Contributor

  • Wrapped code to 80 chars
  • Extracted nested inner functions to module-level scope in lib/ldp.js

}
})
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure that taking this function out is a good idea.
They have one particular purpose bound to one particular function, putting them out is a bit confusing.
I think I like them being well encapsulated in the scope of the function. Also, these functions may even disappear or should be in a library (like folder-to-rdf was initially)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defining multiple nested functions inside a function, and then calling them, is a pretty serious anti-pattern tho.
Feel free to move em to a separate file or module tho.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, it doesn't feel right.
Putting out a function in the middle of many others, doesn't feel right either.

Moving them in a file sounds great. Recently I keep on seeing this (the anti-pattern that you mention) happen more and more in other javascript projects that I am following

@dmitrizagidulin
Copy link
Contributor Author

@nicola how's this? I moved the inner functions in question to their own module file, ldp-container.js. (We're gonna refactor these files anyway when we move to a modular backend API).

@nicola nicola merged commit f1a62f3 into master Apr 27, 2016
@nicola
Copy link
Contributor

nicola commented Apr 27, 2016

Perfect @dmitrizagidulin!

@dmitrizagidulin dmitrizagidulin deleted the dz_refactor branch April 27, 2016 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants