Sport Cards Task












4















The task:



You will receive several input lines in one of the following formats:




  • "{card} - {sport} - {price}"

  • "check {card}"


The card and sport are
strings. Price will be a floating point number. You need to keep track
of every card. When you receive a card, a sport and a price,
register the card in the database if it isn't present, otherwise add
the sport and the price. If the card already contains the sport, you
need to overwrite the price. If you receive "check {card}" you need to
check if the card is available or not and print it on the console in
the format:



"{card} is available!" if the card is present and



"{card} is not available!" if the card is not present



You should end your program when you receive the command "end". At that point you should print the cards, ordered by sports’ count in desecending order. Foreach card print their sport and price ordered by sports’ name.




Examples



My Solution



using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.RegularExpressions;

class Program
{
class Card
{
public Card(string n, string s, double p)
{
name = n;
addSport(s, p);
}
public void addSport(string sportName, double price)
{
if (sportPriceInfo.ContainsKey(sportName))
{
sportPriceInfo[sportName] = price;
}
else
{
sportCount++;
sportPriceInfo.Add(sportName, price);
}
}
public string name;
public int sportCount = 0;
Dictionary<string, double> sportPriceInfo = new Dictionary<string, double>();
public void showInfo()
{
Console.WriteLine(name + ':');
var sortedDictionary = from pair in sportPriceInfo
orderby pair.Key ascending
select pair;
foreach (var item in sortedDictionary)
{
Console.WriteLine(" -{0} - {1:0.00}", item.Key, item.Value);
}
}
}
class Database
{
List<Card> allCards = new List<Card>();
public void addCard(string n, string s, double p)
{
int cardIndex = isCardPresent(n);
if (cardIndex >= 0)
{
allCards[cardIndex].addSport(s, p);
}
else
{
allCards.Add(new Card(n, s, p));
}
}
public void checkCard(string n)
{
if (isCardPresent(n) >= 0)
{
Console.WriteLine(n + " is available!");
}
else
{
Console.WriteLine(n + " is not available!");
}
}
private int isCardPresent(string n)
{
//returns the index of the matched item, otherwise returns -1
int cardIndex = -1;
for (int i = 0; i < allCards.Count; i++)
{
if (allCards[i].name == n)
{
cardIndex = i;
break;
}
}
return cardIndex;
}
public void showDataBase()
{
List<Card> sortedCards = allCards.OrderByDescending(o => o.sportCount).ToList();
foreach (var card in sortedCards)
{
card.showInfo();
}
}
}
static void Main()
{
string line;
Database db = new Database();
Match lineSplit;
while ((line = Console.ReadLine()) != "end")
{
if (Regex.IsMatch(line, @"^(w+) - (w+) - (d+.d+)b$"))
{
lineSplit = Regex.Match(line, @"^(w+) - (w+) - (d+.d+)b$");
string inputName = lineSplit.Groups[1].Value;
string inputSport = lineSplit.Groups[2].Value;
double inputPrice = double.Parse(lineSplit.Groups[3].Value);
db.addCard(inputName, inputSport, inputPrice);
}
else
{
lineSplit = Regex.Match(line, @"^check (w+)b$");
db.checkCard(lineSplit.Groups[1].Value);
}
}
db.showDataBase();
}
}









share|improve this question






















  • Give your code a bit of breathing room. It's a bit difficult to see where elements begin and end. Microsoft has guidelines: docs.microsoft.com/en-us/dotnet/csharp/programming-guide/…
    – Jesse C. Slicer
    Dec 27 at 3:40
















4















The task:



You will receive several input lines in one of the following formats:




  • "{card} - {sport} - {price}"

  • "check {card}"


The card and sport are
strings. Price will be a floating point number. You need to keep track
of every card. When you receive a card, a sport and a price,
register the card in the database if it isn't present, otherwise add
the sport and the price. If the card already contains the sport, you
need to overwrite the price. If you receive "check {card}" you need to
check if the card is available or not and print it on the console in
the format:



"{card} is available!" if the card is present and



"{card} is not available!" if the card is not present



You should end your program when you receive the command "end". At that point you should print the cards, ordered by sports’ count in desecending order. Foreach card print their sport and price ordered by sports’ name.




Examples



My Solution



using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.RegularExpressions;

class Program
{
class Card
{
public Card(string n, string s, double p)
{
name = n;
addSport(s, p);
}
public void addSport(string sportName, double price)
{
if (sportPriceInfo.ContainsKey(sportName))
{
sportPriceInfo[sportName] = price;
}
else
{
sportCount++;
sportPriceInfo.Add(sportName, price);
}
}
public string name;
public int sportCount = 0;
Dictionary<string, double> sportPriceInfo = new Dictionary<string, double>();
public void showInfo()
{
Console.WriteLine(name + ':');
var sortedDictionary = from pair in sportPriceInfo
orderby pair.Key ascending
select pair;
foreach (var item in sortedDictionary)
{
Console.WriteLine(" -{0} - {1:0.00}", item.Key, item.Value);
}
}
}
class Database
{
List<Card> allCards = new List<Card>();
public void addCard(string n, string s, double p)
{
int cardIndex = isCardPresent(n);
if (cardIndex >= 0)
{
allCards[cardIndex].addSport(s, p);
}
else
{
allCards.Add(new Card(n, s, p));
}
}
public void checkCard(string n)
{
if (isCardPresent(n) >= 0)
{
Console.WriteLine(n + " is available!");
}
else
{
Console.WriteLine(n + " is not available!");
}
}
private int isCardPresent(string n)
{
//returns the index of the matched item, otherwise returns -1
int cardIndex = -1;
for (int i = 0; i < allCards.Count; i++)
{
if (allCards[i].name == n)
{
cardIndex = i;
break;
}
}
return cardIndex;
}
public void showDataBase()
{
List<Card> sortedCards = allCards.OrderByDescending(o => o.sportCount).ToList();
foreach (var card in sortedCards)
{
card.showInfo();
}
}
}
static void Main()
{
string line;
Database db = new Database();
Match lineSplit;
while ((line = Console.ReadLine()) != "end")
{
if (Regex.IsMatch(line, @"^(w+) - (w+) - (d+.d+)b$"))
{
lineSplit = Regex.Match(line, @"^(w+) - (w+) - (d+.d+)b$");
string inputName = lineSplit.Groups[1].Value;
string inputSport = lineSplit.Groups[2].Value;
double inputPrice = double.Parse(lineSplit.Groups[3].Value);
db.addCard(inputName, inputSport, inputPrice);
}
else
{
lineSplit = Regex.Match(line, @"^check (w+)b$");
db.checkCard(lineSplit.Groups[1].Value);
}
}
db.showDataBase();
}
}









share|improve this question






















  • Give your code a bit of breathing room. It's a bit difficult to see where elements begin and end. Microsoft has guidelines: docs.microsoft.com/en-us/dotnet/csharp/programming-guide/…
    – Jesse C. Slicer
    Dec 27 at 3:40














4












4








4








The task:



You will receive several input lines in one of the following formats:




  • "{card} - {sport} - {price}"

  • "check {card}"


The card and sport are
strings. Price will be a floating point number. You need to keep track
of every card. When you receive a card, a sport and a price,
register the card in the database if it isn't present, otherwise add
the sport and the price. If the card already contains the sport, you
need to overwrite the price. If you receive "check {card}" you need to
check if the card is available or not and print it on the console in
the format:



"{card} is available!" if the card is present and



"{card} is not available!" if the card is not present



You should end your program when you receive the command "end". At that point you should print the cards, ordered by sports’ count in desecending order. Foreach card print their sport and price ordered by sports’ name.




Examples



My Solution



using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.RegularExpressions;

class Program
{
class Card
{
public Card(string n, string s, double p)
{
name = n;
addSport(s, p);
}
public void addSport(string sportName, double price)
{
if (sportPriceInfo.ContainsKey(sportName))
{
sportPriceInfo[sportName] = price;
}
else
{
sportCount++;
sportPriceInfo.Add(sportName, price);
}
}
public string name;
public int sportCount = 0;
Dictionary<string, double> sportPriceInfo = new Dictionary<string, double>();
public void showInfo()
{
Console.WriteLine(name + ':');
var sortedDictionary = from pair in sportPriceInfo
orderby pair.Key ascending
select pair;
foreach (var item in sortedDictionary)
{
Console.WriteLine(" -{0} - {1:0.00}", item.Key, item.Value);
}
}
}
class Database
{
List<Card> allCards = new List<Card>();
public void addCard(string n, string s, double p)
{
int cardIndex = isCardPresent(n);
if (cardIndex >= 0)
{
allCards[cardIndex].addSport(s, p);
}
else
{
allCards.Add(new Card(n, s, p));
}
}
public void checkCard(string n)
{
if (isCardPresent(n) >= 0)
{
Console.WriteLine(n + " is available!");
}
else
{
Console.WriteLine(n + " is not available!");
}
}
private int isCardPresent(string n)
{
//returns the index of the matched item, otherwise returns -1
int cardIndex = -1;
for (int i = 0; i < allCards.Count; i++)
{
if (allCards[i].name == n)
{
cardIndex = i;
break;
}
}
return cardIndex;
}
public void showDataBase()
{
List<Card> sortedCards = allCards.OrderByDescending(o => o.sportCount).ToList();
foreach (var card in sortedCards)
{
card.showInfo();
}
}
}
static void Main()
{
string line;
Database db = new Database();
Match lineSplit;
while ((line = Console.ReadLine()) != "end")
{
if (Regex.IsMatch(line, @"^(w+) - (w+) - (d+.d+)b$"))
{
lineSplit = Regex.Match(line, @"^(w+) - (w+) - (d+.d+)b$");
string inputName = lineSplit.Groups[1].Value;
string inputSport = lineSplit.Groups[2].Value;
double inputPrice = double.Parse(lineSplit.Groups[3].Value);
db.addCard(inputName, inputSport, inputPrice);
}
else
{
lineSplit = Regex.Match(line, @"^check (w+)b$");
db.checkCard(lineSplit.Groups[1].Value);
}
}
db.showDataBase();
}
}









share|improve this question














The task:



You will receive several input lines in one of the following formats:




  • "{card} - {sport} - {price}"

  • "check {card}"


The card and sport are
strings. Price will be a floating point number. You need to keep track
of every card. When you receive a card, a sport and a price,
register the card in the database if it isn't present, otherwise add
the sport and the price. If the card already contains the sport, you
need to overwrite the price. If you receive "check {card}" you need to
check if the card is available or not and print it on the console in
the format:



"{card} is available!" if the card is present and



"{card} is not available!" if the card is not present



You should end your program when you receive the command "end". At that point you should print the cards, ordered by sports’ count in desecending order. Foreach card print their sport and price ordered by sports’ name.




Examples



My Solution



using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.RegularExpressions;

class Program
{
class Card
{
public Card(string n, string s, double p)
{
name = n;
addSport(s, p);
}
public void addSport(string sportName, double price)
{
if (sportPriceInfo.ContainsKey(sportName))
{
sportPriceInfo[sportName] = price;
}
else
{
sportCount++;
sportPriceInfo.Add(sportName, price);
}
}
public string name;
public int sportCount = 0;
Dictionary<string, double> sportPriceInfo = new Dictionary<string, double>();
public void showInfo()
{
Console.WriteLine(name + ':');
var sortedDictionary = from pair in sportPriceInfo
orderby pair.Key ascending
select pair;
foreach (var item in sortedDictionary)
{
Console.WriteLine(" -{0} - {1:0.00}", item.Key, item.Value);
}
}
}
class Database
{
List<Card> allCards = new List<Card>();
public void addCard(string n, string s, double p)
{
int cardIndex = isCardPresent(n);
if (cardIndex >= 0)
{
allCards[cardIndex].addSport(s, p);
}
else
{
allCards.Add(new Card(n, s, p));
}
}
public void checkCard(string n)
{
if (isCardPresent(n) >= 0)
{
Console.WriteLine(n + " is available!");
}
else
{
Console.WriteLine(n + " is not available!");
}
}
private int isCardPresent(string n)
{
//returns the index of the matched item, otherwise returns -1
int cardIndex = -1;
for (int i = 0; i < allCards.Count; i++)
{
if (allCards[i].name == n)
{
cardIndex = i;
break;
}
}
return cardIndex;
}
public void showDataBase()
{
List<Card> sortedCards = allCards.OrderByDescending(o => o.sportCount).ToList();
foreach (var card in sortedCards)
{
card.showInfo();
}
}
}
static void Main()
{
string line;
Database db = new Database();
Match lineSplit;
while ((line = Console.ReadLine()) != "end")
{
if (Regex.IsMatch(line, @"^(w+) - (w+) - (d+.d+)b$"))
{
lineSplit = Regex.Match(line, @"^(w+) - (w+) - (d+.d+)b$");
string inputName = lineSplit.Groups[1].Value;
string inputSport = lineSplit.Groups[2].Value;
double inputPrice = double.Parse(lineSplit.Groups[3].Value);
db.addCard(inputName, inputSport, inputPrice);
}
else
{
lineSplit = Regex.Match(line, @"^check (w+)b$");
db.checkCard(lineSplit.Groups[1].Value);
}
}
db.showDataBase();
}
}






c# performance programming-challenge






share|improve this question













share|improve this question











share|improve this question




share|improve this question










asked Dec 25 at 2:48









Cecobent

405




405












  • Give your code a bit of breathing room. It's a bit difficult to see where elements begin and end. Microsoft has guidelines: docs.microsoft.com/en-us/dotnet/csharp/programming-guide/…
    – Jesse C. Slicer
    Dec 27 at 3:40


















  • Give your code a bit of breathing room. It's a bit difficult to see where elements begin and end. Microsoft has guidelines: docs.microsoft.com/en-us/dotnet/csharp/programming-guide/…
    – Jesse C. Slicer
    Dec 27 at 3:40
















Give your code a bit of breathing room. It's a bit difficult to see where elements begin and end. Microsoft has guidelines: docs.microsoft.com/en-us/dotnet/csharp/programming-guide/…
– Jesse C. Slicer
Dec 27 at 3:40




Give your code a bit of breathing room. It's a bit difficult to see where elements begin and end. Microsoft has guidelines: docs.microsoft.com/en-us/dotnet/csharp/programming-guide/…
– Jesse C. Slicer
Dec 27 at 3:40










1 Answer
1






active

oldest

votes


















6















  public void addSport(string sportName, double price)
{
...



and




  public string name;



In general the convention in C# is that all public members are named with PascalCase:



public void AddSport(..)


and



public string Name;


Where private members or local variables are named with camelCase:



private int sportCount = 0;





  public int sportCount = 0;



Counting the sports with a separate variable is unnecessary and error prone. Instead rely on the Count member of the sportPriceInfo Dictionary:



public int Count => sportPriceInfo.Count;





  public void showInfo()
{
Console.WriteLine(name + ':');
var sortedDictionary = from pair in sportPriceInfo
orderby pair.Key ascending
select pair;
foreach (var item in sortedDictionary)
{
Console.WriteLine(" -{0} - {1:0.00}", item.Key, item.Value);
}
}



In general avoid writing to the console from your model. If you want a model class to be able to write itself then inject a delegate with the appropriate signature:



public delegate void LineWriter(string format, params object parameters);


and use it like this:



  public void showInfo(LineWriter lineWriter)
{
lineWriter("{0}: ", name);
var sortedDictionary = from pair in sportPriceInfo
orderby pair.Key ascending
select pair;
foreach (var item in sortedDictionary)
{
lineWriter(" -{0} - {1:0.00}", item.Key, item.Value);
}
}


The you can call the method like:



card.showInfo(Console.WriteLine);


It is more flexible this way, if you for instance want to write to a log or use Debug.WriteLine






  public void addSport(string sportName, double price)
{
if (sportPriceInfo.ContainsKey(sportName))
{
sportPriceInfo[sportName] = price;
}
else
{
sportCount++;
sportPriceInfo.Add(sportName, price);
}
}



Here the rule says that a second sport with the same name replaces an existing one, so no need to search for it:



public void addSport(string sportName, double price)
{
sportPriceInfo[sportName] = price;
}


this will either replace or add the new sport.






class Database
{
List<Card> allCards = new List<Card>();



Instead of a List<Card> I would use a Dictionary<string, Card> instance as collection for the cards. It will make the maintenance easier:



  public void addCard(string n, string s, double p)
{
if (!allCards.TryGetValue(n, out Card card))
{
card = new Card(n, s, p);
allCards[n] = card;
}
else
{
card.addSport(s, p);
}


or



  public bool checkCard(string n) => allCards.ContainsKey(n); // see below




BTW: avoid using abbreviated names for method arguments. Instead of n, s and p - name, sport and price would be more informative. Remember that arguments are the "interface" that a consumer must rely on - without necessarily knowing about the objects internal behavior.



Again: the name allCards is too "verbose". Why not just cards?






  public void checkCard(string n)
{
if (isCardPresent(n) >= 0)
{
Console.WriteLine(n + " is available!");
}
else
{
Console.WriteLine(n + " is not available!");
}
}



Here again: don't write to the console from your models. Instead return a flag and let the client react accordingly to that:



public bool HasCard(string name) => allCards.ContainsKey.(name); // If using a dictionary instead of a list.





    if (Regex.IsMatch(line, @"^(w+) - (w+) - (d+.d+)b$"))
{
lineSplit = Regex.Match(line, @"^(w+) - (w+) - (d+.d+)b$");



Here you parse the string two times with the same pattern. It is unnecessary because you can use the Success property of the returned Match object from Regex.Match():



Match match = Regex.Match(line, @"^(w+) - (w+) - (d+.d+)b$");

if (match.Success)
{
...
}




About the pattern @"^(w+) - (w+) - (d+.d+)b$":



It doesn't catch names with two or more words or with hyphens or the like.



And if you take a careful look at the input examples in the image, you'll see that the hyphens aren't of equal length, so you'll have to handle both dashes and hyphens.



All in all that could be done with a pattern like:



@"^(?<card>.+) [-–] (?<sport>.+) [-–] (?<price>d+.d+)$"


Here I also name the subexpressions so it is possible to query the match object like:



      string inputName = lineSplit.Groups["card"].Value;
string inputSport = lineSplit.Groups["sport"].Value;
double inputPrice = double.Parse(lineSplit.Groups["price"].Value);





double.Parse(lineSplit.Groups["price"].Value)




When you parse a numeric string you'll have to consider the current locale. So if the input number always uses '.' as decimal separator, you should parse with a IFormatProvider argument:



double.Parse(lineSplit.Groups["price"].Value, CultureInfo.InvariantCulture);


Otherwise the parser may interpret the price differently, if the current culture for instance uses ',' as separator.



If the price is in an invalid format, double.Parse will fail with an exception. You should handle that in a try...catch block, or use double.TryParse(...) instead and handle a false return from that appropriately.





When it comes to prices and currency, it is by the way common to use the decimal type instead of double.





When I try your method with the two provided input examples it prints " is not available!" near the top in both examples?






share|improve this answer























    Your Answer





    StackExchange.ifUsing("editor", function () {
    return StackExchange.using("mathjaxEditing", function () {
    StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
    StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
    });
    });
    }, "mathjax-editing");

    StackExchange.ifUsing("editor", function () {
    StackExchange.using("externalEditor", function () {
    StackExchange.using("snippets", function () {
    StackExchange.snippets.init();
    });
    });
    }, "code-snippets");

    StackExchange.ready(function() {
    var channelOptions = {
    tags: "".split(" "),
    id: "196"
    };
    initTagRenderer("".split(" "), "".split(" "), channelOptions);

    StackExchange.using("externalEditor", function() {
    // Have to fire editor after snippets, if snippets enabled
    if (StackExchange.settings.snippets.snippetsEnabled) {
    StackExchange.using("snippets", function() {
    createEditor();
    });
    }
    else {
    createEditor();
    }
    });

    function createEditor() {
    StackExchange.prepareEditor({
    heartbeatType: 'answer',
    autoActivateHeartbeat: false,
    convertImagesToLinks: false,
    noModals: true,
    showLowRepImageUploadWarning: true,
    reputationToPostImages: null,
    bindNavPrevention: true,
    postfix: "",
    imageUploader: {
    brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
    contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
    allowUrls: true
    },
    onDemand: true,
    discardSelector: ".discard-answer"
    ,immediatelyShowMarkdownHelp:true
    });


    }
    });














    draft saved

    draft discarded


















    StackExchange.ready(
    function () {
    StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f210295%2fsport-cards-task%23new-answer', 'question_page');
    }
    );

    Post as a guest















    Required, but never shown

























    1 Answer
    1






    active

    oldest

    votes








    1 Answer
    1






    active

    oldest

    votes









    active

    oldest

    votes






    active

    oldest

    votes









    6















      public void addSport(string sportName, double price)
    {
    ...



    and




      public string name;



    In general the convention in C# is that all public members are named with PascalCase:



    public void AddSport(..)


    and



    public string Name;


    Where private members or local variables are named with camelCase:



    private int sportCount = 0;





      public int sportCount = 0;



    Counting the sports with a separate variable is unnecessary and error prone. Instead rely on the Count member of the sportPriceInfo Dictionary:



    public int Count => sportPriceInfo.Count;





      public void showInfo()
    {
    Console.WriteLine(name + ':');
    var sortedDictionary = from pair in sportPriceInfo
    orderby pair.Key ascending
    select pair;
    foreach (var item in sortedDictionary)
    {
    Console.WriteLine(" -{0} - {1:0.00}", item.Key, item.Value);
    }
    }



    In general avoid writing to the console from your model. If you want a model class to be able to write itself then inject a delegate with the appropriate signature:



    public delegate void LineWriter(string format, params object parameters);


    and use it like this:



      public void showInfo(LineWriter lineWriter)
    {
    lineWriter("{0}: ", name);
    var sortedDictionary = from pair in sportPriceInfo
    orderby pair.Key ascending
    select pair;
    foreach (var item in sortedDictionary)
    {
    lineWriter(" -{0} - {1:0.00}", item.Key, item.Value);
    }
    }


    The you can call the method like:



    card.showInfo(Console.WriteLine);


    It is more flexible this way, if you for instance want to write to a log or use Debug.WriteLine






      public void addSport(string sportName, double price)
    {
    if (sportPriceInfo.ContainsKey(sportName))
    {
    sportPriceInfo[sportName] = price;
    }
    else
    {
    sportCount++;
    sportPriceInfo.Add(sportName, price);
    }
    }



    Here the rule says that a second sport with the same name replaces an existing one, so no need to search for it:



    public void addSport(string sportName, double price)
    {
    sportPriceInfo[sportName] = price;
    }


    this will either replace or add the new sport.






    class Database
    {
    List<Card> allCards = new List<Card>();



    Instead of a List<Card> I would use a Dictionary<string, Card> instance as collection for the cards. It will make the maintenance easier:



      public void addCard(string n, string s, double p)
    {
    if (!allCards.TryGetValue(n, out Card card))
    {
    card = new Card(n, s, p);
    allCards[n] = card;
    }
    else
    {
    card.addSport(s, p);
    }


    or



      public bool checkCard(string n) => allCards.ContainsKey(n); // see below




    BTW: avoid using abbreviated names for method arguments. Instead of n, s and p - name, sport and price would be more informative. Remember that arguments are the "interface" that a consumer must rely on - without necessarily knowing about the objects internal behavior.



    Again: the name allCards is too "verbose". Why not just cards?






      public void checkCard(string n)
    {
    if (isCardPresent(n) >= 0)
    {
    Console.WriteLine(n + " is available!");
    }
    else
    {
    Console.WriteLine(n + " is not available!");
    }
    }



    Here again: don't write to the console from your models. Instead return a flag and let the client react accordingly to that:



    public bool HasCard(string name) => allCards.ContainsKey.(name); // If using a dictionary instead of a list.





        if (Regex.IsMatch(line, @"^(w+) - (w+) - (d+.d+)b$"))
    {
    lineSplit = Regex.Match(line, @"^(w+) - (w+) - (d+.d+)b$");



    Here you parse the string two times with the same pattern. It is unnecessary because you can use the Success property of the returned Match object from Regex.Match():



    Match match = Regex.Match(line, @"^(w+) - (w+) - (d+.d+)b$");

    if (match.Success)
    {
    ...
    }




    About the pattern @"^(w+) - (w+) - (d+.d+)b$":



    It doesn't catch names with two or more words or with hyphens or the like.



    And if you take a careful look at the input examples in the image, you'll see that the hyphens aren't of equal length, so you'll have to handle both dashes and hyphens.



    All in all that could be done with a pattern like:



    @"^(?<card>.+) [-–] (?<sport>.+) [-–] (?<price>d+.d+)$"


    Here I also name the subexpressions so it is possible to query the match object like:



          string inputName = lineSplit.Groups["card"].Value;
    string inputSport = lineSplit.Groups["sport"].Value;
    double inputPrice = double.Parse(lineSplit.Groups["price"].Value);





    double.Parse(lineSplit.Groups["price"].Value)




    When you parse a numeric string you'll have to consider the current locale. So if the input number always uses '.' as decimal separator, you should parse with a IFormatProvider argument:



    double.Parse(lineSplit.Groups["price"].Value, CultureInfo.InvariantCulture);


    Otherwise the parser may interpret the price differently, if the current culture for instance uses ',' as separator.



    If the price is in an invalid format, double.Parse will fail with an exception. You should handle that in a try...catch block, or use double.TryParse(...) instead and handle a false return from that appropriately.





    When it comes to prices and currency, it is by the way common to use the decimal type instead of double.





    When I try your method with the two provided input examples it prints " is not available!" near the top in both examples?






    share|improve this answer




























      6















        public void addSport(string sportName, double price)
      {
      ...



      and




        public string name;



      In general the convention in C# is that all public members are named with PascalCase:



      public void AddSport(..)


      and



      public string Name;


      Where private members or local variables are named with camelCase:



      private int sportCount = 0;





        public int sportCount = 0;



      Counting the sports with a separate variable is unnecessary and error prone. Instead rely on the Count member of the sportPriceInfo Dictionary:



      public int Count => sportPriceInfo.Count;





        public void showInfo()
      {
      Console.WriteLine(name + ':');
      var sortedDictionary = from pair in sportPriceInfo
      orderby pair.Key ascending
      select pair;
      foreach (var item in sortedDictionary)
      {
      Console.WriteLine(" -{0} - {1:0.00}", item.Key, item.Value);
      }
      }



      In general avoid writing to the console from your model. If you want a model class to be able to write itself then inject a delegate with the appropriate signature:



      public delegate void LineWriter(string format, params object parameters);


      and use it like this:



        public void showInfo(LineWriter lineWriter)
      {
      lineWriter("{0}: ", name);
      var sortedDictionary = from pair in sportPriceInfo
      orderby pair.Key ascending
      select pair;
      foreach (var item in sortedDictionary)
      {
      lineWriter(" -{0} - {1:0.00}", item.Key, item.Value);
      }
      }


      The you can call the method like:



      card.showInfo(Console.WriteLine);


      It is more flexible this way, if you for instance want to write to a log or use Debug.WriteLine






        public void addSport(string sportName, double price)
      {
      if (sportPriceInfo.ContainsKey(sportName))
      {
      sportPriceInfo[sportName] = price;
      }
      else
      {
      sportCount++;
      sportPriceInfo.Add(sportName, price);
      }
      }



      Here the rule says that a second sport with the same name replaces an existing one, so no need to search for it:



      public void addSport(string sportName, double price)
      {
      sportPriceInfo[sportName] = price;
      }


      this will either replace or add the new sport.






      class Database
      {
      List<Card> allCards = new List<Card>();



      Instead of a List<Card> I would use a Dictionary<string, Card> instance as collection for the cards. It will make the maintenance easier:



        public void addCard(string n, string s, double p)
      {
      if (!allCards.TryGetValue(n, out Card card))
      {
      card = new Card(n, s, p);
      allCards[n] = card;
      }
      else
      {
      card.addSport(s, p);
      }


      or



        public bool checkCard(string n) => allCards.ContainsKey(n); // see below




      BTW: avoid using abbreviated names for method arguments. Instead of n, s and p - name, sport and price would be more informative. Remember that arguments are the "interface" that a consumer must rely on - without necessarily knowing about the objects internal behavior.



      Again: the name allCards is too "verbose". Why not just cards?






        public void checkCard(string n)
      {
      if (isCardPresent(n) >= 0)
      {
      Console.WriteLine(n + " is available!");
      }
      else
      {
      Console.WriteLine(n + " is not available!");
      }
      }



      Here again: don't write to the console from your models. Instead return a flag and let the client react accordingly to that:



      public bool HasCard(string name) => allCards.ContainsKey.(name); // If using a dictionary instead of a list.





          if (Regex.IsMatch(line, @"^(w+) - (w+) - (d+.d+)b$"))
      {
      lineSplit = Regex.Match(line, @"^(w+) - (w+) - (d+.d+)b$");



      Here you parse the string two times with the same pattern. It is unnecessary because you can use the Success property of the returned Match object from Regex.Match():



      Match match = Regex.Match(line, @"^(w+) - (w+) - (d+.d+)b$");

      if (match.Success)
      {
      ...
      }




      About the pattern @"^(w+) - (w+) - (d+.d+)b$":



      It doesn't catch names with two or more words or with hyphens or the like.



      And if you take a careful look at the input examples in the image, you'll see that the hyphens aren't of equal length, so you'll have to handle both dashes and hyphens.



      All in all that could be done with a pattern like:



      @"^(?<card>.+) [-–] (?<sport>.+) [-–] (?<price>d+.d+)$"


      Here I also name the subexpressions so it is possible to query the match object like:



            string inputName = lineSplit.Groups["card"].Value;
      string inputSport = lineSplit.Groups["sport"].Value;
      double inputPrice = double.Parse(lineSplit.Groups["price"].Value);





      double.Parse(lineSplit.Groups["price"].Value)




      When you parse a numeric string you'll have to consider the current locale. So if the input number always uses '.' as decimal separator, you should parse with a IFormatProvider argument:



      double.Parse(lineSplit.Groups["price"].Value, CultureInfo.InvariantCulture);


      Otherwise the parser may interpret the price differently, if the current culture for instance uses ',' as separator.



      If the price is in an invalid format, double.Parse will fail with an exception. You should handle that in a try...catch block, or use double.TryParse(...) instead and handle a false return from that appropriately.





      When it comes to prices and currency, it is by the way common to use the decimal type instead of double.





      When I try your method with the two provided input examples it prints " is not available!" near the top in both examples?






      share|improve this answer


























        6












        6








        6







          public void addSport(string sportName, double price)
        {
        ...



        and




          public string name;



        In general the convention in C# is that all public members are named with PascalCase:



        public void AddSport(..)


        and



        public string Name;


        Where private members or local variables are named with camelCase:



        private int sportCount = 0;





          public int sportCount = 0;



        Counting the sports with a separate variable is unnecessary and error prone. Instead rely on the Count member of the sportPriceInfo Dictionary:



        public int Count => sportPriceInfo.Count;





          public void showInfo()
        {
        Console.WriteLine(name + ':');
        var sortedDictionary = from pair in sportPriceInfo
        orderby pair.Key ascending
        select pair;
        foreach (var item in sortedDictionary)
        {
        Console.WriteLine(" -{0} - {1:0.00}", item.Key, item.Value);
        }
        }



        In general avoid writing to the console from your model. If you want a model class to be able to write itself then inject a delegate with the appropriate signature:



        public delegate void LineWriter(string format, params object parameters);


        and use it like this:



          public void showInfo(LineWriter lineWriter)
        {
        lineWriter("{0}: ", name);
        var sortedDictionary = from pair in sportPriceInfo
        orderby pair.Key ascending
        select pair;
        foreach (var item in sortedDictionary)
        {
        lineWriter(" -{0} - {1:0.00}", item.Key, item.Value);
        }
        }


        The you can call the method like:



        card.showInfo(Console.WriteLine);


        It is more flexible this way, if you for instance want to write to a log or use Debug.WriteLine






          public void addSport(string sportName, double price)
        {
        if (sportPriceInfo.ContainsKey(sportName))
        {
        sportPriceInfo[sportName] = price;
        }
        else
        {
        sportCount++;
        sportPriceInfo.Add(sportName, price);
        }
        }



        Here the rule says that a second sport with the same name replaces an existing one, so no need to search for it:



        public void addSport(string sportName, double price)
        {
        sportPriceInfo[sportName] = price;
        }


        this will either replace or add the new sport.






        class Database
        {
        List<Card> allCards = new List<Card>();



        Instead of a List<Card> I would use a Dictionary<string, Card> instance as collection for the cards. It will make the maintenance easier:



          public void addCard(string n, string s, double p)
        {
        if (!allCards.TryGetValue(n, out Card card))
        {
        card = new Card(n, s, p);
        allCards[n] = card;
        }
        else
        {
        card.addSport(s, p);
        }


        or



          public bool checkCard(string n) => allCards.ContainsKey(n); // see below




        BTW: avoid using abbreviated names for method arguments. Instead of n, s and p - name, sport and price would be more informative. Remember that arguments are the "interface" that a consumer must rely on - without necessarily knowing about the objects internal behavior.



        Again: the name allCards is too "verbose". Why not just cards?






          public void checkCard(string n)
        {
        if (isCardPresent(n) >= 0)
        {
        Console.WriteLine(n + " is available!");
        }
        else
        {
        Console.WriteLine(n + " is not available!");
        }
        }



        Here again: don't write to the console from your models. Instead return a flag and let the client react accordingly to that:



        public bool HasCard(string name) => allCards.ContainsKey.(name); // If using a dictionary instead of a list.





            if (Regex.IsMatch(line, @"^(w+) - (w+) - (d+.d+)b$"))
        {
        lineSplit = Regex.Match(line, @"^(w+) - (w+) - (d+.d+)b$");



        Here you parse the string two times with the same pattern. It is unnecessary because you can use the Success property of the returned Match object from Regex.Match():



        Match match = Regex.Match(line, @"^(w+) - (w+) - (d+.d+)b$");

        if (match.Success)
        {
        ...
        }




        About the pattern @"^(w+) - (w+) - (d+.d+)b$":



        It doesn't catch names with two or more words or with hyphens or the like.



        And if you take a careful look at the input examples in the image, you'll see that the hyphens aren't of equal length, so you'll have to handle both dashes and hyphens.



        All in all that could be done with a pattern like:



        @"^(?<card>.+) [-–] (?<sport>.+) [-–] (?<price>d+.d+)$"


        Here I also name the subexpressions so it is possible to query the match object like:



              string inputName = lineSplit.Groups["card"].Value;
        string inputSport = lineSplit.Groups["sport"].Value;
        double inputPrice = double.Parse(lineSplit.Groups["price"].Value);





        double.Parse(lineSplit.Groups["price"].Value)




        When you parse a numeric string you'll have to consider the current locale. So if the input number always uses '.' as decimal separator, you should parse with a IFormatProvider argument:



        double.Parse(lineSplit.Groups["price"].Value, CultureInfo.InvariantCulture);


        Otherwise the parser may interpret the price differently, if the current culture for instance uses ',' as separator.



        If the price is in an invalid format, double.Parse will fail with an exception. You should handle that in a try...catch block, or use double.TryParse(...) instead and handle a false return from that appropriately.





        When it comes to prices and currency, it is by the way common to use the decimal type instead of double.





        When I try your method with the two provided input examples it prints " is not available!" near the top in both examples?






        share|improve this answer















          public void addSport(string sportName, double price)
        {
        ...



        and




          public string name;



        In general the convention in C# is that all public members are named with PascalCase:



        public void AddSport(..)


        and



        public string Name;


        Where private members or local variables are named with camelCase:



        private int sportCount = 0;





          public int sportCount = 0;



        Counting the sports with a separate variable is unnecessary and error prone. Instead rely on the Count member of the sportPriceInfo Dictionary:



        public int Count => sportPriceInfo.Count;





          public void showInfo()
        {
        Console.WriteLine(name + ':');
        var sortedDictionary = from pair in sportPriceInfo
        orderby pair.Key ascending
        select pair;
        foreach (var item in sortedDictionary)
        {
        Console.WriteLine(" -{0} - {1:0.00}", item.Key, item.Value);
        }
        }



        In general avoid writing to the console from your model. If you want a model class to be able to write itself then inject a delegate with the appropriate signature:



        public delegate void LineWriter(string format, params object parameters);


        and use it like this:



          public void showInfo(LineWriter lineWriter)
        {
        lineWriter("{0}: ", name);
        var sortedDictionary = from pair in sportPriceInfo
        orderby pair.Key ascending
        select pair;
        foreach (var item in sortedDictionary)
        {
        lineWriter(" -{0} - {1:0.00}", item.Key, item.Value);
        }
        }


        The you can call the method like:



        card.showInfo(Console.WriteLine);


        It is more flexible this way, if you for instance want to write to a log or use Debug.WriteLine






          public void addSport(string sportName, double price)
        {
        if (sportPriceInfo.ContainsKey(sportName))
        {
        sportPriceInfo[sportName] = price;
        }
        else
        {
        sportCount++;
        sportPriceInfo.Add(sportName, price);
        }
        }



        Here the rule says that a second sport with the same name replaces an existing one, so no need to search for it:



        public void addSport(string sportName, double price)
        {
        sportPriceInfo[sportName] = price;
        }


        this will either replace or add the new sport.






        class Database
        {
        List<Card> allCards = new List<Card>();



        Instead of a List<Card> I would use a Dictionary<string, Card> instance as collection for the cards. It will make the maintenance easier:



          public void addCard(string n, string s, double p)
        {
        if (!allCards.TryGetValue(n, out Card card))
        {
        card = new Card(n, s, p);
        allCards[n] = card;
        }
        else
        {
        card.addSport(s, p);
        }


        or



          public bool checkCard(string n) => allCards.ContainsKey(n); // see below




        BTW: avoid using abbreviated names for method arguments. Instead of n, s and p - name, sport and price would be more informative. Remember that arguments are the "interface" that a consumer must rely on - without necessarily knowing about the objects internal behavior.



        Again: the name allCards is too "verbose". Why not just cards?






          public void checkCard(string n)
        {
        if (isCardPresent(n) >= 0)
        {
        Console.WriteLine(n + " is available!");
        }
        else
        {
        Console.WriteLine(n + " is not available!");
        }
        }



        Here again: don't write to the console from your models. Instead return a flag and let the client react accordingly to that:



        public bool HasCard(string name) => allCards.ContainsKey.(name); // If using a dictionary instead of a list.





            if (Regex.IsMatch(line, @"^(w+) - (w+) - (d+.d+)b$"))
        {
        lineSplit = Regex.Match(line, @"^(w+) - (w+) - (d+.d+)b$");



        Here you parse the string two times with the same pattern. It is unnecessary because you can use the Success property of the returned Match object from Regex.Match():



        Match match = Regex.Match(line, @"^(w+) - (w+) - (d+.d+)b$");

        if (match.Success)
        {
        ...
        }




        About the pattern @"^(w+) - (w+) - (d+.d+)b$":



        It doesn't catch names with two or more words or with hyphens or the like.



        And if you take a careful look at the input examples in the image, you'll see that the hyphens aren't of equal length, so you'll have to handle both dashes and hyphens.



        All in all that could be done with a pattern like:



        @"^(?<card>.+) [-–] (?<sport>.+) [-–] (?<price>d+.d+)$"


        Here I also name the subexpressions so it is possible to query the match object like:



              string inputName = lineSplit.Groups["card"].Value;
        string inputSport = lineSplit.Groups["sport"].Value;
        double inputPrice = double.Parse(lineSplit.Groups["price"].Value);





        double.Parse(lineSplit.Groups["price"].Value)




        When you parse a numeric string you'll have to consider the current locale. So if the input number always uses '.' as decimal separator, you should parse with a IFormatProvider argument:



        double.Parse(lineSplit.Groups["price"].Value, CultureInfo.InvariantCulture);


        Otherwise the parser may interpret the price differently, if the current culture for instance uses ',' as separator.



        If the price is in an invalid format, double.Parse will fail with an exception. You should handle that in a try...catch block, or use double.TryParse(...) instead and handle a false return from that appropriately.





        When it comes to prices and currency, it is by the way common to use the decimal type instead of double.





        When I try your method with the two provided input examples it prints " is not available!" near the top in both examples?







        share|improve this answer














        share|improve this answer



        share|improve this answer








        edited yesterday

























        answered Dec 26 at 18:54









        Henrik Hansen

        6,8081824




        6,8081824






























            draft saved

            draft discarded




















































            Thanks for contributing an answer to Code Review Stack Exchange!


            • Please be sure to answer the question. Provide details and share your research!

            But avoid



            • Asking for help, clarification, or responding to other answers.

            • Making statements based on opinion; back them up with references or personal experience.


            Use MathJax to format equations. MathJax reference.


            To learn more, see our tips on writing great answers.





            Some of your past answers have not been well-received, and you're in danger of being blocked from answering.


            Please pay close attention to the following guidance:


            • Please be sure to answer the question. Provide details and share your research!

            But avoid



            • Asking for help, clarification, or responding to other answers.

            • Making statements based on opinion; back them up with references or personal experience.


            To learn more, see our tips on writing great answers.




            draft saved


            draft discarded














            StackExchange.ready(
            function () {
            StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f210295%2fsport-cards-task%23new-answer', 'question_page');
            }
            );

            Post as a guest















            Required, but never shown





















































            Required, but never shown














            Required, but never shown












            Required, but never shown







            Required, but never shown

































            Required, but never shown














            Required, but never shown












            Required, but never shown







            Required, but never shown







            Popular posts from this blog

            Сан-Квентин

            Алькесар

            Josef Freinademetz