Skip to content
This repository was archived by the owner on Mar 14, 2020. It is now read-only.

[Serialize] Export updateHeaderFile in $.rib#203

Closed
DonnaWuDongxia wants to merge 1 commit into
intel:masterfrom
DonnaWuDongxia:update-header
Closed

[Serialize] Export updateHeaderFile in $.rib#203
DonnaWuDongxia wants to merge 1 commit into
intel:masterfrom
DonnaWuDongxia:update-header

Conversation

@DonnaWuDongxia

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread src/js/serialize.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can use "array = $.merge([], design.getProperty(property)) to copy an array instead of using extend.

@DonnaWuDongxia

Copy link
Copy Markdown
Contributor Author

Updated!
Hope this pull request can be merged quickly.

@grgustaf

Copy link
Copy Markdown
Contributor

Apparently I didn't post my comment here before I closed the window. :(

Sorry to be a stickler here, but the function updateHeaderFile() reminded me of a topic from a book, probably either "Writing Solid Code" or "Code Complete", some old Microsoft classics. The point of the topic was that functions should have a single purpose, not try to combine multiple behaviors into one. This gives more room for bugs, and makes the code less self-documenting. I believe that applies very well here: it would be much better to have addCustomFile / removeCustomFile / replaceCustomFile functions, than put all behaviors in one here. Each could then validate the inputs with its particular behavior in mind.

When you are reading a call to "updateHeaderFile", you have no idea what it is doing until you look into the detail of the implementation or at least description. But with the three functions above, you can surmise what they are doing.

@DonnaWuDongxia

Copy link
Copy Markdown
Contributor Author

Replaced by #206

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.

3 participants