2
\$\begingroup\$

I have these few helper function that are used to check if the string in the given array is present and if the certain steps are present and then only have the other function fire. The reason I currently have them separated is because arrTours need to be associated with only those arrSteps. those will always be the values in those arrays

// Handles for manditory action needed by user
const arrTourOver = ['OverviewTour'];
const arrStepOver = [6, 7];
export const handleJoyrideOverview = (
  dispatch: any,
  tour: any,
  index: number
) => {
  arrTourOver.includes(tour?.openTour) &&
    arrStepOver.includes(tour?.stepIndex) &&
    JoyRideDelayContinue(dispatch, tour, tour?.openTour, index);
};

// Handles for manditory action needed by user
const arrTourResize = ['ResizingWidgets'];
const arrStepResize = [0, 1];
export const handleJoyrideResize = (
  dispatch: any,
  tour: any,
  index: number
) => {
  arrTourResize.includes(tour?.openTour) &&
    arrStepResize.includes(tour?.stepIndex) &&
    JoyRideDelayContinue(dispatch, tour, tour?.openTour, index);
};

// Handles for manditory action needed by user
const arrTourDock = ['DockbarFunctions'];
const arrStepDock = [3, 4];
export const handleJoyrideDock = (dispatch: any, tour: any, index: number) => {
  arrTourDock.includes(tour?.openTour) &&
    arrStepDock.includes(tour?.stepIndex) &&
    JoyRideDelayContinue(dispatch, tour, tour?.openTour, index);
};

These are the 3 I currently have but I will be adding a few more I just want to figure out reducing the redundancy before I keep going

this part isn't really needed but I'll put JoyRideDelayContinue function below just in case

export const JoyRideDelayContinue = (
  dispatch: any,
  tour: any,
  tourType: string,
  stepIndex: number
) => {
  setTimeout(() => {
    if (tour && tour.openTour === tourType) {
      dispatch({ type: 'SET_OPEN_TOUR', payload: '' });
      dispatch({
        type: 'PROGRESS_NEXT_OR_PREV',
        payload: { type: tourType, stepIndex: stepIndex }
      });
      setTimeout(
        () => dispatch({ type: 'SET_OPEN_TOUR', payload: tourType }),
        500
      );
    }
  }, 1000);
};
\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

Define types

If you are using a typed language like TypeScript, you should use it correctly. Defining most of the variables as type any does not give you robust type protection. Using TypeScript this way is just bloating your code with noise.

Low quality

Sorry to say this, but the code is rather low quality.

The code has a lot of repeated and some redundant code.

Data is unorganized and strewn ad-hoc in the code creating a maintenance nightmare.

Names are too long and you use pre and post fix naming to organize related data. That is what objects are for. You should seldomly need to use pre/post fixed naming in an OO language.

Your intent is not clear due to the way you use ?. It is possible that you have used them correctly, but that is a very unusual situation and should be handled before you pass tour to any function.

Only export what you need. I have the feeling you have slapped export in front of code that will never be used outside the module.

Rewrite

The rewrite moves the 3 functions into a factory. The data they need is passed as arguments.

The strings and steps are stored in 2 Sets to keep time complexity down.

I am assuming that tour is never nullish when passed and that you never put undefined in the string arrays or the numbers that include undefined which will negate the need for ?.

All the handleJoyride??? calls are added to an export object called joyrides. Thus

const arrTourOver = ['OverviewTour'];
const arrStepOver = [6, 7];
export const handleJoyrideOverview = (

becomes

export const joyrides = {
    overview: createDispatcher(['OverviewTour'], [6, 7]),
    ...

The resulting rewrite as Javascript

export const joyrides = {
   overview: createDispatcher(['OverviewTour'], [6, 7]),
   resize: createDispatcher(['ResizingWidgets'], [0, 1]),
   dock: createDispatcher(['DockbarFunctions'], [3, 4]),
};  

function createDispatcher(strs, steps, dispatcher = JoyRideDelayContinue){
    [strs, steps] = [new Set(strs), new Set(steps)];    
    return (dispatch, tour, idx) => {
        strs.has(tour.openTour) && steps.has(tour.stepsIndex) && 
        dispatcher(dispatch, tour, idx)
    };
}   

const JoyRideDelayContinue = (dispatch, tour, stepIndex) => {
    setTimeout(() => {
        dispatch({type: 'SET_OPEN_TOUR', payload: ''});
        dispatch({type: 'PROGRESS_NEXT_OR_PREV', payload: {type: tour.openTour, stepIndex}});
        setTimeout(dispatch, 500, {type: 'SET_OPEN_TOUR', payload: tour.openTour})
    }, 1000);
}
 
\$\endgroup\$
1
  • \$\begingroup\$ Thank you so much for this feedback \$\endgroup\$ Commented Jul 13, 2021 at 2:50
4
\$\begingroup\$

The first thing I would extract is the condition:

const includesTourData = (tours, steps, tour) => tours.includes(tour?.openTour) && steps.includes(tour?.stepIndex) &&

const handleJoyrideDock = (dispatch: any, tour: any, index: number) =>
      includesTourData(arrTourDock, arrStepsDock, tour) &&    
      JoyRideDelayContinue(dispatch, tour, tour?.openTour, index);

I would improve the params of JoyRideDelayContinue, you're checking for equality inside the function tour.openTour == tourType, but at the call site you call it with tour, tour?.openTour, which is redundant:

export const JoyRideDelayContinue = (
  dispatch: any,
  tour: any,
  stepIndex: number
) => {
  setTimeout(() => {
      dispatch({ type: 'SET_OPEN_TOUR', payload: '' });
      dispatch({
        type: 'PROGRESS_NEXT_OR_PREV',
        payload: { type: tour?.openTour, stepIndex }
      });
      setTimeout(
        () => dispatch({ type: 'SET_OPEN_TOUR', payload: tour?.openTour }),
        500
      );
  }, 1000);
};

Finally, I think that you are adding a level of unnecessary indirection with the handleJoyrideDock-like functions. You could just write the conditional logic at the call site.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.