current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 1 down vote favorite

I have some code that filters an array with type (Suggestion|undefined)[] into an array with type Suggestion[], but I feel it does so very inelegantly:

const optionsProvider: AutocompletionProvider = () => {
    const results: (Suggestion|undefined)[] = flagDescriptions.map(descriptions => {
        let shortFlagWithArgument = descriptions[0].match(/^ *-(\w) (\w*)$/);
        let shortFlagWithoutArgument = descriptions[0].match(/^ *-(\w) *(.*)$/);
        if (shortFlagWithArgument) {
            const flag = shortFlagWithArgument[1];
            const argument = shortFlagWithArgument[2];
            const description = combineManPageLines(descriptions.slice(1));

            return new Suggestion({
                value: `-${flag}`,
                style: styles.option,
                description,
                displayValue: `-${flag} ${argument}`,
            });
        } else if (shortFlagWithoutArgument) {
            const flag = shortFlagWithoutArgument[1];
            const description = combineManPageLines([shortFlagWithoutArgument[2], ...descriptions.slice(1)]);

            return new Suggestion({
                value: `-${flag}`,
                style: styles.option,
                description,
            });
        }
    });
    const filteredResults: Suggestion[] = [];
    results.forEach(result => {
        if (result) {
            filteredResults.push(result);
        }
    });
    return filteredResults;
};

In straight JS, this would just be array.filter(element => element !== undefined) but the best working solution I could come up with in TypeScript was to push the defined elements into a separate array within a forEach.

For context:

type AutocompletionProvider = (context: AutocompletionContext) => Suggestion[] | Promise<Suggestion[]>;

Original PR: https://github.com/shockone/black-screen/pull/623/files

share|improve this question
    
We allow links in questions, but the code you wish to be reviewed must be in the question body itself. – Dan Pantry Jul 20 '16 at 6:57
1  
Sorry @DanPantry I didn't realize. Fixed. – Drew Jul 20 '16 at 7:06
    
I don't understand the point of the type (Suggestion|undefined)[] in the first place. It is ok to put undefined into a Suggestion[], so why not change the type to be simply Suggestion[] throughout? – Daniel Ryan Jul 30 '16 at 3:10
up vote 1 down vote accepted

To be using strict null checks, you must be on "typescript": "^2.0.0". Thus, my example will be using this version of typescript with "compilerOptions":{"strictNullChecks": true}.

It does seem that the filter solution is the most elegant solution to the titular question. Sadly, it seems that the typescript static analysis is unable to track this behavior:

const source: (string|undefined)[] = ["one", "two", "three", undefined, "five"]
const filtered: string[] = source.filter(element => element !== undefined)
//  [ts]
//    Type '(string | undefined)[]' is not assignable to type 'string[]'.
//      Type 'string | undefined' is not assignable to type 'string'.
//        Type 'undefined' is not assignable to type 'string'.

This seems to be the case because of the declaration of the filter method:

/** Returns the elements of an array that meet the condition specified in a callback function. */
(method) Array<string | undefined>.filter(callbackfn: (value: string | undefined, index: number, array: (string | undefined)[]) => any, thisArg?: any): (string | undefined)[]

In this case, our own type logic beats out the compiler's. It's these cases that is the reason that Type Assertion is part of the language spec. Thus:

const source: (string|undefined)[] = ["one", "two", "three", undefined, "five"]
const filtered: string[] = source.filter(element => element !== undefined) as string[]

This transpiles exactly as you would expect -- in this case, transpiling is as simple as just removing the type information. If you need to convince the language to convert two unrelated types, you need to step through any to tell the compiler to back off: object as any as string.

Now, looking at the rest of the code:

shortFlagWithArgument and shortFlagWithoutArgument can be consts, as you do not mutate their values.

Personally, I would add an explicit final return to the flagDescriptions.map operation. This makes explicit why the undefined needs to be in the results array in the first place.

The regex patterns seem good candidates for refactoring into a reusable constant. Depending on your project ecosystem, however, it may be better just to leave them inlined.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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