1
\$\begingroup\$

I created this method for consecutive days to be listed with a dash, non-consecutive are separated by “and”. In my UI control I am only getting back a selectedDays: string[],. In this sample I am getting Mon,Tue,Wed,Fri. I am mapping then to the number 1 - 7 and then looping that list and removing consecutive numbers. I am also interested in hearing any other criticisms or thoughts.

Input array []

let arr = ['Mon', 'Tue, 'Wed ', 'Fri'];

Output Mon-Wed and Fri

public static selectedDaysOfWeekToString(selectedDays: string[], isHtml: boolean): string {
    var daysJoinedString: string = "";
    var currentRange: number[] = [];
    var definitionParts: string[] = [];
    let selectedDayData = new Map<string, number>();

    for (const day of selectedDays) {
        let WeekNumber = ScheduleCalendarHelper.GetDayOfWeekNumber(day);
        selectedDayData.set(day, WeekNumber);
    }

    var selectedDayNumbers: number[];
    selectedDayNumbers = Array.from(selectedDayData.values());

    var collapseRange = (selectedDayNumbers: number[]): string => {
        console.log('collapseRange');
        if (selectedDayNumbers.length === 1) {
            return selectedDayNumbers[0].toString();
        }

        var dash = isHtml ? "&ndash;" : " – ";
        return `${Array.from(selectedDayData.keys())[0]}${dash}${Array.from(selectedDayData.keys())[selectedDayNumbers.length - 1]}`;
    };


    for (var i = 0; i < selectedDayData.size; i++) {
        let dayNumber = Array.from(selectedDayData.values())[i];
        if (currentRange.length === 0) {
            console.log(Array.from(selectedDayData.keys())[i]);
            currentRange.push(dayNumber)
        } else {
                if (dayNumber - currentRange[currentRange.length - 1] > 1) {
                    definitionParts.push(collapseRange(currentRange));
                    currentRange = [selectedDayData[i]];
                } else {
                    currentRange.push(dayNumber);
                }
            }
    };

    if (currentRange.length > 0) {
        definitionParts.push(collapseRange(currentRange));
    };

    if (definitionParts.length > 0) {
        daysJoinedString =
            StringHelper.joinStringsWithSpecialLastSeparator(definitionParts.map(x => isHtml ? `<span class="selected-day-in-string">${x}</span>` : x),
                ", ", definitionParts.length === 2 ? " and " : ", and ");
    }

    var pluralModifier: string = (selectedDays.length > 1 ? "s" : "");
    return `day${pluralModifier} ${daysJoinedString}`;
}

public static GetDayOfWeekNumber(day){
    switch (day) {
        case "Sun": {
            return 1;
        }
        case "Mon": {
            return 2;
        }
        case "Tues": {
            return 3;
        }
        case "Wed": {
            return 4;
        }
        case "Thu": {
            return 5;
        }
        case "Fri": {
            return 6;
        }
        case "Sat": {
            return 7;
        }
    }
}

working sample https://stackblitz.com/edit/typescript-zmfzlw?file=index.ts

\$\endgroup\$
2
  • \$\begingroup\$ Input Mon,Tue,Wed,Fri - this is a single string. But is it coming in as an array of strings? \$\endgroup\$
    – radarbob
    Aug 7, 2023 at 23:27
  • \$\begingroup\$ Thanks I corrected that \$\endgroup\$
    – Jefferson
    Aug 8, 2023 at 10:38

3 Answers 3

2
+50
\$\begingroup\$

Here are my suggestions:

  • the sample code does not initially yield the correct output because range collapsing is not performed when you reach the end of selectedDayData (thus omitting Friday)
  • it may be useful to check for edge cases like ['Wed', 'Mon', 'Tue', 'Wed', 'Fri'] which yields Wed–Tue currently (likely not what you intended)
  • use let for variables with limited scope
  • use const when you do not intend to reassign (improves performance)
  • try to modularize into small, focused functions with the minimum necessary parameters
  • abstractions (like a Map from days to numbers) can be hard-coded for MVPs or when the concrete details are unlikely to change
  • prefer "const functions before use" over "var functions after use"
  • whenever reasonable, set useful defaults for parameters
  • if an expression occurs repeatedly, hoist it into a variable to avoid repeating work

I applied these principles to the sample code:

// Import stylesheets
import './style.css';

// dayToNum[day] gives num
const dayToNum = {
  Sun: 0,
  Mon: 1,
  Tue: 2,
  Wed: 3,
  Thu: 4,
  Fri: 5,
  Sat: 6,
};

// numToDay[num] gives day
const numToDay = ['Sun', 'Mon', 'Tue', 'Wed', 'Thu', 'Fri', 'Sat'];

// return the range [n1, n2] as a string
const rangeToString = (n1: number, n2: number, dash: string = '-'): string => {
  return `${numToDay[n1]}${dash}${numToDay[n2]}`;
};
console.log('rangeToString(1, 4)');
console.log(rangeToString(1, 4));

// if nums is an array of integers (ideally nondecreasing), collapse
const collapseSeq = (nums: number[], dash: string = '-'): string => {
  const n = nums.length;
  if (n === 1) return numToDay[nums[0]];
  return rangeToString(nums[0], nums[n - 1], dash);
};
console.log('collapseSeq([1, 2, 5])');
console.log(collapseSeq([1, 2, 5]));

// if nums is an array of increasing integers, collapse sequentially
const collapseNums = (nums: number[], dash: string = '-'): string[] => {
  const n = nums.length;
  const strings: string[] = [];
  let sequence = []; // current run of consecutive integers

  for (const num of nums) {
    let m = sequence.length;
    // if not consecutive, collapse and reset sequence
    if (m > 0 && num > sequence[m - 1] + 1) {
      strings.push(collapseSeq(sequence, dash));
      sequence = [num];
    } else sequence.push(num);
  }

  // if the end of nums is reached, collapse sequence
  if (sequence.length > 0) {
    strings.push(collapseSeq(sequence, dash));
  }

  return strings;
};
console.log('collapseNums([1, 2, 5])');
console.log(collapseNums([1, 2, 5]));

// collapses x by separator 'sep', using 'fin' for the last separator
const collapseStrings = (x: string[], sep: string, fin: string): string => {
  const n = x.length;

  if (n <= 0) return '';
  if (n === 1) return x[0];

  let result = x[0];
  for (let i = 1; i < n - 1; i++) {
    result += sep + x[i];
  }

  return result + fin + x[n - 1];
};
console.log("collapseStrings(['A', 'B', 'C'], ', ', ', and ')");
console.log(collapseStrings(['A', 'B', 'C'], ', ', ', and '));

// converts days to numbers, removing duplicates and sorting
const daysToNums = (days: string[]): number[] => {
  const nums: number[] = [];
  for (let i = 0; i < numToDay.length; i++) {
    if (days.indexOf(numToDay[i]) != -1) {
      nums.push(i);
    }
  }
  return nums;
};
console.log("daysToNums(['Mon', 'Fri', 'Fri', 'Wed'])");
console.log(daysToNums(['Mon', 'Fri', 'Fri', 'Wed']));

// minimum working example
let selectedDays = ['Mon', 'Tue', 'Wed', 'Fri'];

console.log('selectedDays');
console.log(selectedDays);

let selectedNums = daysToNums(selectedDays);

console.log('selectedNums');
console.log(selectedNums);

let isHtml = true;
let dash = isHtml ? '&ndash;' : ' – ';
let stringParts = collapseNums(selectedNums, dash);

console.log('stringParts');
console.log(stringParts);

let displayParts = stringParts.map((x) =>
  isHtml ? `<span class="selected-day-in-string">${x}</span>` : x
);
let fin_sep = stringParts.length === 2 ? ' and ' : ', and ';
let daysJoinedString = collapseStrings(displayParts, ', ', fin_sep);
let pluralModifier: string = selectedDays.length > 1 ? 's' : '';
let appDiv: HTMLElement = document.getElementById('app');
appDiv.innerHTML = `day${pluralModifier}: ${daysJoinedString}`;

Hope that helps! Great work on making a really straightforward-to-edit MVP.

Edit:

We can further improve daysToNums. In general, suppose you have N day strings mapped to integer values in [0, K). Currently, we call indexOf, an O(N) operation, K times. (For us, N = 4 and K = 7). Thus, the function takes O(N * K) time. We can change this to O(N + K):

const daysToNums = (days: string[]): number[] => {
  const k = numToDay.length;
  const present: boolean[] = new Array(k);
  for (const day of days) {
    present[dayToNum[day]] = true;
  }

  const nums: number[] = [];
  for (let i = 0; i < k; i++) {
    if (present[i]) {
      nums.push(i);
    }
  }

  return nums;
};

Note that this version may be more difficult to understand.

\$\endgroup\$
2
\$\begingroup\$

There are a few topics;

  • I would not take care of isHtml, I would have a dedicated function to escape HTML

    function escapeHTML(unsafeText) {
      let div = document.createElement('div');
      div.innerText = unsafeText;
      return div.innerHTML;
    }
    
  • This code takes an array, then builds a map, then converts it back to an array. I would use the Array.map function to convert directly from an array to an array. It would be shorter and more intuitive

  • getDayOfWeekNumber could be a one liner based on

    return ",Sun,Mon,Tue,Wed,Thu,Fri,Sat".split(",").indexOf(day)

  • You should write some tests, it seems the code does not work in a lot of cases

  • I think that a simple sort of the days array before parsing would resolve a lot of issues

  • For production code, you should drop console.log

  • Consider replacing var with const or let

This is JavaScript, but you should be able to follow;

function formatDays(days){

  const orderedDays = "Sun,Mon,Tue,Wed,Thu,Fri,Sat".split(",");
  const formatDateRange = (start,end) => start==end?orderedDays[start]:(orderedDays[start]+"-"+orderedDays[end]);
  const numberedDays = days.map( day => orderedDays.indexOf(day) ).sort();
  const groupedDays = [];
  let start, end;

  for(let i = 0; i < numberedDays.length; i++){
    const day = numberedDays[i];
    if(i==0){
      start = end = day;
      continue;
    }
    if(day == end+1){
      end = day;
    }else{
      groupedDays.push(formatDateRange(start, end));
      start = end = day;
    }   
  }
  groupedDays.push(formatDateRange(start, end));
  return groupedDays.join(" and ");
}
 
console.log( formatDays([]) );
console.log( formatDays(['Wed']));
console.log( formatDays(['Mon', 'Tue', 'Wed', 'Fri']));
console.log( formatDays(['Sun', 'Mon', 'Wed', 'Thu', 'Sat']));

\$\endgroup\$
1
\$\begingroup\$

Very hard to understand because of multiple generic data structures, key constants buried in code, working variables exposed; all because related functions are not encapsulated with their data. This steam punk code must be integrated, refactored into high level "consecutive day shorthand" algorithm steps composed of object method calls.


Map days to 0 - 6. This is consistent with JS Date object.


One fundamental data structure - a "Day".

function Day(shortName, index) {
  this.name = shortName;
  this.index = index;
}

function WeekDays() {
  var Week = [
    new Day("Sun", 0) ,
    new Day("Mon", 1) ,
    new Day("Tue", 2) ,
    new Day("Wed", 3) ,
    new Day("Thur", 4) ,
    new Day("Fri", 5) ,
    new Day("Sat", 6) ,
  ] , 

  function getIndex(shortName)  { return ( Week.find( x -> x.name === shortName )).index; }  
  function getByName(shortName) { return Week.find( x -> x['name'] === shortName); } ,
  function getByIndex(index)    { return Week.find( x -> x['index'] === index); }
  function newDay(shortName) {
    let template = this.getByName(shortName);
    return new Day(template.name, template.index);
  }
    
} // WeekDays

Something similar for a "consecutive days shorthand" object. Include constants for the "parts" such as dash, 'and', and whatever the heck "SpecialLastSeparator" is.

function ConsecutiveDays( thatInputString ) {
  var theDays = [];  // array of day objects.
  var shortHandDays = "";
  const dash = '-';
  const also = 'and';

  function makeDays( thatInputString ) {
    // converting strings to Day object array

    theDays.sortDays();
  }

  function compareNumbers(a, b) { 
    const A = a['index']; 
    const B = b['index'];
    return A - B; 
  }     
  function sortDays() { theDays.sort(compareNumbers); }

  // functions reflecting algorithm steps

} // ConsecutiveDays

Work hard, very hard, at making descriptive names for everything.

\$\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.