Skip to content
This repository has been archived by the owner on Sep 11, 2023. It is now read-only.

T1-2846 #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

T1-2846 #37

wants to merge 1 commit into from

Conversation

michael-scheurer
Copy link
Contributor

No description provided.

@@ -152,6 +152,16 @@ export default class Basket {
// load default start date instance
if (!startDateInstance) startDateInstance = this.getCurrentDateInstance()

// check that selected date and timespan is within available dates
const tmpBasketEntry = new BasketEntry({})
Copy link
Contributor

Choose a reason for hiding this comment

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

why empty object?

basket/Basket.js Show resolved Hide resolved
@@ -215,6 +225,13 @@ export default class Basket {
* @returns {Promise<boolean>}
*/
async updateBasketEntries(basketEntriesArray) {
// check validity dates
const checkResult = this.checkAvailableDates(basketEntriesArray)
Copy link
Contributor

Choose a reason for hiding this comment

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

again, this seems very weird

Comment on lines +306 to +310
const checkResult = this.checkAvailableDates([basketEntryInstance])
if (!checkResult) {
EventBus.$emit('spinnerHide')
return false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

lots of duplicate code aswell, you have the same logic 3 times here now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How to improve? If the check fails, the function has to exit and return a boolean..

Copy link
Contributor

@fewhnhouse fewhnhouse Nov 13, 2021

Choose a reason for hiding this comment

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

@michael-scheurer it cant be that the logic has to check for this behaviour 3 times in a row. If it indeed does, you should split up the function it is in into 3 separate functions, as it makes it obvious that the current function is handling 3 different usecases at the same time.

basket/Basket.js Show resolved Hide resolved
@@ -655,7 +655,8 @@ export default class Product {
filterProductDefinitionsByProductDefinitionAndAttributeKey(
attributeKey,
productDefinition,
excludeAttributeKeys = []
excludeAttributeKeys = [],
includeMauiOnly = false
) {
let filteredProductDefinitions = []
Copy link
Contributor

Choose a reason for hiding this comment

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

const

Comment on lines +697 to +698
filteredProductDefinitions = filteredProductDefinitions.filter(
(prodDef) => !prodDef.isMauiOnly()
Copy link
Contributor

Choose a reason for hiding this comment

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

do not modify existing objects. this obscures what the array contains. this should be named mauiOnlyProductDefinitions

products/Product.js Show resolved Hide resolved
Comment on lines +934 to +943
for (let i = 0; i < bookingDateList.length; i++) {
const bookingDate = bookingDateList[i]
const foundAvailableDate = availableDateList.find((availableDate) => {
return availableDate.getTime() === bookingDate.getTime()
})
if (!foundAvailableDate) {
EventBus.$emit('notify', i18n.t('basket.dateNotAvailable'))
return false
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

use for of. you are not using an index anywhere.

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