Get count of credit cards that are expired [closed]

Clash Royale CLAN TAG#URR8PPP

.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;

up vote
0
down vote

favorite

Below is a sample implementation for getting the expired credit cards in the credit card list.I wrote a property and initialized it in the constructor. Need an opinion on if this is a proper way to implement it.

 public class User
 
 private readonly List<CreditCard> creditCards;

 public int CountOfCreditCards
 
 get
 
 return counter;
 
 

 private static int counter;

 private User()
 
 creditCards = new List<CreditCard>();
 counter = GetCountOfExpiredCreditCards();
 

 public int GetCountOfExpiredCreditCards()
 
 return creditCards.Count(x => x.ExpirationMonth <= DateTime.Now.Month);
 
 

c# object-oriented

share|improve this question

edited Aug 29 at 15:59

Mast

7,34663484

asked Aug 29 at 15:25

IRaki

72

closed as off-topic by Toby Speight, yuri, Malachi♦ Aug 29 at 18:30

This question appears to be off-topic. The users who voted to close gave this specific reason:

• "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – Toby Speight, yuri, Malachi

If this question can be reworded to fit the rules in the help center, please edit the question.

• 
  
  
  

  
  1
  

  
  
  
  
  
  

  
  Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. Feel free to post a follow-up question, although you're advised to wait at least a day before doing so.
  – Mast
  Aug 29 at 16:00
  

  
  
  
  


• 
  
  
  

  
  

  
  
  
  
  
  

  
  this doesn't work, nothing can ever get assigned to creditCards because it is a private readonly and is never set anywhere. you should have gotten a warning from Visual Studio and/or the compiler. I am going to close this question, because you either don't have working code or you did not provide us with enough code to review this properly.
  – Malachi♦
  Aug 29 at 18:29
  

  
  
  
  


• 
  
  
  

  
  

  
  
  
  
  
  

  
  My bad, May be i should have read the rules before. I was trying to improve my concrete implementation of the class. Thanks for the inputs.
  – IRaki
  Aug 29 at 19:13
  

  
  
  
  

add a comment | 

up vote
0
down vote

favorite

Below is a sample implementation for getting the expired credit cards in the credit card list.I wrote a property and initialized it in the constructor. Need an opinion on if this is a proper way to implement it.

 public class User
 
 private readonly List<CreditCard> creditCards;

 public int CountOfCreditCards
 
 get
 
 return counter;
 
 

 private static int counter;

 private User()
 
 creditCards = new List<CreditCard>();
 counter = GetCountOfExpiredCreditCards();
 

 public int GetCountOfExpiredCreditCards()
 
 return creditCards.Count(x => x.ExpirationMonth <= DateTime.Now.Month);
 
 

c# object-oriented

share|improve this question

edited Aug 29 at 15:59

Mast

7,34663484

asked Aug 29 at 15:25

IRaki

72

closed as off-topic by Toby Speight, yuri, Malachi♦ Aug 29 at 18:30

This question appears to be off-topic. The users who voted to close gave this specific reason:

• "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – Toby Speight, yuri, Malachi

If this question can be reworded to fit the rules in the help center, please edit the question.

• 
  
  
  

  
  1
  

  
  
  
  
  
  

  
  Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. Feel free to post a follow-up question, although you're advised to wait at least a day before doing so.
  – Mast
  Aug 29 at 16:00
  

  
  
  
  


• 
  
  
  

  
  

  
  
  
  
  
  

  
  this doesn't work, nothing can ever get assigned to creditCards because it is a private readonly and is never set anywhere. you should have gotten a warning from Visual Studio and/or the compiler. I am going to close this question, because you either don't have working code or you did not provide us with enough code to review this properly.
  – Malachi♦
  Aug 29 at 18:29
  

  
  
  
  


• 
  
  
  

  
  

  
  
  
  
  
  

  
  My bad, May be i should have read the rules before. I was trying to improve my concrete implementation of the class. Thanks for the inputs.
  – IRaki
  Aug 29 at 19:13
  

  
  
  
  

add a comment | 

up vote
0
down vote

favorite

up vote
0
down vote

favorite

Below is a sample implementation for getting the expired credit cards in the credit card list.I wrote a property and initialized it in the constructor. Need an opinion on if this is a proper way to implement it.

 public class User
 
 private readonly List<CreditCard> creditCards;

 public int CountOfCreditCards
 
 get
 
 return counter;
 
 

 private static int counter;

 private User()
 
 creditCards = new List<CreditCard>();
 counter = GetCountOfExpiredCreditCards();
 

 public int GetCountOfExpiredCreditCards()
 
 return creditCards.Count(x => x.ExpirationMonth <= DateTime.Now.Month);
 
 

c# object-oriented

share|improve this question

edited Aug 29 at 15:59

Mast

7,34663484

asked Aug 29 at 15:25

IRaki

72

Below is a sample implementation for getting the expired credit cards in the credit card list.I wrote a property and initialized it in the constructor. Need an opinion on if this is a proper way to implement it.

 public class User
 
 private readonly List<CreditCard> creditCards;

 public int CountOfCreditCards
 
 get
 
 return counter;
 
 

 private static int counter;

 private User()
 
 creditCards = new List<CreditCard>();
 counter = GetCountOfExpiredCreditCards();
 

 public int GetCountOfExpiredCreditCards()
 
 return creditCards.Count(x => x.ExpirationMonth <= DateTime.Now.Month);
 
 

c# object-oriented

share|improve this question

edited Aug 29 at 15:59

Mast

7,34663484

asked Aug 29 at 15:25

IRaki

72

share|improve this question

share|improve this question

edited Aug 29 at 15:59

Mast

7,34663484

edited Aug 29 at 15:59

Mast

7,34663484

edited Aug 29 at 15:59

Mast

7,34663484

7,34663484

asked Aug 29 at 15:25

IRaki

72

asked Aug 29 at 15:25

IRaki

72

asked Aug 29 at 15:25

IRaki

72

72

closed as off-topic by Toby Speight, yuri, Malachi♦ Aug 29 at 18:30

This question appears to be off-topic. The users who voted to close gave this specific reason:

• "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – Toby Speight, yuri, Malachi

If this question can be reworded to fit the rules in the help center, please edit the question.

closed as off-topic by Toby Speight, yuri, Malachi♦ Aug 29 at 18:30

This question appears to be off-topic. The users who voted to close gave this specific reason:

• "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – Toby Speight, yuri, Malachi

If this question can be reworded to fit the rules in the help center, please edit the question.

• 
  
  
  

  
  1
  

  
  
  
  
  
  

  
  Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. Feel free to post a follow-up question, although you're advised to wait at least a day before doing so.
  – Mast
  Aug 29 at 16:00
  

  
  
  
  


• 
  
  
  

  
  

  
  
  
  
  
  

  
  this doesn't work, nothing can ever get assigned to creditCards because it is a private readonly and is never set anywhere. you should have gotten a warning from Visual Studio and/or the compiler. I am going to close this question, because you either don't have working code or you did not provide us with enough code to review this properly.
  – Malachi♦
  Aug 29 at 18:29
  

  
  
  
  


• 
  
  
  

  
  

  
  
  
  
  
  

  
  My bad, May be i should have read the rules before. I was trying to improve my concrete implementation of the class. Thanks for the inputs.
  – IRaki
  Aug 29 at 19:13
  

  
  
  
  

add a comment | 

• 
  
  
  

  
  1
  

  
  
  
  
  
  

  
  Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. Feel free to post a follow-up question, although you're advised to wait at least a day before doing so.
  – Mast
  Aug 29 at 16:00
  

  
  
  
  


• 
  
  
  

  
  

  
  
  
  
  
  

  
  this doesn't work, nothing can ever get assigned to creditCards because it is a private readonly and is never set anywhere. you should have gotten a warning from Visual Studio and/or the compiler. I am going to close this question, because you either don't have working code or you did not provide us with enough code to review this properly.
  – Malachi♦
  Aug 29 at 18:29
  

  
  
  
  


• 
  
  
  

  
  

  
  
  
  
  
  

  
  My bad, May be i should have read the rules before. I was trying to improve my concrete implementation of the class. Thanks for the inputs.
  – IRaki
  Aug 29 at 19:13
  

  
  
  
  

1

1

Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. Feel free to post a follow-up question, although you're advised to wait at least a day before doing so.
– Mast
Aug 29 at 16:00

Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. Feel free to post a follow-up question, although you're advised to wait at least a day before doing so.
– Mast
Aug 29 at 16:00

this doesn't work, nothing can ever get assigned to creditCards because it is a private readonly and is never set anywhere. you should have gotten a warning from Visual Studio and/or the compiler. I am going to close this question, because you either don't have working code or you did not provide us with enough code to review this properly.
– Malachi♦
Aug 29 at 18:29

this doesn't work, nothing can ever get assigned to creditCards because it is a private readonly and is never set anywhere. you should have gotten a warning from Visual Studio and/or the compiler. I am going to close this question, because you either don't have working code or you did not provide us with enough code to review this properly.
– Malachi♦
Aug 29 at 18:29

My bad, May be i should have read the rules before. I was trying to improve my concrete implementation of the class. Thanks for the inputs.
– IRaki
Aug 29 at 19:13

My bad, May be i should have read the rules before. I was trying to improve my concrete implementation of the class. Thanks for the inputs.
– IRaki
Aug 29 at 19:13

add a comment | 

2 Answers
2

active

oldest

votes

up vote
5
down vote

If a class has one or more private constructors and no public constructors, other classes (except nested classes) cannot create instances of this class.

https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/private-constructors

Make your constructor public.

There is an obvious bug in GetCountOfExpiredCreditCards A card that expires in 01/19(January, 2019) will show up as expired if checked today. Check for both year and month.

share|improve this answer

answered Aug 29 at 15:35

tinstaafl

5,667625

• 
  
  
  

  
  

  
  
  
  
  
  

  
  uggh..How did i miss that, Thanks for pointing out the expiration logic.
  – IRaki
  Aug 29 at 15:48
  

  
  
  
  

add a comment | 

up vote
2
down vote

Consistency is key when you're naming things. You have counter, CountOfCreditCards and a method called GetCountOfExpiredCreditCards. Your method is named perfectly but the other two are less precise. Try to be consistent.

Did you know about read-only auto properties? That would get rid of your counter field:

public int CountOfExpiredCreditCards get; 

private User()

 creditCards = new List<CreditCard>();
 CountOfExpiredCreditCards = GetCountOfExpiredCreditCards();

I would argue that it's fine to just calculate the count on demand by making the method public and removing the property entirely.

You also use DateTime.Now which is the system local time. Timezones and DateTime handling are a very deep rabbit hole so just be aware that generally speaking, you would want to use DateTime.UtcNow, DateTimeOffset, or a third party like Noda Time's ZonedDateTime.

As an aside, you have the counter field as static. That means it's shared between all instances of User which is almost certainly not what you want.

share|improve this answer

answered Aug 29 at 16:13

RobH

14k42461

add a comment | 

2 Answers
2

active

oldest

votes

2 Answers
2

active

oldest

votes

active

oldest

votes

active

oldest

votes

up vote
5
down vote

If a class has one or more private constructors and no public constructors, other classes (except nested classes) cannot create instances of this class.

https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/private-constructors

Make your constructor public.

There is an obvious bug in GetCountOfExpiredCreditCards A card that expires in 01/19(January, 2019) will show up as expired if checked today. Check for both year and month.

share|improve this answer

answered Aug 29 at 15:35

tinstaafl

5,667625

• 
  
  
  

  
  

  
  
  
  
  
  

  
  uggh..How did i miss that, Thanks for pointing out the expiration logic.
  – IRaki
  Aug 29 at 15:48
  

  
  
  
  

add a comment | 

up vote
5
down vote

If a class has one or more private constructors and no public constructors, other classes (except nested classes) cannot create instances of this class.

https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/private-constructors

Make your constructor public.

There is an obvious bug in GetCountOfExpiredCreditCards A card that expires in 01/19(January, 2019) will show up as expired if checked today. Check for both year and month.

share|improve this answer

answered Aug 29 at 15:35

tinstaafl

5,667625

• 
  
  
  

  
  

  
  
  
  
  
  

  
  uggh..How did i miss that, Thanks for pointing out the expiration logic.
  – IRaki
  Aug 29 at 15:48
  

  
  
  
  

add a comment | 

up vote
5
down vote

up vote
5
down vote

If a class has one or more private constructors and no public constructors, other classes (except nested classes) cannot create instances of this class.

https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/private-constructors

Make your constructor public.

There is an obvious bug in GetCountOfExpiredCreditCards A card that expires in 01/19(January, 2019) will show up as expired if checked today. Check for both year and month.

share|improve this answer

answered Aug 29 at 15:35

tinstaafl

5,667625

If a class has one or more private constructors and no public constructors, other classes (except nested classes) cannot create instances of this class.

https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/private-constructors

Make your constructor public.

There is an obvious bug in GetCountOfExpiredCreditCards A card that expires in 01/19(January, 2019) will show up as expired if checked today. Check for both year and month.

share|improve this answer

answered Aug 29 at 15:35

tinstaafl

5,667625

share|improve this answer

share|improve this answer

answered Aug 29 at 15:35

tinstaafl

5,667625

answered Aug 29 at 15:35

tinstaafl

5,667625

answered Aug 29 at 15:35

tinstaafl

5,667625

5,667625

• 
  
  
  

  
  

  
  
  
  
  
  

  
  uggh..How did i miss that, Thanks for pointing out the expiration logic.
  – IRaki
  Aug 29 at 15:48
  

  
  
  
  

add a comment | 

• 
  
  
  

  
  

  
  
  
  
  
  

  
  uggh..How did i miss that, Thanks for pointing out the expiration logic.
  – IRaki
  Aug 29 at 15:48
  

  
  
  
  

uggh..How did i miss that, Thanks for pointing out the expiration logic.
– IRaki
Aug 29 at 15:48

uggh..How did i miss that, Thanks for pointing out the expiration logic.
– IRaki
Aug 29 at 15:48

add a comment | 

up vote
2
down vote

Consistency is key when you're naming things. You have counter, CountOfCreditCards and a method called GetCountOfExpiredCreditCards. Your method is named perfectly but the other two are less precise. Try to be consistent.

Did you know about read-only auto properties? That would get rid of your counter field:

public int CountOfExpiredCreditCards get; 

private User()

 creditCards = new List<CreditCard>();
 CountOfExpiredCreditCards = GetCountOfExpiredCreditCards();

I would argue that it's fine to just calculate the count on demand by making the method public and removing the property entirely.

You also use DateTime.Now which is the system local time. Timezones and DateTime handling are a very deep rabbit hole so just be aware that generally speaking, you would want to use DateTime.UtcNow, DateTimeOffset, or a third party like Noda Time's ZonedDateTime.

As an aside, you have the counter field as static. That means it's shared between all instances of User which is almost certainly not what you want.

share|improve this answer

answered Aug 29 at 16:13

RobH

14k42461

add a comment | 

up vote
2
down vote

Consistency is key when you're naming things. You have counter, CountOfCreditCards and a method called GetCountOfExpiredCreditCards. Your method is named perfectly but the other two are less precise. Try to be consistent.

Did you know about read-only auto properties? That would get rid of your counter field:

public int CountOfExpiredCreditCards get; 

private User()

 creditCards = new List<CreditCard>();
 CountOfExpiredCreditCards = GetCountOfExpiredCreditCards();

I would argue that it's fine to just calculate the count on demand by making the method public and removing the property entirely.

You also use DateTime.Now which is the system local time. Timezones and DateTime handling are a very deep rabbit hole so just be aware that generally speaking, you would want to use DateTime.UtcNow, DateTimeOffset, or a third party like Noda Time's ZonedDateTime.

As an aside, you have the counter field as static. That means it's shared between all instances of User which is almost certainly not what you want.

share|improve this answer

answered Aug 29 at 16:13

RobH

14k42461

add a comment | 

up vote
2
down vote

up vote
2
down vote

Consistency is key when you're naming things. You have counter, CountOfCreditCards and a method called GetCountOfExpiredCreditCards. Your method is named perfectly but the other two are less precise. Try to be consistent.

Did you know about read-only auto properties? That would get rid of your counter field:

public int CountOfExpiredCreditCards get; 

private User()

 creditCards = new List<CreditCard>();
 CountOfExpiredCreditCards = GetCountOfExpiredCreditCards();

I would argue that it's fine to just calculate the count on demand by making the method public and removing the property entirely.

You also use DateTime.Now which is the system local time. Timezones and DateTime handling are a very deep rabbit hole so just be aware that generally speaking, you would want to use DateTime.UtcNow, DateTimeOffset, or a third party like Noda Time's ZonedDateTime.

As an aside, you have the counter field as static. That means it's shared between all instances of User which is almost certainly not what you want.

share|improve this answer

answered Aug 29 at 16:13

RobH

14k42461

Consistency is key when you're naming things. You have counter, CountOfCreditCards and a method called GetCountOfExpiredCreditCards. Your method is named perfectly but the other two are less precise. Try to be consistent.

Did you know about read-only auto properties? That would get rid of your counter field:

public int CountOfExpiredCreditCards get; 

private User()

 creditCards = new List<CreditCard>();
 CountOfExpiredCreditCards = GetCountOfExpiredCreditCards();

I would argue that it's fine to just calculate the count on demand by making the method public and removing the property entirely.

You also use DateTime.Now which is the system local time. Timezones and DateTime handling are a very deep rabbit hole so just be aware that generally speaking, you would want to use DateTime.UtcNow, DateTimeOffset, or a third party like Noda Time's ZonedDateTime.

As an aside, you have the counter field as static. That means it's shared between all instances of User which is almost certainly not what you want.

share|improve this answer

answered Aug 29 at 16:13

RobH

14k42461

share|improve this answer

share|improve this answer

answered Aug 29 at 16:13

RobH

14k42461

answered Aug 29 at 16:13

RobH

14k42461

answered Aug 29 at 16:13

RobH

14k42461

14k42461

add a comment | 

add a comment |