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

refactor variable naming #114

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

t-ober
Copy link
Contributor

@t-ober t-ober commented Mar 14, 2022

Resolves #113

@t-ober t-ober added the enhancement New feature or request label Mar 14, 2022
Copy link
Member

@sebastian-peter sebastian-peter left a comment

Choose a reason for hiding this comment

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

In ICONWeatherModel, the column fields (including getters and setters) are still using snake case. Although renaming them might look a little ugly. But then again, we've already adapted the naming in FileModel.

Furthermore, the following entities could be refactored as well:

  • edu.ie3.tools.utils.DatabaseController#persistence_unit_name
  • edu.ie3.tools.utils.ConfigurationParameters#SQL_FORMATTER
  • edu.ie3.tools.utils.FileEraser#filestatusLogger
  • edu.ie3.tools.Decompressor#filestatusLogger
  • edu.ie3.tools.Decompressor#folderpath
  • edu.ie3.tools.Downloader#filestatusLogger
  • edu.ie3.tools.Extractor#eccodesLocation
  • edu.ie3.tools.Main#filestatus
  • edu.ie3.tools.Main#database_schema

I also ignored the notation of timesteps everywhere, which one could consider.

@t-ober
Copy link
Contributor Author

t-ober commented Mar 28, 2022

In ICONWeatherModel, the column fields (including getters and setters) are still using snake case. Although renaming them might look a little ugly. But then again, we've already adapted the naming in FileModel.

Furthermore, the following entities could be refactored as well:

  • edu.ie3.tools.utils.DatabaseController#persistence_unit_name
  • edu.ie3.tools.utils.ConfigurationParameters#SQL_FORMATTER
  • edu.ie3.tools.utils.FileEraser#filestatusLogger
  • edu.ie3.tools.Decompressor#filestatusLogger
  • edu.ie3.tools.Decompressor#folderpath
  • edu.ie3.tools.Downloader#filestatusLogger
  • edu.ie3.tools.Extractor#eccodesLocation
  • edu.ie3.tools.Main#filestatus
  • edu.ie3.tools.Main#database_schema

I also ignored the notation of timesteps everywhere, which one could consider.

Did refactor the IconWeatherModel. The rest I would not dow here nor now. To be honest the whole project could use refactoring. But that probably should not be one of our priorities atm.

filestatusLogger.trace(
file.getName() + " | ss | sufficient_size = true | Download success");

filemodel.setDownload_date(ZonedDateTime.now());
filemodel.setDownloadDate(ZonedDateTime.now());
Copy link

Choose a reason for hiding this comment

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

JavaTimeDefaultTimeZone: ZonedDateTime.now() is not allowed because it silently uses the system default time-zone. You must pass an explicit time-zone (e.g., ZoneId.of("America/Los_Angeles")) to this method. (details)

Suggested change
filemodel.setDownloadDate(ZonedDateTime.now());
filemodel.setDownloadDate(ZonedDateTime.now(ZoneId.systemDefault()));

(at-me in a reply with help or ignore)

filestatusLogger.trace(
file.getName() + " | ss | sufficient_size = true | Download success");

filemodel.setDownload_date(ZonedDateTime.now());
filemodel.setDownloadDate(ZonedDateTime.now());
Copy link

Choose a reason for hiding this comment

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

JavaTimeDefaultTimeZone: ZonedDateTime.now() is not allowed because it silently uses the system default time-zone. You must pass an explicit time-zone (e.g., ZoneId.of("America/Los_Angeles")) to this method.

Suggested change
filemodel.setDownloadDate(ZonedDateTime.now());
filemodel.setDownloadDate(ZonedDateTime.now(ZoneId.systemDefault()));

(at-me in a reply with help or ignore)


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

tasks.add(new Decompressor(file, folderpath));
}
} else if (file.getDownload_fails() > 3
} else if (file.getDownloadFails() > 3
|| file.getModelrun().isBefore(ZonedDateTime.now().minusDays(1))) {
Copy link

Choose a reason for hiding this comment

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

JavaTimeDefaultTimeZone: ZonedDateTime.now() is not allowed because it silently uses the system default time-zone. You must pass an explicit time-zone (e.g., ZoneId.of("America/Los_Angeles")) to this method.

Suggested change
|| file.getModelrun().isBefore(ZonedDateTime.now().minusDays(1))) {
|| file.getModelrun().isBefore(ZonedDateTime.now(ZoneId.systemDefault()).minusDays(1))) {

(at-me in a reply with help or ignore)


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -195,15 +195,15 @@ public boolean downloadFile(String folder, FileModel filemodel) {
Copy link

Choose a reason for hiding this comment

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

PATH_TRAVERSAL_IN: This API (java/io/File.(Ljava/lang/String;)V) reads a file whose location might be specified by user input

(at-me in a reply with help or ignore)


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@sonatype-lift
Copy link

sonatype-lift bot commented Oct 12, 2022

⚠️ 2 God Classes were detected by Lift in this project. Visit the Lift web console for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor variable naming in FileModel
2 participants