Sport Cards Task
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
add a comment |
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
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
add a comment |
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
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
c# performance programming-challenge
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
add a comment |
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
add a comment |
1 Answer
1
active
oldest
votes
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?
add a comment |
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
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
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?
add a comment |
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?
add a comment |
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?
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?
edited yesterday
answered Dec 26 at 18:54
Henrik Hansen
6,8081824
6,8081824
add a comment |
add a comment |
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.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
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
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