Mixing
Finding Isogram Word
$begingroup$
An isogram (also known as a "nonpattern word") is a word or phrase without a repeating letter, however spaces and hyphens are allowed to appear multiple times.
The IsIsogram() method takes a string and returns boolean value.
- INPUT: anik OUTPUT: true
- INPUT: A nika OUTPUT: false
Approach 01: Using Dictionary<TKey, TValue>
static bool IsIsogram(string str)
{
//create a dictionary with 26 keys and assign false as default value
var storeOccurance = new Dictionary<int, bool>();
for (int i = 0; i <= 25; i++)
{
storeOccurance[i] = false;
}
str = Regex.Replace(str, "[ -]", ""); //ignore whitespace and dash
//iterate over each character of the string
foreach (var ch in str.ToLower())
{
//(ch - 'a') is used to calculate the key. Check if the character key has false as it's value,
//meaning they were never stored
if (!storeOccurance[ch - 'a'])
{
storeOccurance[ch - 'a'] = true; //assign true when they are stored in the dictionary
}
else //otherwise, if a character key has true as it's value then it means that it exists in the dictionary
{
return false; //return false and exits the iteration..Multiple occurance in dictionary
//means the string has repeating character so it's not Isogram
}
}
return true;
}
Approach 02: Using HashSet<T>
static bool IsIsogram(string str)
{
//TRICK: HashSet<T>.Add(T) returns false if an item already exists in HashSet<T>,
//otherwise returns true and add the character to the datastructure
var charHash = new HashSet<char>();
foreach (var ch in str.ToLower())
{
//check if the charHash.Add() returns false, if it does then terminate
//as it means the character already exists
if (!charHash.Add(ch) && char.IsLetter(ch))
return false;
}
return true; //otherwise return true
}
Approach 03: Using LINQ
static bool IsIsogram(string str)
{ //Suppose "anik a" is a given string
return str.Where(char.IsLetter) //filter only the letters ||here- only a, n, i, k, a will be taken
.GroupBy(char.ToLower) //create group by characters(and transform them to lowercase) || here, there will be a group for each character a, n, i, k
.All(g => g.Count() == 1); //for every group, count the number of it's element and check if it's 1
//|| here, 'a' group has 2 elements so it return false though other groups has only one element in their group
}
Given those above approaches:
- Which approach would you recommend regarding readability, clean code and performance?
- Is there any scope for improvement in the existing approaches?
c# programming-challenge comparative-review linq dictionary
edited Jan 22 at 10:14
t3chb0t
35k752124
asked Jan 21 at 20:37
AKdeBergAKdeBerg
9815
$endgroup$
add a comment |
$begingroup$
An isogram (also known as a "nonpattern word") is a word or phrase without a repeating letter, however spaces and hyphens are allowed to appear multiple times.
The IsIsogram() method takes a string and returns boolean value.
- INPUT: anik OUTPUT: true
- INPUT: A nika OUTPUT: false
Approach 01: Using Dictionary<TKey, TValue>
static bool IsIsogram(string str)
{
//create a dictionary with 26 keys and assign false as default value
var storeOccurance = new Dictionary<int, bool>();
for (int i = 0; i <= 25; i++)
{
storeOccurance[i] = false;
}
str = Regex.Replace(str, "[ -]", ""); //ignore whitespace and dash
//iterate over each character of the string
foreach (var ch in str.ToLower())
{
//(ch - 'a') is used to calculate the key. Check if the character key has false as it's value,
//meaning they were never stored
if (!storeOccurance[ch - 'a'])
{
storeOccurance[ch - 'a'] = true; //assign true when they are stored in the dictionary
}
else //otherwise, if a character key has true as it's value then it means that it exists in the dictionary
{
return false; //return false and exits the iteration..Multiple occurance in dictionary
//means the string has repeating character so it's not Isogram
}
}
return true;
}
Approach 02: Using HashSet<T>
static bool IsIsogram(string str)
{
//TRICK: HashSet<T>.Add(T) returns false if an item already exists in HashSet<T>,
//otherwise returns true and add the character to the datastructure
var charHash = new HashSet<char>();
foreach (var ch in str.ToLower())
{
//check if the charHash.Add() returns false, if it does then terminate
//as it means the character already exists
if (!charHash.Add(ch) && char.IsLetter(ch))
return false;
}
return true; //otherwise return true
}
Approach 03: Using LINQ
static bool IsIsogram(string str)
{ //Suppose "anik a" is a given string
return str.Where(char.IsLetter) //filter only the letters ||here- only a, n, i, k, a will be taken
.GroupBy(char.ToLower) //create group by characters(and transform them to lowercase) || here, there will be a group for each character a, n, i, k
.All(g => g.Count() == 1); //for every group, count the number of it's element and check if it's 1
//|| here, 'a' group has 2 elements so it return false though other groups has only one element in their group
}
Given those above approaches:
- Which approach would you recommend regarding readability, clean code and performance?
- Is there any scope for improvement in the existing approaches?
c# programming-challenge comparative-review linq dictionary
edited Jan 22 at 10:14
t3chb0t
35k752124
asked Jan 21 at 20:37
AKdeBergAKdeBerg
9815
$endgroup$
1
$begingroup$
4th solution (after deleting authorized multiple characters) :return str.Count() == str.distinct().count();
$endgroup$
– Guillaume Beauvois
Jan 22 at 11:24
1
$begingroup$
Along the same lines, if you're OK with using side effects of HashSet.Add:var uniques = new HashSet<char>(); return str.Where(char.IsLetter).Select(char.ToLower).All(uniques.Add);
$endgroup$
– benj2240
Jan 22 at 20:43
add a comment |
$begingroup$
An isogram (also known as a "nonpattern word") is a word or phrase without a repeating letter, however spaces and hyphens are allowed to appear multiple times.
The IsIsogram() method takes a string and returns boolean value.
- INPUT: anik OUTPUT: true
- INPUT: A nika OUTPUT: false
Approach 01: Using Dictionary<TKey, TValue>
static bool IsIsogram(string str)
{
//create a dictionary with 26 keys and assign false as default value
var storeOccurance = new Dictionary<int, bool>();
for (int i = 0; i <= 25; i++)
{
storeOccurance[i] = false;
}
str = Regex.Replace(str, "[ -]", ""); //ignore whitespace and dash
//iterate over each character of the string
foreach (var ch in str.ToLower())
{
//(ch - 'a') is used to calculate the key. Check if the character key has false as it's value,
//meaning they were never stored
if (!storeOccurance[ch - 'a'])
{
storeOccurance[ch - 'a'] = true; //assign true when they are stored in the dictionary
}
else //otherwise, if a character key has true as it's value then it means that it exists in the dictionary
{
return false; //return false and exits the iteration..Multiple occurance in dictionary
//means the string has repeating character so it's not Isogram
}
}
return true;
}
Approach 02: Using HashSet<T>
static bool IsIsogram(string str)
{
//TRICK: HashSet<T>.Add(T) returns false if an item already exists in HashSet<T>,
//otherwise returns true and add the character to the datastructure
var charHash = new HashSet<char>();
foreach (var ch in str.ToLower())
{
//check if the charHash.Add() returns false, if it does then terminate
//as it means the character already exists
if (!charHash.Add(ch) && char.IsLetter(ch))
return false;
}
return true; //otherwise return true
}
Approach 03: Using LINQ
static bool IsIsogram(string str)
{ //Suppose "anik a" is a given string
return str.Where(char.IsLetter) //filter only the letters ||here- only a, n, i, k, a will be taken
.GroupBy(char.ToLower) //create group by characters(and transform them to lowercase) || here, there will be a group for each character a, n, i, k
.All(g => g.Count() == 1); //for every group, count the number of it's element and check if it's 1
//|| here, 'a' group has 2 elements so it return false though other groups has only one element in their group
}
Given those above approaches:
- Which approach would you recommend regarding readability, clean code and performance?
- Is there any scope for improvement in the existing approaches?
c# programming-challenge comparative-review linq dictionary
edited Jan 22 at 10:14
t3chb0t
35k752124
asked Jan 21 at 20:37
AKdeBergAKdeBerg
9815
$endgroup$
An isogram (also known as a "nonpattern word") is a word or phrase without a repeating letter, however spaces and hyphens are allowed to appear multiple times.
The IsIsogram() method takes a string and returns boolean value.
- INPUT: anik OUTPUT: true
- INPUT: A nika OUTPUT: false
Approach 01: Using Dictionary<TKey, TValue>
static bool IsIsogram(string str)
{
//create a dictionary with 26 keys and assign false as default value
var storeOccurance = new Dictionary<int, bool>();
for (int i = 0; i <= 25; i++)
{
storeOccurance[i] = false;
}
str = Regex.Replace(str, "[ -]", ""); //ignore whitespace and dash
//iterate over each character of the string
foreach (var ch in str.ToLower())
{
//(ch - 'a') is used to calculate the key. Check if the character key has false as it's value,
//meaning they were never stored
if (!storeOccurance[ch - 'a'])
{
storeOccurance[ch - 'a'] = true; //assign true when they are stored in the dictionary
}
else //otherwise, if a character key has true as it's value then it means that it exists in the dictionary
{
return false; //return false and exits the iteration..Multiple occurance in dictionary
//means the string has repeating character so it's not Isogram
}
}
return true;
}
Approach 02: Using HashSet<T>
static bool IsIsogram(string str)
{
//TRICK: HashSet<T>.Add(T) returns false if an item already exists in HashSet<T>,
//otherwise returns true and add the character to the datastructure
var charHash = new HashSet<char>();
foreach (var ch in str.ToLower())
{
//check if the charHash.Add() returns false, if it does then terminate
//as it means the character already exists
if (!charHash.Add(ch) && char.IsLetter(ch))
return false;
}
return true; //otherwise return true
}
Approach 03: Using LINQ
static bool IsIsogram(string str)
{ //Suppose "anik a" is a given string
return str.Where(char.IsLetter) //filter only the letters ||here- only a, n, i, k, a will be taken
.GroupBy(char.ToLower) //create group by characters(and transform them to lowercase) || here, there will be a group for each character a, n, i, k
.All(g => g.Count() == 1); //for every group, count the number of it's element and check if it's 1
//|| here, 'a' group has 2 elements so it return false though other groups has only one element in their group
}
Given those above approaches:
- Which approach would you recommend regarding readability, clean code and performance?
- Is there any scope for improvement in the existing approaches?
c# programming-challenge comparative-review linq dictionary
c# programming-challenge comparative-review linq dictionary
edited Jan 22 at 10:14
t3chb0t
35k752124
asked Jan 21 at 20:37
AKdeBergAKdeBerg
9815
edited Jan 22 at 10:14
t3chb0t
35k752124
asked Jan 21 at 20:37
AKdeBergAKdeBerg
9815
edited Jan 22 at 10:14
t3chb0t
35k752124
edited Jan 22 at 10:14
t3chb0t
35k752124
edited Jan 22 at 10:14
t3chb0t
35k752124
35k752124
asked Jan 21 at 20:37
AKdeBergAKdeBerg
9815
asked Jan 21 at 20:37
AKdeBergAKdeBerg
9815
asked Jan 21 at 20:37
AKdeBergAKdeBerg
9815
9815
1
$begingroup$
4th solution (after deleting authorized multiple characters) :return str.Count() == str.distinct().count();
$endgroup$
– Guillaume Beauvois
Jan 22 at 11:24
1
$begingroup$
Along the same lines, if you're OK with using side effects of HashSet.Add:var uniques = new HashSet<char>(); return str.Where(char.IsLetter).Select(char.ToLower).All(uniques.Add);
$endgroup$
– benj2240
Jan 22 at 20:43
add a comment |
1
$begingroup$
4th solution (after deleting authorized multiple characters) :return str.Count() == str.distinct().count();
$endgroup$
– Guillaume Beauvois
Jan 22 at 11:24
1
$begingroup$
Along the same lines, if you're OK with using side effects of HashSet.Add:var uniques = new HashSet<char>(); return str.Where(char.IsLetter).Select(char.ToLower).All(uniques.Add);
$endgroup$
– benj2240
Jan 22 at 20:43
1
1
$begingroup$
4th solution (after deleting authorized multiple characters) :
return str.Count() == str.distinct().count();$endgroup$
– Guillaume Beauvois
Jan 22 at 11:24
$begingroup$
4th solution (after deleting authorized multiple characters) :
return str.Count() == str.distinct().count();$endgroup$
– Guillaume Beauvois
Jan 22 at 11:24
1
1
$begingroup$
Along the same lines, if you're OK with using side effects of HashSet.Add:
var uniques = new HashSet<char>(); return str.Where(char.IsLetter).Select(char.ToLower).All(uniques.Add);$endgroup$
– benj2240
Jan 22 at 20:43
$begingroup$
Along the same lines, if you're OK with using side effects of HashSet.Add:
var uniques = new HashSet<char>(); return str.Where(char.IsLetter).Select(char.ToLower).All(uniques.Add);$endgroup$
– benj2240
Jan 22 at 20:43
add a comment |
5 Answers
5
active
oldest
votes
$begingroup$
I like the second approach since it allows you to exit as soon as a false condition is reached and is simpler than the first. I would offer one minor optimization, keep the check for isletter separate and only check for duplicate if isletter is true:
static bool IsIsogram(string str)
{
//TRICK: HashSet<T>.Add(T) returns false if an item already exists in HashSet<T>,
//otherwise returns true and add the character to the datastructure
var charHash = new HashSet<char>();
foreach (var ch in str.ToLower())
{
//check if the charHash.Add() returns false, if it does then terminate
//as it means the character already exists
if(char.IsLetter(ch))
{
if (!charHash.Add(ch))
return false;
}
}
return true; //otherwise return true
}
answered Jan 21 at 21:24
tinstaafltinstaafl
6,7881928
$endgroup$
$begingroup$
Or you could just swap those two inside the sameif, it will short-circuit if the first evaluates tofalse, no need to unnecessary nesting IMO.
$endgroup$
– auhmaan
Jan 22 at 12:51
add a comment |
$begingroup$
Choice of Map Key
Ultimately, as you are querying your map based upon the character, you should really make your key as the character. This makes your regular subtraction of 'a' unnecessary.
var storeOccurance = new Dictionary<char, bool>();
for (char c = 'a'; c <= 'z'; c++)
{
storeOccurance[c] = false;
}
Inconsistent behaviour for non-letter characters
Given a string such as anik00, the first approach will produce a false response, as the duplicate 0s are detected like any other letter. The other two approaches will produce true, as 0s are ignored.
Comments
I don't think these comments are necessary, as the code is clear enough on its own. If you feel the need to add comments to explain what something is doing, you should extract methods instead to make these intentions clear.
Which approach would I choose?
The litmus test is how easy it is to understand. I've not seen any serious LINQ for a number of years, and despite that I can understand your LINQ query without difficulty. If you can create understandable code in fewer lines, it's a good thing.
answered Jan 21 at 21:21
Joe CJoe C
1,055211
$endgroup$
add a comment |
$begingroup$
I think i would go for LINQ approach. For this simple operation, it is short and easy to understand. Don't reinvent the wheel, use LINQ.
Here is another shorter version of LINQ :
public static bool IsIsogram(string input)
{
var preprocessedInput = Regex.Replace (input.ToLower(), "[ -]", "").Dump();
return preprocessedInput.Length == preprocessedInput.Distinct().Count();
}
But i would like to introduce another approach which is recursive. There is no need to use HashSets or Dictionaries within the implementation.
public static bool IsIsogram( string input){
if (string.IsNullOrEmpty(input))
{
throw new ArgumentNullException(nameof(input));
//or we can return true , no character, no repeated character :)
}
var preprocessedInput = Regex.Replace(input.ToLower(), "[ -]", "");
if (input.Length==1)
{
return true;
}
var firstHalf = preprocessedInput.Substring(0,preprocessedInput.Length/2);
var secondHalf = preprocessedInput.Remove(0,preprocessedInput.Length/2);
if (firstHalf.Intersect(secondHalf).Any())
{
return false;
}
return IsIsogram(firstHalf) && IsIsogram(secondHalf);
}
The input string is divided into two string and checked for any intersected characters. If there is an intersecting character then, returns false.
If there is not any intersected character then, each string divided into two substrings and called the method for each recursively.
answered Jan 21 at 23:39
RenasRenas
336
$endgroup$
add a comment |
$begingroup$
I suggest using an array of boolean, so instead of
var storeOccurance = new Dictionary<int, bool>();
use simply
var storeOccurance = new bool[26];
Although if you want to allow a large character set instead of just a-z, say the whole of unicode, the HashSet approach may be appropriate, although in that case you would have to consider surrogates ( the situation where a character is represented by two chars ).
answered Jan 21 at 23:27
George BarwoodGeorge Barwood
440110
$endgroup$
add a comment |
$begingroup$
The third approach is definitely the one to start with. It's the simplest one and unless you don't have to analyze an entire library of texts it would probably be sufficient in 99.99% use-cases.
Without measuring/benchmarking every presented approach it's not possible to say which one would be the fastest one. It also rarely matters. Don't overengineer it if you don't have a good explanation for doing so.
answered Jan 22 at 10:09
t3chb0tt3chb0t
35k752124
$endgroup$
2
$begingroup$
BothAnyandAllwill return as soon as they find a match/mismatch.
$endgroup$
– Pieter Witvoet
Jan 22 at 10:21
$begingroup$
@PieterWitvoet oops, you're so right ;-]
$endgroup$
– t3chb0t
Jan 22 at 10:23
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%2f211936%2ffinding-isogram-word%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
5 Answers
5
active
oldest
votes
5 Answers
5
active
oldest
votes
active
oldest
votes
active
oldest
votes
$begingroup$
I like the second approach since it allows you to exit as soon as a false condition is reached and is simpler than the first. I would offer one minor optimization, keep the check for isletter separate and only check for duplicate if isletter is true:
static bool IsIsogram(string str)
{
//TRICK: HashSet<T>.Add(T) returns false if an item already exists in HashSet<T>,
//otherwise returns true and add the character to the datastructure
var charHash = new HashSet<char>();
foreach (var ch in str.ToLower())
{
//check if the charHash.Add() returns false, if it does then terminate
//as it means the character already exists
if(char.IsLetter(ch))
{
if (!charHash.Add(ch))
return false;
}
}
return true; //otherwise return true
}
answered Jan 21 at 21:24
tinstaafltinstaafl
6,7881928
$endgroup$
$begingroup$
Or you could just swap those two inside the sameif, it will short-circuit if the first evaluates tofalse, no need to unnecessary nesting IMO.
$endgroup$
– auhmaan
Jan 22 at 12:51
add a comment |
$begingroup$
I like the second approach since it allows you to exit as soon as a false condition is reached and is simpler than the first. I would offer one minor optimization, keep the check for isletter separate and only check for duplicate if isletter is true:
static bool IsIsogram(string str)
{
//TRICK: HashSet<T>.Add(T) returns false if an item already exists in HashSet<T>,
//otherwise returns true and add the character to the datastructure
var charHash = new HashSet<char>();
foreach (var ch in str.ToLower())
{
//check if the charHash.Add() returns false, if it does then terminate
//as it means the character already exists
if(char.IsLetter(ch))
{
if (!charHash.Add(ch))
return false;
}
}
return true; //otherwise return true
}
answered Jan 21 at 21:24
tinstaafltinstaafl
6,7881928
$endgroup$
$begingroup$
Or you could just swap those two inside the sameif, it will short-circuit if the first evaluates tofalse, no need to unnecessary nesting IMO.
$endgroup$
– auhmaan
Jan 22 at 12:51
add a comment |
$begingroup$
I like the second approach since it allows you to exit as soon as a false condition is reached and is simpler than the first. I would offer one minor optimization, keep the check for isletter separate and only check for duplicate if isletter is true:
static bool IsIsogram(string str)
{
//TRICK: HashSet<T>.Add(T) returns false if an item already exists in HashSet<T>,
//otherwise returns true and add the character to the datastructure
var charHash = new HashSet<char>();
foreach (var ch in str.ToLower())
{
//check if the charHash.Add() returns false, if it does then terminate
//as it means the character already exists
if(char.IsLetter(ch))
{
if (!charHash.Add(ch))
return false;
}
}
return true; //otherwise return true
}
answered Jan 21 at 21:24
tinstaafltinstaafl
6,7881928
$endgroup$
I like the second approach since it allows you to exit as soon as a false condition is reached and is simpler than the first. I would offer one minor optimization, keep the check for isletter separate and only check for duplicate if isletter is true:
static bool IsIsogram(string str)
{
//TRICK: HashSet<T>.Add(T) returns false if an item already exists in HashSet<T>,
//otherwise returns true and add the character to the datastructure
var charHash = new HashSet<char>();
foreach (var ch in str.ToLower())
{
//check if the charHash.Add() returns false, if it does then terminate
//as it means the character already exists
if(char.IsLetter(ch))
{
if (!charHash.Add(ch))
return false;
}
}
return true; //otherwise return true
}
answered Jan 21 at 21:24
tinstaafltinstaafl
6,7881928
answered Jan 21 at 21:24
tinstaafltinstaafl
6,7881928
answered Jan 21 at 21:24
tinstaafltinstaafl
6,7881928
answered Jan 21 at 21:24
tinstaafltinstaafl
6,7881928
6,7881928
$begingroup$
Or you could just swap those two inside the sameif, it will short-circuit if the first evaluates tofalse, no need to unnecessary nesting IMO.
$endgroup$
– auhmaan
Jan 22 at 12:51
add a comment |
$begingroup$
Or you could just swap those two inside the sameif, it will short-circuit if the first evaluates tofalse, no need to unnecessary nesting IMO.
$endgroup$
– auhmaan
Jan 22 at 12:51
$begingroup$
Or you could just swap those two inside the same
if, it will short-circuit if the first evaluates to false, no need to unnecessary nesting IMO.$endgroup$
– auhmaan
Jan 22 at 12:51
$begingroup$
Or you could just swap those two inside the same
if, it will short-circuit if the first evaluates to false, no need to unnecessary nesting IMO.$endgroup$
– auhmaan
Jan 22 at 12:51
add a comment |
$begingroup$
Choice of Map Key
Ultimately, as you are querying your map based upon the character, you should really make your key as the character. This makes your regular subtraction of 'a' unnecessary.
var storeOccurance = new Dictionary<char, bool>();
for (char c = 'a'; c <= 'z'; c++)
{
storeOccurance[c] = false;
}
Inconsistent behaviour for non-letter characters
Given a string such as anik00, the first approach will produce a false response, as the duplicate 0s are detected like any other letter. The other two approaches will produce true, as 0s are ignored.
Comments
I don't think these comments are necessary, as the code is clear enough on its own. If you feel the need to add comments to explain what something is doing, you should extract methods instead to make these intentions clear.
Which approach would I choose?
The litmus test is how easy it is to understand. I've not seen any serious LINQ for a number of years, and despite that I can understand your LINQ query without difficulty. If you can create understandable code in fewer lines, it's a good thing.
answered Jan 21 at 21:21
Joe CJoe C
1,055211
$endgroup$
add a comment |
$begingroup$
Choice of Map Key
Ultimately, as you are querying your map based upon the character, you should really make your key as the character. This makes your regular subtraction of 'a' unnecessary.
var storeOccurance = new Dictionary<char, bool>();
for (char c = 'a'; c <= 'z'; c++)
{
storeOccurance[c] = false;
}
Inconsistent behaviour for non-letter characters
Given a string such as anik00, the first approach will produce a false response, as the duplicate 0s are detected like any other letter. The other two approaches will produce true, as 0s are ignored.
Comments
I don't think these comments are necessary, as the code is clear enough on its own. If you feel the need to add comments to explain what something is doing, you should extract methods instead to make these intentions clear.
Which approach would I choose?
The litmus test is how easy it is to understand. I've not seen any serious LINQ for a number of years, and despite that I can understand your LINQ query without difficulty. If you can create understandable code in fewer lines, it's a good thing.
answered Jan 21 at 21:21
Joe CJoe C
1,055211
$endgroup$
add a comment |
$begingroup$
Choice of Map Key
Ultimately, as you are querying your map based upon the character, you should really make your key as the character. This makes your regular subtraction of 'a' unnecessary.
var storeOccurance = new Dictionary<char, bool>();
for (char c = 'a'; c <= 'z'; c++)
{
storeOccurance[c] = false;
}
Inconsistent behaviour for non-letter characters
Given a string such as anik00, the first approach will produce a false response, as the duplicate 0s are detected like any other letter. The other two approaches will produce true, as 0s are ignored.
Comments
I don't think these comments are necessary, as the code is clear enough on its own. If you feel the need to add comments to explain what something is doing, you should extract methods instead to make these intentions clear.
Which approach would I choose?
The litmus test is how easy it is to understand. I've not seen any serious LINQ for a number of years, and despite that I can understand your LINQ query without difficulty. If you can create understandable code in fewer lines, it's a good thing.
answered Jan 21 at 21:21
Joe CJoe C
1,055211
$endgroup$
Choice of Map Key
Ultimately, as you are querying your map based upon the character, you should really make your key as the character. This makes your regular subtraction of 'a' unnecessary.
var storeOccurance = new Dictionary<char, bool>();
for (char c = 'a'; c <= 'z'; c++)
{
storeOccurance[c] = false;
}
Inconsistent behaviour for non-letter characters
Given a string such as anik00, the first approach will produce a false response, as the duplicate 0s are detected like any other letter. The other two approaches will produce true, as 0s are ignored.
Comments
I don't think these comments are necessary, as the code is clear enough on its own. If you feel the need to add comments to explain what something is doing, you should extract methods instead to make these intentions clear.
Which approach would I choose?
The litmus test is how easy it is to understand. I've not seen any serious LINQ for a number of years, and despite that I can understand your LINQ query without difficulty. If you can create understandable code in fewer lines, it's a good thing.
answered Jan 21 at 21:21
Joe CJoe C
1,055211
answered Jan 21 at 21:21
Joe CJoe C
1,055211
answered Jan 21 at 21:21
Joe CJoe C
1,055211
answered Jan 21 at 21:21
Joe CJoe C
1,055211
1,055211
add a comment |
add a comment |
$begingroup$
I think i would go for LINQ approach. For this simple operation, it is short and easy to understand. Don't reinvent the wheel, use LINQ.
Here is another shorter version of LINQ :
public static bool IsIsogram(string input)
{
var preprocessedInput = Regex.Replace (input.ToLower(), "[ -]", "").Dump();
return preprocessedInput.Length == preprocessedInput.Distinct().Count();
}
But i would like to introduce another approach which is recursive. There is no need to use HashSets or Dictionaries within the implementation.
public static bool IsIsogram( string input){
if (string.IsNullOrEmpty(input))
{
throw new ArgumentNullException(nameof(input));
//or we can return true , no character, no repeated character :)
}
var preprocessedInput = Regex.Replace(input.ToLower(), "[ -]", "");
if (input.Length==1)
{
return true;
}
var firstHalf = preprocessedInput.Substring(0,preprocessedInput.Length/2);
var secondHalf = preprocessedInput.Remove(0,preprocessedInput.Length/2);
if (firstHalf.Intersect(secondHalf).Any())
{
return false;
}
return IsIsogram(firstHalf) && IsIsogram(secondHalf);
}
The input string is divided into two string and checked for any intersected characters. If there is an intersecting character then, returns false.
If there is not any intersected character then, each string divided into two substrings and called the method for each recursively.
answered Jan 21 at 23:39
RenasRenas
336
$endgroup$
add a comment |
$begingroup$
I think i would go for LINQ approach. For this simple operation, it is short and easy to understand. Don't reinvent the wheel, use LINQ.
Here is another shorter version of LINQ :
public static bool IsIsogram(string input)
{
var preprocessedInput = Regex.Replace (input.ToLower(), "[ -]", "").Dump();
return preprocessedInput.Length == preprocessedInput.Distinct().Count();
}
But i would like to introduce another approach which is recursive. There is no need to use HashSets or Dictionaries within the implementation.
public static bool IsIsogram( string input){
if (string.IsNullOrEmpty(input))
{
throw new ArgumentNullException(nameof(input));
//or we can return true , no character, no repeated character :)
}
var preprocessedInput = Regex.Replace(input.ToLower(), "[ -]", "");
if (input.Length==1)
{
return true;
}
var firstHalf = preprocessedInput.Substring(0,preprocessedInput.Length/2);
var secondHalf = preprocessedInput.Remove(0,preprocessedInput.Length/2);
if (firstHalf.Intersect(secondHalf).Any())
{
return false;
}
return IsIsogram(firstHalf) && IsIsogram(secondHalf);
}
The input string is divided into two string and checked for any intersected characters. If there is an intersecting character then, returns false.
If there is not any intersected character then, each string divided into two substrings and called the method for each recursively.
answered Jan 21 at 23:39
RenasRenas
336
$endgroup$
add a comment |
$begingroup$
I think i would go for LINQ approach. For this simple operation, it is short and easy to understand. Don't reinvent the wheel, use LINQ.
Here is another shorter version of LINQ :
public static bool IsIsogram(string input)
{
var preprocessedInput = Regex.Replace (input.ToLower(), "[ -]", "").Dump();
return preprocessedInput.Length == preprocessedInput.Distinct().Count();
}
But i would like to introduce another approach which is recursive. There is no need to use HashSets or Dictionaries within the implementation.
public static bool IsIsogram( string input){
if (string.IsNullOrEmpty(input))
{
throw new ArgumentNullException(nameof(input));
//or we can return true , no character, no repeated character :)
}
var preprocessedInput = Regex.Replace(input.ToLower(), "[ -]", "");
if (input.Length==1)
{
return true;
}
var firstHalf = preprocessedInput.Substring(0,preprocessedInput.Length/2);
var secondHalf = preprocessedInput.Remove(0,preprocessedInput.Length/2);
if (firstHalf.Intersect(secondHalf).Any())
{
return false;
}
return IsIsogram(firstHalf) && IsIsogram(secondHalf);
}
The input string is divided into two string and checked for any intersected characters. If there is an intersecting character then, returns false.
If there is not any intersected character then, each string divided into two substrings and called the method for each recursively.
answered Jan 21 at 23:39
RenasRenas
336
$endgroup$
I think i would go for LINQ approach. For this simple operation, it is short and easy to understand. Don't reinvent the wheel, use LINQ.
Here is another shorter version of LINQ :
public static bool IsIsogram(string input)
{
var preprocessedInput = Regex.Replace (input.ToLower(), "[ -]", "").Dump();
return preprocessedInput.Length == preprocessedInput.Distinct().Count();
}
But i would like to introduce another approach which is recursive. There is no need to use HashSets or Dictionaries within the implementation.
public static bool IsIsogram( string input){
if (string.IsNullOrEmpty(input))
{
throw new ArgumentNullException(nameof(input));
//or we can return true , no character, no repeated character :)
}
var preprocessedInput = Regex.Replace(input.ToLower(), "[ -]", "");
if (input.Length==1)
{
return true;
}
var firstHalf = preprocessedInput.Substring(0,preprocessedInput.Length/2);
var secondHalf = preprocessedInput.Remove(0,preprocessedInput.Length/2);
if (firstHalf.Intersect(secondHalf).Any())
{
return false;
}
return IsIsogram(firstHalf) && IsIsogram(secondHalf);
}
The input string is divided into two string and checked for any intersected characters. If there is an intersecting character then, returns false.
If there is not any intersected character then, each string divided into two substrings and called the method for each recursively.
answered Jan 21 at 23:39
RenasRenas
336
edited Jan 22 at 12:34
answered Jan 21 at 23:39
RenasRenas
336
answered Jan 21 at 23:39
RenasRenas
336
answered Jan 21 at 23:39
RenasRenas
336
336
add a comment |
add a comment |
$begingroup$
I suggest using an array of boolean, so instead of
var storeOccurance = new Dictionary<int, bool>();
use simply
var storeOccurance = new bool[26];
Although if you want to allow a large character set instead of just a-z, say the whole of unicode, the HashSet approach may be appropriate, although in that case you would have to consider surrogates ( the situation where a character is represented by two chars ).
answered Jan 21 at 23:27
George BarwoodGeorge Barwood
440110
$endgroup$
add a comment |
$begingroup$
I suggest using an array of boolean, so instead of
var storeOccurance = new Dictionary<int, bool>();
use simply
var storeOccurance = new bool[26];
Although if you want to allow a large character set instead of just a-z, say the whole of unicode, the HashSet approach may be appropriate, although in that case you would have to consider surrogates ( the situation where a character is represented by two chars ).
answered Jan 21 at 23:27
George BarwoodGeorge Barwood
440110
$endgroup$
add a comment |
$begingroup$
I suggest using an array of boolean, so instead of
var storeOccurance = new Dictionary<int, bool>();
use simply
var storeOccurance = new bool[26];
Although if you want to allow a large character set instead of just a-z, say the whole of unicode, the HashSet approach may be appropriate, although in that case you would have to consider surrogates ( the situation where a character is represented by two chars ).
answered Jan 21 at 23:27
George BarwoodGeorge Barwood
440110
$endgroup$
I suggest using an array of boolean, so instead of
var storeOccurance = new Dictionary<int, bool>();
use simply
var storeOccurance = new bool[26];
Although if you want to allow a large character set instead of just a-z, say the whole of unicode, the HashSet approach may be appropriate, although in that case you would have to consider surrogates ( the situation where a character is represented by two chars ).
answered Jan 21 at 23:27
George BarwoodGeorge Barwood
440110
answered Jan 21 at 23:27
George BarwoodGeorge Barwood
440110
answered Jan 21 at 23:27
George BarwoodGeorge Barwood
440110
answered Jan 21 at 23:27
George BarwoodGeorge Barwood
440110
440110
add a comment |
add a comment |
$begingroup$
The third approach is definitely the one to start with. It's the simplest one and unless you don't have to analyze an entire library of texts it would probably be sufficient in 99.99% use-cases.
Without measuring/benchmarking every presented approach it's not possible to say which one would be the fastest one. It also rarely matters. Don't overengineer it if you don't have a good explanation for doing so.
answered Jan 22 at 10:09
t3chb0tt3chb0t
35k752124
$endgroup$
2
$begingroup$
BothAnyandAllwill return as soon as they find a match/mismatch.
$endgroup$
– Pieter Witvoet
Jan 22 at 10:21
$begingroup$
@PieterWitvoet oops, you're so right ;-]
$endgroup$
– t3chb0t
Jan 22 at 10:23
add a comment |
$begingroup$
The third approach is definitely the one to start with. It's the simplest one and unless you don't have to analyze an entire library of texts it would probably be sufficient in 99.99% use-cases.
Without measuring/benchmarking every presented approach it's not possible to say which one would be the fastest one. It also rarely matters. Don't overengineer it if you don't have a good explanation for doing so.
answered Jan 22 at 10:09
t3chb0tt3chb0t
35k752124
$endgroup$
2
$begingroup$
BothAnyandAllwill return as soon as they find a match/mismatch.
$endgroup$
– Pieter Witvoet
Jan 22 at 10:21
$begingroup$
@PieterWitvoet oops, you're so right ;-]
$endgroup$
– t3chb0t
Jan 22 at 10:23
add a comment |
$begingroup$
The third approach is definitely the one to start with. It's the simplest one and unless you don't have to analyze an entire library of texts it would probably be sufficient in 99.99% use-cases.
Without measuring/benchmarking every presented approach it's not possible to say which one would be the fastest one. It also rarely matters. Don't overengineer it if you don't have a good explanation for doing so.
answered Jan 22 at 10:09
t3chb0tt3chb0t
35k752124
$endgroup$
The third approach is definitely the one to start with. It's the simplest one and unless you don't have to analyze an entire library of texts it would probably be sufficient in 99.99% use-cases.
Without measuring/benchmarking every presented approach it's not possible to say which one would be the fastest one. It also rarely matters. Don't overengineer it if you don't have a good explanation for doing so.
answered Jan 22 at 10:09
t3chb0tt3chb0t
35k752124
edited Jan 22 at 10:22
answered Jan 22 at 10:09
t3chb0tt3chb0t
35k752124
answered Jan 22 at 10:09
t3chb0tt3chb0t
35k752124
answered Jan 22 at 10:09
t3chb0tt3chb0t
35k752124
35k752124
2
$begingroup$
BothAnyandAllwill return as soon as they find a match/mismatch.
$endgroup$
– Pieter Witvoet
Jan 22 at 10:21
$begingroup$
@PieterWitvoet oops, you're so right ;-]
$endgroup$
– t3chb0t
Jan 22 at 10:23
add a comment |
2
$begingroup$
BothAnyandAllwill return as soon as they find a match/mismatch.
$endgroup$
– Pieter Witvoet
Jan 22 at 10:21
$begingroup$
@PieterWitvoet oops, you're so right ;-]
$endgroup$
– t3chb0t
Jan 22 at 10:23
2
2
$begingroup$
Both
Any and All will return as soon as they find a match/mismatch.$endgroup$
– Pieter Witvoet
Jan 22 at 10:21
$begingroup$
Both
Any and All will return as soon as they find a match/mismatch.$endgroup$
– Pieter Witvoet
Jan 22 at 10:21
$begingroup$
@PieterWitvoet oops, you're so right ;-]
$endgroup$
– t3chb0t
Jan 22 at 10:23
$begingroup$
@PieterWitvoet oops, you're so right ;-]
$endgroup$
– t3chb0t
Jan 22 at 10:23
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.
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%2f211936%2ffinding-isogram-word%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
1
$begingroup$
4th solution (after deleting authorized multiple characters) :
return str.Count() == str.distinct().count();$endgroup$
– Guillaume Beauvois
Jan 22 at 11:24
1
$begingroup$
Along the same lines, if you're OK with using side effects of HashSet.Add:
var uniques = new HashSet<char>(); return str.Where(char.IsLetter).Select(char.ToLower).All(uniques.Add);$endgroup$
– benj2240
Jan 22 at 20:43