Skip to content
This repository has been archived by the owner on Apr 29, 2022. It is now read-only.

Code style and spelling fixes, updated index page. #81

Merged
merged 4 commits into from
Sep 14, 2019
Merged

Code style and spelling fixes, updated index page. #81

merged 4 commits into from
Sep 14, 2019

Conversation

SeppPenner
Copy link
Contributor

I know that this is nothing really important, but I don't like it when the code is unclean...

I will work on the bar chart soon. I assume, I should take the line chart as reference?

Copy link
Owner

@Joelius300 Joelius300 left a comment

Choose a reason for hiding this comment

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

Stupid spelling mistakes and typos which were fixed:

  • Tupel
  • readial
  • typesafe
  • barchart

Are there any more of those or did you fix all of them? I'm guessing you didn't look through each class right (that would be way too much)?

By the way, the usings were moved into the namespace for ColorUtil which is bad. We have them outside the namespace.

Also I don't like putting the top brace on the same line like you did with Summaries in WeatherForecastService.

This doesn't fix the private _prefix yet. Do we want to change that throughout the project (will it only affect the samples)? If so, you could add that to this PR as well.

Good update on the index page :)

@SeppPenner
Copy link
Contributor Author

Are there any more of those or did you fix all of them?

I can check that. I haven't seen more, but there is a Visual Studio tool that finds code issues. I will double-check that.

By the way, the usings were moved into the namespace for ColorUtil which is bad. We have them outside the namespace.

I thought, I adjusted that. I must have missed this class.

Also I don't like putting the top brace on the same line like you did with Summaries in WeatherForecastService.

Arg... I don't know why my StyleCop does this all the time... I don't want the Java code style...

This doesn't fix the private _prefix yet. Do we want to change that throughout the project (will it only affect the samples)? If so, you could add that to this PR as well.

I wasn't sure about this one, therefore it's not yet added. I can do that.

Do we want to change that throughout the project (will it only affect the samples)?

I have no idea if it will only affect the samples, but I can check that as well.

@Joelius300
Copy link
Owner

Also regarding the bar-chart rework. The way these mixable-datasets are implemented don't satisfy me quite yet. That's something I will try to improve in the future. Maybe I'll even settle for just having non-generic versions. One for string, one for double and one for point. I think that would actually cover everything but it's of course less dynamic :/ Again, this is subject to change sometime later but recommendations are welcome :)

If you want you can already start verifying the properties, fixing inconsistencies etc. Keep all the things in mind we discussed in the conventions and also all the reviews of your previous reworks. If you start before I have managed to update the conventions file, you really need to look at these references so we stay consistent and don't waste time :)

@SeppPenner
Copy link
Contributor Author

I've done that except the private fields for now.

I will start this and the bar chart rework on Monday, I guess...

@Joelius300 Joelius300 merged commit 80ce217 into Joelius300:master Sep 14, 2019
@SeppPenner SeppPenner deleted the code-cleanup branch September 16, 2019 05:39
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.

2 participants