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

Project Code Cleanup - Improve Code Quality #74

Open
10 tasks
epsi1on opened this issue Aug 14, 2020 · 9 comments
Open
10 tasks

Project Code Cleanup - Improve Code Quality #74

epsi1on opened this issue Aug 14, 2020 · 9 comments
Assignees
Labels
Conversation Conversation about technical stuff of a new feature

Comments

@epsi1on
Copy link
Member

epsi1on commented Aug 14, 2020

EDIT:

Milestone 1

  • BriefFiniteElementNet / BriefFiniteElementNet.Common
    • Remove obsolete code.
    • Fix not fully implemented classes that use Obsolete attribute.
    • Inspect Unreachable code detected warnings and fix or comment what to do.
  • Add .editorconfig to maintain a consistent coding style.

Milestone 2

  • BriefFiniteElementNet / BriefFiniteElementNet.Common
    • Add unit tests for public (and internal) classes.
    • Add XML comments to public classes.
    • Add more XML comments (internal or even private classes).
  • BriefFiniteElementNet.Validation
    • Remove BriefFiniteElementNet.Controls dependency

Milestone 3

  • BriefFiniteElementNet / BriefFiniteElementNet.Common
    • fix TODO items
  • BriefFiniteElementNet.Validation
    • fix TODO items
      ======================

discussion continued from comment number 673732028 of #36 (link)

  1. I think having a good coverage is crucial to ensure that people contributing code do not unintentionally break anything. OpenCover reports the following code coverage, which is (expectedly) low:
    Line coverage: 13.4% (2266 of 16832)
    Branch coverage: 13.0% ( 494 of 3788)

Yes i also think that unit tests are not enough yet, only some element helpers have unit tests, others do not. So is it necessary to increase the unit tests? i think there is need for it

  1. When building the solution, Visual Studio reports 345 warnings, some of them warning about obsolete code usage, which should easily be fixed, but also many obsolete warnings, telling a feature is incomplete/not working. Such code should not be distributed via nuget (meaning not be part of the main project or master branch).

I've used [Obsolete()] attribute for both deprecated codes and new codes which are under development and not tested yet. As you stated, I'm also waiting to get an stable version then publish that version in nuget.org.

  1. Building the solution with enabled generation of documentation, 1185 warnings are reported. So there's also a massive lack of documentation, which might discourage people to contribute code (because they don't understand the intention of the code that's already there).

You mean XML documentation for methods and fields and property ?

Thanks

@epsi1on epsi1on added bug Conversation Conversation about technical stuff of a new feature and removed bug labels Aug 14, 2020
@epsi1on epsi1on changed the title Project Code Cleanup Project Code Cleanup - Improve Code Quality Aug 14, 2020
@wo80
Copy link
Contributor

wo80 commented Aug 14, 2020

EDIT: removed task list.

@epsi1on epsi1on self-assigned this Aug 20, 2020
@wo80
Copy link
Contributor

wo80 commented Aug 20, 2020

You can either remove Milestone 3 or add some specific tasks (one addition - with rather low priority - could be to inspect the TODOs scattered across the projects).

I think you should put the heading Plan towards BriefFiniteElementNet version 2 and a nuget release back in place (or change the title of the issue). All the tasks should be addressed before publishing a final nuget package. Of course you can publish prerelease packages - as you might have noticed, I added <Version>2.0.0-pre</Version> when converting the project files. The pre means nuget will handle it as a prerelease.

@epsi1on
Copy link
Member Author

epsi1on commented Aug 21, 2020

Maybe need to make a new project with title Plan towards BriefFiniteElementNet version 2 and a nuget release?
First we should address stuff that need to be done before release nuget package...

@wo80
Copy link
Contributor

wo80 commented Aug 21, 2020

  • Having a project to track the progress sounds like a good idea.
  • You can publish a prerelease/alpha/beta package at any point. The final package should address all tasks that we define in this issue. It's up to you to decide what the final release should look like, so feel free to add or remove tasks.
  • Each milestone should be finished before working on the next milestone starts. Obviously, you don't want to write tests or documentation for code that will be deprecated.
  • For each task defined, details could be discussed in a separate issue.

@epsi1on
Copy link
Member Author

epsi1on commented Aug 23, 2020

I rather to move obsolete classes into another namespace, and remove them in next version, what you think about it?
For integration tests, you mean it should be automatically tested by visual studio? currently it is a console application...

@epsi1on
Copy link
Member Author

epsi1on commented Aug 23, 2020

@wo80
Copy link
Contributor

wo80 commented Aug 23, 2020

I rather to move obsolete classes into another namespace, and remove them in next version, what you think about it?

If you think that the obsolete code is still widely used, that would be a fair compromise. Alternatively, you could add another project BriefFiniteElementNet.Compatibility, which could even be distributed via nuget, if anybody feels the need.

For integration tests, you mean it should be automatically tested by visual studio? currently it is a console application...

If I'm correct, at the moment it's a loose collection of tests, which must be run manually. Just add a class TestRunner (or the like) which calls all those tests and gives a hint if anything doesn't work as expected. I will start working on the Matrix class, once we have this.

@epsi1on epsi1on pinned this issue Aug 28, 2020
@rubsy92
Copy link
Contributor

rubsy92 commented Sep 30, 2020

Hi,

I'm going through the todo-list and I noticed item "complete TriangleElement codes that support all 3 types of loads.": https://github.com/BriefFiniteElementNet/BriefFiniteElement.Net/projects/4#card-44141482. What do you mean with all 3 types of loads?

@epsi1on
Copy link
Member Author

epsi1on commented Sep 30, 2020

There are 3 type of elemental loads, Uniform, Concentrated and PartialNonUniform. I think currently only uniform load is supported by triangle element.
At least IElementHelper.GetLocalEquivalentNodalLoads() method should be implemented for both DkqHelper and CstHelper and the code should support ConcentratedLoad and PartialNonUniformLoad .

@epsi1on epsi1on unpinned this issue Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Conversation Conversation about technical stuff of a new feature
Projects
None yet
Development

No branches or pull requests

3 participants