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

Cosmetics and small refactoring #14

Merged
merged 12 commits into from
Jan 21, 2023
Merged

Cosmetics and small refactoring #14

merged 12 commits into from
Jan 21, 2023

Conversation

telephon
Copy link
Contributor

@telephon telephon commented Jan 19, 2023

  • no need to make class methods here
  • move assets to a better place
  • and just some formatting

Copy link
Member

@capital-G capital-G left a comment

Choose a reason for hiding this comment

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

  • no need to make class methods here

In other languages its best practice to make use of static methods as much as possible if they are deterministic because they can not modify the object (and there is a neglectable speed boost)

  • no need to make them private

I consider them a perfect example of something that should be private static method - they are of no concern to the actual test as they are internal, deterministic helper methods and should therefore not be exposed in autocompletion or docs.


I currently find the path layout rather confusing as the test files are at a different location (why is this a convention in SC? makes mirroring folder layouts necessary on bigger projects - java does this but this is due to their namespacing rules on filenames which SC does not have).
the asset files for the test yet remain in the classes folder.

Tests/TestJSONlib.sc Outdated Show resolved Hide resolved
Tests/TestJSONlib.sc Outdated Show resolved Hide resolved
@telephon
Copy link
Contributor Author

I consider them a perfect example of something that should be private static method

Hm, yes, from a java perspective probably true.

But in sclang it is rather unidiomatic to do it like this, because you always have to go through an extra indirection if you want to call the method from an instance ( this.class.something )

Private methods are really only those which you can't or shouldn't call from outside because of a technical condition. They don't read that well, usually there is a wrapper for them that "speaks" better.

If you like to make them private, put them in a section at the end of the class definition. That way they are not obscuring the more important things.

@capital-G
Copy link
Member

capital-G commented Jan 19, 2023

But in sclang it is rather unidiomatic to do it like this, because you always have to go through an extra indirection if you want to call the method from an instance ( this.class.something )

oh, now I remember. Something for the SC4 backlog as well :)

If you like to make them private, put them in a section at the end of the class definition. That way they are not obscuring the more important things.

To me its less about the source code but about the interface you provide - on test this is probably neglectable but in general it helps to understand what is happening in a quick manner w/o lurking into source-code/docs/method-scrolling by relying more on autocomplete/object inspection. Having a concise exposed interface, good naming of methods/args and have their expected (return) types transparent makes coding so much more comfortable - but I am not too sure that SC is all about make coding easy :D

Some things I think should be changed are

  • the location of the JSON files (which are test assets) and
  • dirname.dirname - calling dirname of a diraname results in going one dir up? can't we use something which is more obvious/standardized when working with paths such as ../ (if it works on unix/windows environment?)
  • more consistency on directory cases
    grafik

@telephon
Copy link
Contributor Author

  • dirname.dirname - calling dirname of a diraname results in going one dir up?

yes, what else? :)

The rest of the discussion (about private and class methods) we should have somewhere separately.

@capital-G
Copy link
Member

yes, what else? :)

I would assume that the dirname of a directory would be the dirname itself, so identity?

@telephon
Copy link
Contributor Author

telephon commented Jan 21, 2023

ah I see! I read "the directory of a directory"
Or also: "the dirname X of a directory is the dirname X", but "the dirname X of a dirname Y is not Y itself"

@capital-G capital-G merged commit be4ecf7 into main Jan 21, 2023
@capital-G capital-G deleted the cosmetics branch January 23, 2023 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants