-
Notifications
You must be signed in to change notification settings - Fork 1
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
Tasks solved #1
base: main
Are you sure you want to change the base?
Tasks solved #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Kassymzhan, thank you for working on our Test-Task and providing us your approach. For us it is essentially important to give you a valuable feedback regarding your code to solve the task.
First of all, your solution fulfills all requirements. You have solved the task in time and you have worked with the hints we gave you. The navbar is in place and works as expected and you have added the functionality correctly.
I have made some comments and provided some improvements which are (in my opinion) making the code more readable, maintainable and easier to test (when units tests would be in place).
If you want you can comment or improve your solution with my hints.
Thank you,
Ralf
@@ -10,10 +10,11 @@ | |||
"@types/node": "^16.18.12", | |||
"@types/react": "^18.0.28", | |||
"@types/react-dom": "^18.0.10", | |||
"dayjs": "^1.11.7", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea to use a date-library!
src/App.tsx
Outdated
let month = test.getMonth(); | ||
test.setDate(test.getDate() + 1); | ||
return test.getMonth() !== month; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is a nice helper function to get the last day of the month.
The calculation is also a valuable approach to get the las day of the month.
I'd use this method more generic, to get the last day of the selected timespan.
So you can save some lines of code and tests further down.
I'd go with something like this:
const isLastDayOf = (dateString: string, dateResolution: IDateResolution) => {
if (dateResolution === 'daily') {
return true;
}
if (dateResolution === 'weekly') {
return dayjs(dateString).format('d') === '0';
}
return ...
};
and also there is a nice approach dayjs offers you to compare against the last day of month:
dayjs(dateString).format('DD') === dayjs(dateString).endOf('month').format('DD');
src/App.tsx
Outdated
import './App.css'; | ||
import FileUploader from "./components/FileUploader"; | ||
import Papa from "papaparse"; | ||
import TableView from "./components/TableView"; | ||
import ChartView from './components/ChartView'; | ||
const dayjs = require('dayjs') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import is more useful here:
import dayjs from "dayjs";
src/App.tsx
Outdated
import './App.css'; | ||
import FileUploader from "./components/FileUploader"; | ||
import Papa from "papaparse"; | ||
import TableView from "./components/TableView"; | ||
import ChartView from './components/ChartView'; | ||
const dayjs = require('dayjs') | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To not redefine the date-resolution again and agin I'd go for a new type here:
type IDateResolution = 'daily' | 'weekly' | 'monthly';
src/App.tsx
Outdated
const [data, setData] = useState<IDataShape[]>([]); | ||
const [sortBy, setSortBy] = useState<'daily' | 'weekly' | 'monthly'>('daily') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new type you can use directly here:
const [sortBy, setSortBy] = useState<IDateResolution>('daily');
src/App.tsx
Outdated
sun: 0, | ||
} | ||
|
||
if (sortBy === 'weekly') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as you early return, this line is not needed anymore
src/App.tsx
Outdated
// @ts-ignore | ||
weather[data[i].weather] += 1 | ||
|
||
if (week_day === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you can use the isLastDayOf function from the top.
if (isLastDayOf(data[i].date, sortBy)) {
src/App.tsx
Outdated
weather.fog = 0; | ||
} | ||
} | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This else and all after this can be removed now.
weather.fog = 0; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until here!
src/App.tsx
Outdated
} | ||
} else { | ||
for (let i = 0; i < data.length; i++) { | ||
total_days += 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have seen there is a last empty line which has invalida data, so I vaildated this here and checked if it is an empty string.
if (0 === data[i].date.length) {
continue;
}
… maintainable and easier to test
Good morning, Ralf.
Thank you for responding feedback and informative link to improve my code skills.
I have added in my branch new commit in which all your advises are used and code became more readable and more generic.
Link for the last commit - c09e68d
Many thanks,
Kassymzhan Assimov
Отправлено из мобильной Почты Mail.ru
Saturday, 18 February 2023, 02:36 +0600 from ***@***.*** ***@***.***>:
…
@rabbl requested changes on this pull request.
Hi Kassymzhan, thank you for working on our Test-Task and providing us you approach. For us it is essentially important to give you a valuable feedback regarding your approach.
First of all, your approach fulfills all requirements. You have solved the task in time and you have worked with the hints we gave you. The navbar is in place and works as expected and you have added the functionality correctly.
I have made some comments and provided some solutions which are (in my opinion) making the code more readable, maintainable and easier to test (when units tests would be in place).
If you want you can improve your solution with my hints.
Thank you, Ralf
----------------------------------------------------------------------
In package.json :
> @@ -10,10 +10,11 @@
***@***.***/node": "^16.18.12",
***@***.***/react": "^18.0.28",
***@***.***/react-dom": "^18.0.10",
+ "dayjs": "^1.11.7",
Good idea to use a date-library!
----------------------------------------------------------------------
In src/App.tsx :
> @@ -47,9 +51,152 @@ function App() {
}, [selectedFile]);
+
+ function isLastDay(date: string) {
+ let d = new Date(date)
+ let test = new Date(d.getTime())
+ let month = test.getMonth();
+ test.setDate(test.getDate() + 1);
+ return test.getMonth() !== month;
+ }
This method is a nice helper function to get the last day of the month.
The calculation is also a valuable approach to get the las day of the month.
I'd use this method more generic, to get the last day of the selected timespan.
So you can save some lines of code and tests further down.
I'd go with something like this:
const isLastDayOf = (dateString: string, dateResolution: IDateResolution) => {
if (dateResolution === 'daily') {
return true;
}
if (dateResolution === 'weekly') {
return dayjs(dateString).format('d') === '0';
}
return ...
};
and also there is a nice approach dayjs offers you to compare against the last day of month:
dayjs(dateString).format('DD') === dayjs(dateString).endOf('month').format('DD');
----------------------------------------------------------------------
In src/App.tsx :
> import './App.css';
import FileUploader from "./components/FileUploader";
import Papa from "papaparse";
import TableView from "./components/TableView";
+import ChartView from './components/ChartView';
+const dayjs = require('dayjs')
import is more useful here:
import dayjs from "dayjs";
----------------------------------------------------------------------
In src/App.tsx :
> import './App.css';
import FileUploader from "./components/FileUploader";
import Papa from "papaparse";
import TableView from "./components/TableView";
+import ChartView from './components/ChartView';
+const dayjs = require('dayjs')
To not redefine the date-resolution again and agin I'd go for a new type here:
type IDateResolution = 'daily' | 'weekly' | 'monthly';
----------------------------------------------------------------------
In src/App.tsx :
> const [data, setData] = useState<IDataShape[]>([]);
+ const [sortBy, setSortBy] = useState<'daily' | 'weekly' | 'monthly'>('daily')
The new type you can use directly here:
const [sortBy, setSortBy] = useState<IDateResolution>('daily');
----------------------------------------------------------------------
In src/App.tsx :
> @@ -47,9 +51,152 @@ function App() {
}, [selectedFile]);
+
+ function isLastDay(date: string) {
+ let d = new Date(date)
+ let test = new Date(d.getTime())
+ let month = test.getMonth();
+ test.setDate(test.getDate() + 1);
+ return test.getMonth() !== month;
+ }
+
+
+ const sortedData = useMemo(() => {
Good approach here, usng useMemo!
----------------------------------------------------------------------
In src/App.tsx :
> @@ -47,9 +51,152 @@ function App() {
}, [selectedFile]);
+
+ function isLastDay(date: string) {
+ let d = new Date(date)
+ let test = new Date(d.getTime())
+ let month = test.getMonth();
+ test.setDate(test.getDate() + 1);
+ return test.getMonth() !== month;
+ }
+
+
+ const sortedData = useMemo(() => {
+
+ if (sortBy === 'daily') {
+ return data
+ } else {
Here you can go with an early return. Means, you can and should remove the else here.
https://levelup.gitconnected.com/how-you-can-avoid-using-else-in-your-code-871197a1adbc
----------------------------------------------------------------------
In src/App.tsx :
> + let total_days = 0
+ let week_day = 0
+ let pr_sum = 0
+ let temp_max = 0
+ let temp_min = 0
+ let wind_sum = 0
+ let arr: IDataShape[] = []
+ let weather = {
+ fog: 0,
+ rain: 0,
+ snow: 0,
+ drizzle: 0,
+ sun: 0,
+ }
+
+ if (sortBy === 'weekly') {
as you early return, this line is not needed anymore
----------------------------------------------------------------------
In src/App.tsx :
> + drizzle: 0,
+ sun: 0,
+ }
+
+ if (sortBy === 'weekly') {
+ for (let i = 0; i < data.length; i++) {
+ total_days += 1
+ week_day = Number(dayjs(data[i].date).format('d'))
+ pr_sum += data[i].precipitation
+ temp_max += data[i].temp_max
+ temp_min += data[i].temp_min
+ wind_sum += data[i].wind
+ // @ts-ignore
+ weather[data[i].weather] += 1
+
+ if (week_day === 0) {
Here you can use the isLastDayOf function from the top.
if (isLastDayOf(data[i].date, sortBy)) {
----------------------------------------------------------------------
In src/App.tsx :
> + // @ts-ignore
+ weather: Object.keys(weather).reduce((a, b) => (weather[a] > weather[b] ? a : b)),
+ });
+ total_days = 0;
+ pr_sum = 0.0;
+ temp_max = 0.0;
+ temp_min = 0.0;
+ wind_sum = 0.0;
+ weather.rain = 0;
+ weather.snow = 0;
+ weather.drizzle = 0;
+ weather.sun = 0;
+ weather.fog = 0;
+ }
+ }
+ } else {
This else and all after this can be removed now.
----------------------------------------------------------------------
In src/App.tsx :
> + // @ts-ignore
+ weather: Object.keys(weather).reduce((a, b) => (weather[a] > weather[b] ? a : b)),
+ });
+ total_days = 0;
+ pr_sum = 0.0;
+ temp_max = 0.0;
+ temp_min = 0.0;
+ wind_sum = 0.0;
+ weather.rain = 0;
+ weather.snow = 0;
+ weather.drizzle = 0;
+ weather.sun = 0;
+ weather.fog = 0;
+ }
+ }
+ }
Until here!
----------------------------------------------------------------------
In src/App.tsx :
> + });
+ total_days = 0;
+ pr_sum = 0.0;
+ temp_max = 0.0;
+ temp_min = 0.0;
+ wind_sum = 0.0;
+ weather.rain = 0;
+ weather.snow = 0;
+ weather.drizzle = 0;
+ weather.sun = 0;
+ weather.fog = 0;
+ }
+ }
+ } else {
+ for (let i = 0; i < data.length; i++) {
+ total_days += 1
I have seen there is a last empty line which has invalida data, so I vaildated this here and checked if it is an empty string.
if (0 === data[i].date.length) {
continue;
}
—
Reply to this email directly, view it on GitHub , or unsubscribe .
You are receiving this because you authored the thread. Message ID: <inowas/react-test-task/pull/1/review/1304137950 @ github . com>
|
No description provided.