Skip to content
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

Changing array type from generic to [] #9

Merged
merged 1 commit into from
Oct 31, 2017
Merged

Conversation

hisuwh
Copy link
Contributor

@hisuwh hisuwh commented Sep 12, 2017

I wanted to start a discussion on this.
My preference for array types is T[] as opposed to the generic form Array<T>

It is more concise, less typing (2 chars as opposed to 7) and puts the more important information up front (what is this an array of).

But this is quite a significant change so I would be interested to see what the consensus of the company is.

Copy link
Contributor

@jamesadarich jamesadarich left a comment

Choose a reason for hiding this comment

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

I'd vote in favour of Array due to the slightly more readable nature personally :)

@davidgruar
Copy link
Contributor

davidgruar commented Sep 12, 2017

Personally I prefer T[], for the same reasons I prefer int? to Nullable<int> in C# - easier to type and less noise.

@Jameskmonger
Copy link
Contributor

I prefer Array<T> as opposed to T[] generally - I find it clearer and don't really see any benefit to shaving off a few characters. It also becomes much easier to read with multiple dimensional arrays, personally (T[][] vs Array<Array<T>>).

I think it reads more naturally, too: "an array of T" (Array<T>) vs "t array" (T[])

@petevb
Copy link
Contributor

petevb commented Sep 12, 2017

There's a fairly comprehensive style guide from Basarat here which I'd be inclined to follow, purely because MSFT don't go into enough detail in their Style/Contribution Guidelines.

However, it seems to be their way:

FWIW, I've always written int[] (not Array<int>) and therefore also T[] not Array<T>. But I'm arguing for consistency rather than personal preference. Honest :)

petevb
petevb previously approved these changes Sep 12, 2017
Copy link
Contributor

@petevb petevb left a comment

Choose a reason for hiding this comment

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

Please get consensus and/or approval from a grown-up (e.g. Phil/Frans). After the commit is okay if this is blocking you, otherwise before ideally.

@Jameskmonger
Copy link
Contributor

I'm not entirely happy with this, actually. It would cause some readability issues in EBC.

Now, we have:

  • Array<T>
  • KnockoutObservableArray<T>

Whereas this change would give us

  • T[]
  • KnockoutObservableArray<T>

@OwenPattison
Copy link
Contributor

My preference would be to use Array for similar reasons to before, i find it more readable and its just what i use generally.

@petevb
Copy link
Contributor

petevb commented Sep 13, 2017

I don't care which we choose, only that we're consistent. That's why I looked for guidance on the web. If you guys (Experian) have already established a precedent then unless there's a very good reason to change we should continue with what you've done. I can't think of any justification to change from Array<T> to something else, even though I prefer the alternative.

@hisuwh
Copy link
Contributor Author

hisuwh commented Sep 13, 2017

I don't think we do have a precedent in Experian because we don't have any linting.
@OwenPattison do you mean the generic Array<> type?

@petevb
Copy link
Contributor

petevb commented Sep 13, 2017

Is the code reasonably consistent? The linter should match the code.

@OwenPattison
Copy link
Contributor

OwenPattison commented Sep 13, 2017

Sorry yes i meant Array<T> but i agree with Pete consistency is the key here

@petevb petevb dismissed their stale review September 13, 2017 16:54

Being internally consistent is more important than moving to msft/"Basarat" standard.

Copy link
Contributor

@Jameskmonger Jameskmonger left a comment

Choose a reason for hiding this comment

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

I'd prefer the Array generic type as that's what we're using in EBC due to the similarity with KnockoutObservableArray.

@hisuwh
Copy link
Contributor Author

hisuwh commented Sep 15, 2017

Your statement is not entirely correct James:

image

@hisuwh
Copy link
Contributor Author

hisuwh commented Sep 15, 2017

Experian is not consistent so I still believe this is still open for debate

@petevb
Copy link
Contributor

petevb commented Sep 15, 2017

Good to know. For comparison (with those 42 occurrences) how many Array<> would you find please?

@Jameskmonger
Copy link
Contributor

Jameskmonger commented Sep 15, 2017

image

108 usages of Array<> (not including KnockoutObservableArray<>)

Including KnockoutObservableArray<> there are 171 usages

@davidgruar
Copy link
Contributor

I think KnockoutObservableArray is a red herring. It's like saying that because project A happens to use a .NET type called FooBarNullable<T> a lot, company style should be to use Nullable<T> instead of T?.

@petevb
Copy link
Contributor

petevb commented Sep 15, 2017

I agree with that, even so it's 108 to 42. A way greater majority than something trivial like exiting Europe, but not particularly consistent either.

@hisuwh
Copy link
Contributor Author

hisuwh commented Sep 15, 2017

@dfibuch @thyde1 - forgot you guys were using typescript on Finygo - what's your thoughts?

@thyde1
Copy link
Member

thyde1 commented Sep 15, 2017

I think consistency is what is important here. However, I personally prefer T[] because its more concise, and really shouldn't cause confusion. FWIW, we have generally used this syntax in Finygo.

@thyde1
Copy link
Member

thyde1 commented Sep 15, 2017

In terms of usages, Finygo has Array<T>: 118 vs T[]: 401

@hisuwh
Copy link
Contributor Author

hisuwh commented Sep 20, 2017

So to summarise:

  • For ----- 3 - (Me, David, Tom)
  • Against - 3 - (James, James, Owen)
  • In favour of consistency - Pete

Yet to comment @dfibuch @philoUK

On consistency:

  • usages of Array<> - 226
  • usages of [] - 443

@dfibuch
Copy link
Member

dfibuch commented Sep 20, 2017

I personally follow the pattern already set out on the project, but my preference is to type as little as possible so generally would do T[]

@petevb
Copy link
Contributor

petevb commented Sep 20, 2017

Folks, Thanks for the discussion, stats, and summary.

Please use T[] going forward. It should be trivial to update Array<T> to T[] in existing code.

@hisuwh
Copy link
Contributor Author

hisuwh commented Oct 4, 2017

Can we merge this then if we have decided.

@petevb
Copy link
Contributor

petevb commented Oct 4, 2017 via email

@Jameskmonger
Copy link
Contributor

I'll merge this and publish later.

@Jameskmonger Jameskmonger self-assigned this Oct 4, 2017
@Jameskmonger
Copy link
Contributor

Just come across a use case for Array<> over T[]. How would you instantiate an empty typed array without explicitly specifying the type? I can't figure out how. See the difference between the two below.

class Foo {
    private bazArray = new Array<number>();

    private barArray: number[] = [];   
}

That means that arrays are going to be declared differently to other types.

class Foo {
    private count = 3;
    private baz = new Baz();
    private isRed = false;
    private person = {
        name: "Bob",
        age: 22
    };

    // different to the others :(
    private friends: Friend[] = [];
}

@petevb petevb merged commit b03dd32 into master Oct 31, 2017
@petevb petevb deleted the change-array-type branch October 31, 2017 15:19
Jameskmonger pushed a commit that referenced this pull request Mar 12, 2018
Changing array type from generic to []
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants