Get consecutive integer number ranges from list of intConsolidate list of ranges that overlapConverting from binary to unaryFinding the indices of the two items whose price adds up to to a valueHackerEarth - SimpleFunctionFollow-up 1: Compare 2 unordered, rooted trees for shape-isomorphismHackerrank: Max ScoreFilter data with optional parametersAt least 2 paths down the binary tree have the same sumLeetcode 54: Spiral MatrixRecursive function challenge

Mistake in years of experience in resume?

Does a large simulator bay have standard public address announcements?

How would 10 generations of living underground change the human body?

How come there are so many candidates for the 2020 Democratic party presidential nomination?

As an international instructor, should I openly talk about my accent?

Multiple options vs single option UI

How to fry ground beef so it is well-browned

Dynamic SOQL query relationship with field visibility for Users

How to not starve gigantic beasts

Don’t seats that recline flat defeat the purpose of having seatbelts?

"Whatever a Russian does, they end up making the Kalashnikov gun"? Are there any similar proverbs in English?

Why was the Spitfire's elliptical wing almost uncopied by other aircraft of World War 2?

Contradiction proof for inequality of P and NP?

What makes accurate emulation of old systems a difficult task?

Coordinate my way to the name of the (video) game

A ​Note ​on ​N!

Who was the lone kid in the line of people at the lake at the end of Avengers: Endgame?

What's the polite way to say "I need to urinate"?

How do I reattach a shelf to the wall when it ripped out of the wall?

How can Republicans who favour free markets, consistently express anger when they don't like the outcome of that choice?

Is there really no use for MD5 anymore?

Extension of 2-adic valuation to the real numbers

Betweenness centrality formula

Why does Mind Blank stop the Feeblemind spell?



Get consecutive integer number ranges from list of int


Consolidate list of ranges that overlapConverting from binary to unaryFinding the indices of the two items whose price adds up to to a valueHackerEarth - SimpleFunctionFollow-up 1: Compare 2 unordered, rooted trees for shape-isomorphismHackerrank: Max ScoreFilter data with optional parametersAt least 2 paths down the binary tree have the same sumLeetcode 54: Spiral MatrixRecursive function challenge






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








3












$begingroup$


This is an algorithm to return a list of consecutive integer ranges from a list of integers.



The practical use of such an algorithm is to filter lists more efficiently, i.e. rather than check e.g. 1000 items (1..1000; x==1 || x==2 || ... || x==1000), it might be possible to check only 2 items (x >= 1 && x <= 1000).



Does this algorithm have any mistakes, can it be optimized, or are there any other improvements you can suggest?



(As a side note: I know this code does not follow the standard C# naming conventions. I do not like to follow that particular convention; I prefer "snake_case" as it is easier on my eyes.)



Sample output:



(x >= 0 && x <= 1690)
(x >= 13642 && x <= 15331)
(x >= 27283 && x <= 27296)
(x >= 27769 && x <= 27776)
(x >= 28249 && x <= 28256)
(x >= 28729 && x <= 28736)
(x >= 29209 && x <= 29222)


The algorithm (built Visual Studio 2017, C# 7.3, .NET 4.7.2):



public static List<(int from, int to)> get_consecutive_ranges(List<int> fids)



Example use:



// slow:
body = body.Where(a => fids.Contains(a.fid)).ToList();

// fast:
body = body.Where(a => fids_fast.Any(x => a.fid >= x.from && a.fid <= x.to)).ToList();









share|improve this question









New contributor




Aalawlx is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.







$endgroup$







  • 1




    $begingroup$
    @t3chb0t no I just dislike camel case as I personally find it to be quite difficult to read and thus more likely to cause eye strain. Thanks for your suggestion, I will add it to my question.
    $endgroup$
    – Aalawlx
    5 hours ago










  • $begingroup$
    One more suggestion... if you have any running demo console/unit-test code (which I strongly believe) then you should add it too. It'll make it easier to review your algorithm as people will be able to actually execute it.
    $endgroup$
    – t3chb0t
    5 hours ago






  • 1




    $begingroup$
    What does fids stand for? Would help me understand what your algorithm is doing better ( at least I would hope ).
    $endgroup$
    – Shelby115
    4 hours ago










  • $begingroup$
    @Shelby115 actually, your naming makes it more generic, which is good. However, to answer the question, fids means 'features ids'. The reason I am writing this method is to filter criteria-specific feature id numbers from millions of id numbers (integers). The actual use case is machine learning, classification specifically, where millions of features are available, but only a small subset can be used or tested together.
    $endgroup$
    – Aalawlx
    3 hours ago

















3












$begingroup$


This is an algorithm to return a list of consecutive integer ranges from a list of integers.



The practical use of such an algorithm is to filter lists more efficiently, i.e. rather than check e.g. 1000 items (1..1000; x==1 || x==2 || ... || x==1000), it might be possible to check only 2 items (x >= 1 && x <= 1000).



Does this algorithm have any mistakes, can it be optimized, or are there any other improvements you can suggest?



(As a side note: I know this code does not follow the standard C# naming conventions. I do not like to follow that particular convention; I prefer "snake_case" as it is easier on my eyes.)



Sample output:



(x >= 0 && x <= 1690)
(x >= 13642 && x <= 15331)
(x >= 27283 && x <= 27296)
(x >= 27769 && x <= 27776)
(x >= 28249 && x <= 28256)
(x >= 28729 && x <= 28736)
(x >= 29209 && x <= 29222)


The algorithm (built Visual Studio 2017, C# 7.3, .NET 4.7.2):



public static List<(int from, int to)> get_consecutive_ranges(List<int> fids)



Example use:



// slow:
body = body.Where(a => fids.Contains(a.fid)).ToList();

// fast:
body = body.Where(a => fids_fast.Any(x => a.fid >= x.from && a.fid <= x.to)).ToList();









share|improve this question









New contributor




Aalawlx is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.







$endgroup$







  • 1




    $begingroup$
    @t3chb0t no I just dislike camel case as I personally find it to be quite difficult to read and thus more likely to cause eye strain. Thanks for your suggestion, I will add it to my question.
    $endgroup$
    – Aalawlx
    5 hours ago










  • $begingroup$
    One more suggestion... if you have any running demo console/unit-test code (which I strongly believe) then you should add it too. It'll make it easier to review your algorithm as people will be able to actually execute it.
    $endgroup$
    – t3chb0t
    5 hours ago






  • 1




    $begingroup$
    What does fids stand for? Would help me understand what your algorithm is doing better ( at least I would hope ).
    $endgroup$
    – Shelby115
    4 hours ago










  • $begingroup$
    @Shelby115 actually, your naming makes it more generic, which is good. However, to answer the question, fids means 'features ids'. The reason I am writing this method is to filter criteria-specific feature id numbers from millions of id numbers (integers). The actual use case is machine learning, classification specifically, where millions of features are available, but only a small subset can be used or tested together.
    $endgroup$
    – Aalawlx
    3 hours ago













3












3








3





$begingroup$


This is an algorithm to return a list of consecutive integer ranges from a list of integers.



The practical use of such an algorithm is to filter lists more efficiently, i.e. rather than check e.g. 1000 items (1..1000; x==1 || x==2 || ... || x==1000), it might be possible to check only 2 items (x >= 1 && x <= 1000).



Does this algorithm have any mistakes, can it be optimized, or are there any other improvements you can suggest?



(As a side note: I know this code does not follow the standard C# naming conventions. I do not like to follow that particular convention; I prefer "snake_case" as it is easier on my eyes.)



Sample output:



(x >= 0 && x <= 1690)
(x >= 13642 && x <= 15331)
(x >= 27283 && x <= 27296)
(x >= 27769 && x <= 27776)
(x >= 28249 && x <= 28256)
(x >= 28729 && x <= 28736)
(x >= 29209 && x <= 29222)


The algorithm (built Visual Studio 2017, C# 7.3, .NET 4.7.2):



public static List<(int from, int to)> get_consecutive_ranges(List<int> fids)



Example use:



// slow:
body = body.Where(a => fids.Contains(a.fid)).ToList();

// fast:
body = body.Where(a => fids_fast.Any(x => a.fid >= x.from && a.fid <= x.to)).ToList();









share|improve this question









New contributor




Aalawlx is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.







$endgroup$




This is an algorithm to return a list of consecutive integer ranges from a list of integers.



The practical use of such an algorithm is to filter lists more efficiently, i.e. rather than check e.g. 1000 items (1..1000; x==1 || x==2 || ... || x==1000), it might be possible to check only 2 items (x >= 1 && x <= 1000).



Does this algorithm have any mistakes, can it be optimized, or are there any other improvements you can suggest?



(As a side note: I know this code does not follow the standard C# naming conventions. I do not like to follow that particular convention; I prefer "snake_case" as it is easier on my eyes.)



Sample output:



(x >= 0 && x <= 1690)
(x >= 13642 && x <= 15331)
(x >= 27283 && x <= 27296)
(x >= 27769 && x <= 27776)
(x >= 28249 && x <= 28256)
(x >= 28729 && x <= 28736)
(x >= 29209 && x <= 29222)


The algorithm (built Visual Studio 2017, C# 7.3, .NET 4.7.2):



public static List<(int from, int to)> get_consecutive_ranges(List<int> fids)



Example use:



// slow:
body = body.Where(a => fids.Contains(a.fid)).ToList();

// fast:
body = body.Where(a => fids_fast.Any(x => a.fid >= x.from && a.fid <= x.to)).ToList();






c# algorithm interval






share|improve this question









New contributor




Aalawlx is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.











share|improve this question









New contributor




Aalawlx is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.









share|improve this question




share|improve this question








edited 16 mins ago









200_success

132k20158423




132k20158423






New contributor




Aalawlx is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.









asked 6 hours ago









AalawlxAalawlx

1214




1214




New contributor




Aalawlx is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.





New contributor





Aalawlx is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.






Aalawlx is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.







  • 1




    $begingroup$
    @t3chb0t no I just dislike camel case as I personally find it to be quite difficult to read and thus more likely to cause eye strain. Thanks for your suggestion, I will add it to my question.
    $endgroup$
    – Aalawlx
    5 hours ago










  • $begingroup$
    One more suggestion... if you have any running demo console/unit-test code (which I strongly believe) then you should add it too. It'll make it easier to review your algorithm as people will be able to actually execute it.
    $endgroup$
    – t3chb0t
    5 hours ago






  • 1




    $begingroup$
    What does fids stand for? Would help me understand what your algorithm is doing better ( at least I would hope ).
    $endgroup$
    – Shelby115
    4 hours ago










  • $begingroup$
    @Shelby115 actually, your naming makes it more generic, which is good. However, to answer the question, fids means 'features ids'. The reason I am writing this method is to filter criteria-specific feature id numbers from millions of id numbers (integers). The actual use case is machine learning, classification specifically, where millions of features are available, but only a small subset can be used or tested together.
    $endgroup$
    – Aalawlx
    3 hours ago












  • 1




    $begingroup$
    @t3chb0t no I just dislike camel case as I personally find it to be quite difficult to read and thus more likely to cause eye strain. Thanks for your suggestion, I will add it to my question.
    $endgroup$
    – Aalawlx
    5 hours ago










  • $begingroup$
    One more suggestion... if you have any running demo console/unit-test code (which I strongly believe) then you should add it too. It'll make it easier to review your algorithm as people will be able to actually execute it.
    $endgroup$
    – t3chb0t
    5 hours ago






  • 1




    $begingroup$
    What does fids stand for? Would help me understand what your algorithm is doing better ( at least I would hope ).
    $endgroup$
    – Shelby115
    4 hours ago










  • $begingroup$
    @Shelby115 actually, your naming makes it more generic, which is good. However, to answer the question, fids means 'features ids'. The reason I am writing this method is to filter criteria-specific feature id numbers from millions of id numbers (integers). The actual use case is machine learning, classification specifically, where millions of features are available, but only a small subset can be used or tested together.
    $endgroup$
    – Aalawlx
    3 hours ago







1




1




$begingroup$
@t3chb0t no I just dislike camel case as I personally find it to be quite difficult to read and thus more likely to cause eye strain. Thanks for your suggestion, I will add it to my question.
$endgroup$
– Aalawlx
5 hours ago




$begingroup$
@t3chb0t no I just dislike camel case as I personally find it to be quite difficult to read and thus more likely to cause eye strain. Thanks for your suggestion, I will add it to my question.
$endgroup$
– Aalawlx
5 hours ago












$begingroup$
One more suggestion... if you have any running demo console/unit-test code (which I strongly believe) then you should add it too. It'll make it easier to review your algorithm as people will be able to actually execute it.
$endgroup$
– t3chb0t
5 hours ago




$begingroup$
One more suggestion... if you have any running demo console/unit-test code (which I strongly believe) then you should add it too. It'll make it easier to review your algorithm as people will be able to actually execute it.
$endgroup$
– t3chb0t
5 hours ago




1




1




$begingroup$
What does fids stand for? Would help me understand what your algorithm is doing better ( at least I would hope ).
$endgroup$
– Shelby115
4 hours ago




$begingroup$
What does fids stand for? Would help me understand what your algorithm is doing better ( at least I would hope ).
$endgroup$
– Shelby115
4 hours ago












$begingroup$
@Shelby115 actually, your naming makes it more generic, which is good. However, to answer the question, fids means 'features ids'. The reason I am writing this method is to filter criteria-specific feature id numbers from millions of id numbers (integers). The actual use case is machine learning, classification specifically, where millions of features are available, but only a small subset can be used or tested together.
$endgroup$
– Aalawlx
3 hours ago




$begingroup$
@Shelby115 actually, your naming makes it more generic, which is good. However, to answer the question, fids means 'features ids'. The reason I am writing this method is to filter criteria-specific feature id numbers from millions of id numbers (integers). The actual use case is machine learning, classification specifically, where millions of features are available, but only a small subset can be used or tested together.
$endgroup$
– Aalawlx
3 hours ago










2 Answers
2






active

oldest

votes


















3












$begingroup$

As a pre-note, I had to translate everything to camelCase and PascalCase because I was having a very difficult time reading your code. No judgments, just understand I was running short on time and didn't translate it back on the conclusion.



Readability



Use meaningful names.




  • fids I changed to integers as from your description I gather it's the list of integers used to create the ranges.


  • fids_index seems very noisy. Standard convention is i, j, etc. When you reduce the iterator variable down to a single character it's much easier to focus on what actually matters.

  • Booleans should be prefixed with Is or Has or Was etc. So in this case IsFirst and IsLast instead of first and last makes it easier to read as english. You could even consider using var isNotFirst = i != 0; as you only use !first in the algorithm.

  • Last vs Previous. is_conseq_with_last is refering to previous, so I switched it to previous to avoid confusion with last.


  • start_index and end_index sound like they would be 0 and integers.Count. They're the start and end of the range you're tracking, let's name them accordingly: rangeStart and rangeEnd.

If-Statement Ordering & Flow



We've all been there, written an algorithm, tested it, and it works, all done right? Well, sometimes that algorithm can be restructured to express what we're actually doing much clearer.



 if (!first && fids[fids_index - 1] == fids[fids_index] - 1)

is_conseq_with_last = true;

else if (!first)

is_conseq_with_last = false;



There's a common denominator here, !first let's work with that, clearly neither of these happen if it's true.



if (!first)

if (fids[fids_index - 1] == fids[fids_index] - 1)

is_conseq_with_last = true;

else

is_conseq_with_last = false;




Well, now it's obvious that inner if-statement can be reduced.



if (!first)

is_conseq_with_last = fids[fids_index - 1] == fids[fids_index] - 1;



And if you wanted, a ternary operator.



is_conseq_with_last = !first && fids[fids_index - 1] == fids[fids_index] - 1;


And it's now also obvious that you're setting the variable with each iteration, so there's no need to declare it outside the loop.



Similar can be done with the bottom if-statement. Here's what I ended up with altogether.



My Version



I am running short on time so there might be a mistake or two and it's still in my coding style so don't take it for face value and try to apply the concepts I'm showing above yourself.



public static List<(int from, int to)> GetConsecutiveRanges(List<int> integers)

if (integers == null





share|improve this answer









$endgroup$












  • $begingroup$
    I wonder whether it would be more efficient to swap these two calls .OrderBy(a => a).Distinct().- just thinking aloud...
    $endgroup$
    – t3chb0t
    4 hours ago










  • $begingroup$
    @t3chb0t I had exactly the same thought, but in the end, both methods must track values, so performance must surely be similar either way around?
    $endgroup$
    – Aalawlx
    4 hours ago










  • $begingroup$
    @Shelby115 There are some very interesting points in your answer - thanks a lot.
    $endgroup$
    – Aalawlx
    4 hours ago



















1












$begingroup$

Your code is really complicated. First, your code should be abstracted a little more. It is not specific to feature IDs, therefore the terminology should not use these words. The same algorithm can be used to select which pages to print from a document, therefore the variables should be just nums and ranges. To test your current code, I wrote:



[Test]
public void TestRanges()

Assert.AreEqual("", Str(Ranges(new List<int>())));
Assert.AreEqual("1", Str(Ranges(new List<int> 1 )));
Assert.AreEqual("1-5", Str(Ranges(new List<int> 1, 2, 3, 4, 5 )));
Assert.AreEqual("1-3, 5", Str(Ranges(new List<int> 1, 2, 3, 5 )));
Assert.AreEqual("1, 3, 5-6", Str(Ranges(new List<int> 1, 3, 5, 6 )));



I wrote a helper function Str so that I don't have to construct a list of ranges for each test case:



public static string Str(List<(int from, int to)> ranges)

var parts = new List<string>();

foreach (var range in ranges)
if (range.from == range.to)
parts.Add(range.from.ToString());
else
parts.Add(range.@from + "-" + range.to);



return string.Join(", ", parts);



After renaming your function to Ranges, these tests ran successfully. So I was ready to refactor your code. I did not really do this since your code looked too complicated to start with. Instead, I remembered that I had successfully used the following pattern quite often:



var start = ...;
while (start < nums.Count)
var end = ...;
while (end < nums.Count)




With this knowledge I wrote the following code:



public static List<(int from, int to)> Ranges(List<int> nums)

nums = nums.OrderBy(a => a).Distinct().ToList();

var ranges = new List<(int from, int to)>();

var start = 0;
while (start < nums.Count)
var end = start + 1; // the range is from [start, end).

while (end < nums.Count && nums[end - 1] + 1 == nums[end])
end++;


ranges.Add((nums[start], nums[end - 1]));
start = end; // continue after the current range


return ranges;



This code doesn't need any special cases for the last range, or anything else. A range either stops when the end of the numbers is reached, or when the following number is not consecutive. This sounds sensible, and this is exactly what the code is doing.



I removed the check for nums == null since it is not necessary. Collections should never be null, and if they are, the code immediately throws an exception, which is fine.



I also removed the special case for nums.Count == 0 since returning an empty list is much better than returning null. Again, expressions that have collection type should never be null. The test cases cover this case, so there's nothing to worry about.






share|improve this answer











$endgroup$













    Your Answer






    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
    );



    );






    Aalawlx is a new contributor. Be nice, and check out our Code of Conduct.









    draft saved

    draft discarded


















    StackExchange.ready(
    function ()
    StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f219204%2fget-consecutive-integer-number-ranges-from-list-of-int%23new-answer', 'question_page');

    );

    Post as a guest















    Required, but never shown

























    2 Answers
    2






    active

    oldest

    votes








    2 Answers
    2






    active

    oldest

    votes









    active

    oldest

    votes






    active

    oldest

    votes









    3












    $begingroup$

    As a pre-note, I had to translate everything to camelCase and PascalCase because I was having a very difficult time reading your code. No judgments, just understand I was running short on time and didn't translate it back on the conclusion.



    Readability



    Use meaningful names.




    • fids I changed to integers as from your description I gather it's the list of integers used to create the ranges.


    • fids_index seems very noisy. Standard convention is i, j, etc. When you reduce the iterator variable down to a single character it's much easier to focus on what actually matters.

    • Booleans should be prefixed with Is or Has or Was etc. So in this case IsFirst and IsLast instead of first and last makes it easier to read as english. You could even consider using var isNotFirst = i != 0; as you only use !first in the algorithm.

    • Last vs Previous. is_conseq_with_last is refering to previous, so I switched it to previous to avoid confusion with last.


    • start_index and end_index sound like they would be 0 and integers.Count. They're the start and end of the range you're tracking, let's name them accordingly: rangeStart and rangeEnd.

    If-Statement Ordering & Flow



    We've all been there, written an algorithm, tested it, and it works, all done right? Well, sometimes that algorithm can be restructured to express what we're actually doing much clearer.



     if (!first && fids[fids_index - 1] == fids[fids_index] - 1)

    is_conseq_with_last = true;

    else if (!first)

    is_conseq_with_last = false;



    There's a common denominator here, !first let's work with that, clearly neither of these happen if it's true.



    if (!first)

    if (fids[fids_index - 1] == fids[fids_index] - 1)

    is_conseq_with_last = true;

    else

    is_conseq_with_last = false;




    Well, now it's obvious that inner if-statement can be reduced.



    if (!first)

    is_conseq_with_last = fids[fids_index - 1] == fids[fids_index] - 1;



    And if you wanted, a ternary operator.



    is_conseq_with_last = !first && fids[fids_index - 1] == fids[fids_index] - 1;


    And it's now also obvious that you're setting the variable with each iteration, so there's no need to declare it outside the loop.



    Similar can be done with the bottom if-statement. Here's what I ended up with altogether.



    My Version



    I am running short on time so there might be a mistake or two and it's still in my coding style so don't take it for face value and try to apply the concepts I'm showing above yourself.



    public static List<(int from, int to)> GetConsecutiveRanges(List<int> integers)

    if (integers == null





    share|improve this answer









    $endgroup$












    • $begingroup$
      I wonder whether it would be more efficient to swap these two calls .OrderBy(a => a).Distinct().- just thinking aloud...
      $endgroup$
      – t3chb0t
      4 hours ago










    • $begingroup$
      @t3chb0t I had exactly the same thought, but in the end, both methods must track values, so performance must surely be similar either way around?
      $endgroup$
      – Aalawlx
      4 hours ago










    • $begingroup$
      @Shelby115 There are some very interesting points in your answer - thanks a lot.
      $endgroup$
      – Aalawlx
      4 hours ago
















    3












    $begingroup$

    As a pre-note, I had to translate everything to camelCase and PascalCase because I was having a very difficult time reading your code. No judgments, just understand I was running short on time and didn't translate it back on the conclusion.



    Readability



    Use meaningful names.




    • fids I changed to integers as from your description I gather it's the list of integers used to create the ranges.


    • fids_index seems very noisy. Standard convention is i, j, etc. When you reduce the iterator variable down to a single character it's much easier to focus on what actually matters.

    • Booleans should be prefixed with Is or Has or Was etc. So in this case IsFirst and IsLast instead of first and last makes it easier to read as english. You could even consider using var isNotFirst = i != 0; as you only use !first in the algorithm.

    • Last vs Previous. is_conseq_with_last is refering to previous, so I switched it to previous to avoid confusion with last.


    • start_index and end_index sound like they would be 0 and integers.Count. They're the start and end of the range you're tracking, let's name them accordingly: rangeStart and rangeEnd.

    If-Statement Ordering & Flow



    We've all been there, written an algorithm, tested it, and it works, all done right? Well, sometimes that algorithm can be restructured to express what we're actually doing much clearer.



     if (!first && fids[fids_index - 1] == fids[fids_index] - 1)

    is_conseq_with_last = true;

    else if (!first)

    is_conseq_with_last = false;



    There's a common denominator here, !first let's work with that, clearly neither of these happen if it's true.



    if (!first)

    if (fids[fids_index - 1] == fids[fids_index] - 1)

    is_conseq_with_last = true;

    else

    is_conseq_with_last = false;




    Well, now it's obvious that inner if-statement can be reduced.



    if (!first)

    is_conseq_with_last = fids[fids_index - 1] == fids[fids_index] - 1;



    And if you wanted, a ternary operator.



    is_conseq_with_last = !first && fids[fids_index - 1] == fids[fids_index] - 1;


    And it's now also obvious that you're setting the variable with each iteration, so there's no need to declare it outside the loop.



    Similar can be done with the bottom if-statement. Here's what I ended up with altogether.



    My Version



    I am running short on time so there might be a mistake or two and it's still in my coding style so don't take it for face value and try to apply the concepts I'm showing above yourself.



    public static List<(int from, int to)> GetConsecutiveRanges(List<int> integers)

    if (integers == null





    share|improve this answer









    $endgroup$












    • $begingroup$
      I wonder whether it would be more efficient to swap these two calls .OrderBy(a => a).Distinct().- just thinking aloud...
      $endgroup$
      – t3chb0t
      4 hours ago










    • $begingroup$
      @t3chb0t I had exactly the same thought, but in the end, both methods must track values, so performance must surely be similar either way around?
      $endgroup$
      – Aalawlx
      4 hours ago










    • $begingroup$
      @Shelby115 There are some very interesting points in your answer - thanks a lot.
      $endgroup$
      – Aalawlx
      4 hours ago














    3












    3








    3





    $begingroup$

    As a pre-note, I had to translate everything to camelCase and PascalCase because I was having a very difficult time reading your code. No judgments, just understand I was running short on time and didn't translate it back on the conclusion.



    Readability



    Use meaningful names.




    • fids I changed to integers as from your description I gather it's the list of integers used to create the ranges.


    • fids_index seems very noisy. Standard convention is i, j, etc. When you reduce the iterator variable down to a single character it's much easier to focus on what actually matters.

    • Booleans should be prefixed with Is or Has or Was etc. So in this case IsFirst and IsLast instead of first and last makes it easier to read as english. You could even consider using var isNotFirst = i != 0; as you only use !first in the algorithm.

    • Last vs Previous. is_conseq_with_last is refering to previous, so I switched it to previous to avoid confusion with last.


    • start_index and end_index sound like they would be 0 and integers.Count. They're the start and end of the range you're tracking, let's name them accordingly: rangeStart and rangeEnd.

    If-Statement Ordering & Flow



    We've all been there, written an algorithm, tested it, and it works, all done right? Well, sometimes that algorithm can be restructured to express what we're actually doing much clearer.



     if (!first && fids[fids_index - 1] == fids[fids_index] - 1)

    is_conseq_with_last = true;

    else if (!first)

    is_conseq_with_last = false;



    There's a common denominator here, !first let's work with that, clearly neither of these happen if it's true.



    if (!first)

    if (fids[fids_index - 1] == fids[fids_index] - 1)

    is_conseq_with_last = true;

    else

    is_conseq_with_last = false;




    Well, now it's obvious that inner if-statement can be reduced.



    if (!first)

    is_conseq_with_last = fids[fids_index - 1] == fids[fids_index] - 1;



    And if you wanted, a ternary operator.



    is_conseq_with_last = !first && fids[fids_index - 1] == fids[fids_index] - 1;


    And it's now also obvious that you're setting the variable with each iteration, so there's no need to declare it outside the loop.



    Similar can be done with the bottom if-statement. Here's what I ended up with altogether.



    My Version



    I am running short on time so there might be a mistake or two and it's still in my coding style so don't take it for face value and try to apply the concepts I'm showing above yourself.



    public static List<(int from, int to)> GetConsecutiveRanges(List<int> integers)

    if (integers == null





    share|improve this answer









    $endgroup$



    As a pre-note, I had to translate everything to camelCase and PascalCase because I was having a very difficult time reading your code. No judgments, just understand I was running short on time and didn't translate it back on the conclusion.



    Readability



    Use meaningful names.




    • fids I changed to integers as from your description I gather it's the list of integers used to create the ranges.


    • fids_index seems very noisy. Standard convention is i, j, etc. When you reduce the iterator variable down to a single character it's much easier to focus on what actually matters.

    • Booleans should be prefixed with Is or Has or Was etc. So in this case IsFirst and IsLast instead of first and last makes it easier to read as english. You could even consider using var isNotFirst = i != 0; as you only use !first in the algorithm.

    • Last vs Previous. is_conseq_with_last is refering to previous, so I switched it to previous to avoid confusion with last.


    • start_index and end_index sound like they would be 0 and integers.Count. They're the start and end of the range you're tracking, let's name them accordingly: rangeStart and rangeEnd.

    If-Statement Ordering & Flow



    We've all been there, written an algorithm, tested it, and it works, all done right? Well, sometimes that algorithm can be restructured to express what we're actually doing much clearer.



     if (!first && fids[fids_index - 1] == fids[fids_index] - 1)

    is_conseq_with_last = true;

    else if (!first)

    is_conseq_with_last = false;



    There's a common denominator here, !first let's work with that, clearly neither of these happen if it's true.



    if (!first)

    if (fids[fids_index - 1] == fids[fids_index] - 1)

    is_conseq_with_last = true;

    else

    is_conseq_with_last = false;




    Well, now it's obvious that inner if-statement can be reduced.



    if (!first)

    is_conseq_with_last = fids[fids_index - 1] == fids[fids_index] - 1;



    And if you wanted, a ternary operator.



    is_conseq_with_last = !first && fids[fids_index - 1] == fids[fids_index] - 1;


    And it's now also obvious that you're setting the variable with each iteration, so there's no need to declare it outside the loop.



    Similar can be done with the bottom if-statement. Here's what I ended up with altogether.



    My Version



    I am running short on time so there might be a mistake or two and it's still in my coding style so don't take it for face value and try to apply the concepts I'm showing above yourself.



    public static List<(int from, int to)> GetConsecutiveRanges(List<int> integers)

    if (integers == null






    share|improve this answer












    share|improve this answer



    share|improve this answer










    answered 4 hours ago









    Shelby115Shelby115

    1,646517




    1,646517











    • $begingroup$
      I wonder whether it would be more efficient to swap these two calls .OrderBy(a => a).Distinct().- just thinking aloud...
      $endgroup$
      – t3chb0t
      4 hours ago










    • $begingroup$
      @t3chb0t I had exactly the same thought, but in the end, both methods must track values, so performance must surely be similar either way around?
      $endgroup$
      – Aalawlx
      4 hours ago










    • $begingroup$
      @Shelby115 There are some very interesting points in your answer - thanks a lot.
      $endgroup$
      – Aalawlx
      4 hours ago

















    • $begingroup$
      I wonder whether it would be more efficient to swap these two calls .OrderBy(a => a).Distinct().- just thinking aloud...
      $endgroup$
      – t3chb0t
      4 hours ago










    • $begingroup$
      @t3chb0t I had exactly the same thought, but in the end, both methods must track values, so performance must surely be similar either way around?
      $endgroup$
      – Aalawlx
      4 hours ago










    • $begingroup$
      @Shelby115 There are some very interesting points in your answer - thanks a lot.
      $endgroup$
      – Aalawlx
      4 hours ago
















    $begingroup$
    I wonder whether it would be more efficient to swap these two calls .OrderBy(a => a).Distinct().- just thinking aloud...
    $endgroup$
    – t3chb0t
    4 hours ago




    $begingroup$
    I wonder whether it would be more efficient to swap these two calls .OrderBy(a => a).Distinct().- just thinking aloud...
    $endgroup$
    – t3chb0t
    4 hours ago












    $begingroup$
    @t3chb0t I had exactly the same thought, but in the end, both methods must track values, so performance must surely be similar either way around?
    $endgroup$
    – Aalawlx
    4 hours ago




    $begingroup$
    @t3chb0t I had exactly the same thought, but in the end, both methods must track values, so performance must surely be similar either way around?
    $endgroup$
    – Aalawlx
    4 hours ago












    $begingroup$
    @Shelby115 There are some very interesting points in your answer - thanks a lot.
    $endgroup$
    – Aalawlx
    4 hours ago





    $begingroup$
    @Shelby115 There are some very interesting points in your answer - thanks a lot.
    $endgroup$
    – Aalawlx
    4 hours ago














    1












    $begingroup$

    Your code is really complicated. First, your code should be abstracted a little more. It is not specific to feature IDs, therefore the terminology should not use these words. The same algorithm can be used to select which pages to print from a document, therefore the variables should be just nums and ranges. To test your current code, I wrote:



    [Test]
    public void TestRanges()

    Assert.AreEqual("", Str(Ranges(new List<int>())));
    Assert.AreEqual("1", Str(Ranges(new List<int> 1 )));
    Assert.AreEqual("1-5", Str(Ranges(new List<int> 1, 2, 3, 4, 5 )));
    Assert.AreEqual("1-3, 5", Str(Ranges(new List<int> 1, 2, 3, 5 )));
    Assert.AreEqual("1, 3, 5-6", Str(Ranges(new List<int> 1, 3, 5, 6 )));



    I wrote a helper function Str so that I don't have to construct a list of ranges for each test case:



    public static string Str(List<(int from, int to)> ranges)

    var parts = new List<string>();

    foreach (var range in ranges)
    if (range.from == range.to)
    parts.Add(range.from.ToString());
    else
    parts.Add(range.@from + "-" + range.to);



    return string.Join(", ", parts);



    After renaming your function to Ranges, these tests ran successfully. So I was ready to refactor your code. I did not really do this since your code looked too complicated to start with. Instead, I remembered that I had successfully used the following pattern quite often:



    var start = ...;
    while (start < nums.Count)
    var end = ...;
    while (end < nums.Count)




    With this knowledge I wrote the following code:



    public static List<(int from, int to)> Ranges(List<int> nums)

    nums = nums.OrderBy(a => a).Distinct().ToList();

    var ranges = new List<(int from, int to)>();

    var start = 0;
    while (start < nums.Count)
    var end = start + 1; // the range is from [start, end).

    while (end < nums.Count && nums[end - 1] + 1 == nums[end])
    end++;


    ranges.Add((nums[start], nums[end - 1]));
    start = end; // continue after the current range


    return ranges;



    This code doesn't need any special cases for the last range, or anything else. A range either stops when the end of the numbers is reached, or when the following number is not consecutive. This sounds sensible, and this is exactly what the code is doing.



    I removed the check for nums == null since it is not necessary. Collections should never be null, and if they are, the code immediately throws an exception, which is fine.



    I also removed the special case for nums.Count == 0 since returning an empty list is much better than returning null. Again, expressions that have collection type should never be null. The test cases cover this case, so there's nothing to worry about.






    share|improve this answer











    $endgroup$

















      1












      $begingroup$

      Your code is really complicated. First, your code should be abstracted a little more. It is not specific to feature IDs, therefore the terminology should not use these words. The same algorithm can be used to select which pages to print from a document, therefore the variables should be just nums and ranges. To test your current code, I wrote:



      [Test]
      public void TestRanges()

      Assert.AreEqual("", Str(Ranges(new List<int>())));
      Assert.AreEqual("1", Str(Ranges(new List<int> 1 )));
      Assert.AreEqual("1-5", Str(Ranges(new List<int> 1, 2, 3, 4, 5 )));
      Assert.AreEqual("1-3, 5", Str(Ranges(new List<int> 1, 2, 3, 5 )));
      Assert.AreEqual("1, 3, 5-6", Str(Ranges(new List<int> 1, 3, 5, 6 )));



      I wrote a helper function Str so that I don't have to construct a list of ranges for each test case:



      public static string Str(List<(int from, int to)> ranges)

      var parts = new List<string>();

      foreach (var range in ranges)
      if (range.from == range.to)
      parts.Add(range.from.ToString());
      else
      parts.Add(range.@from + "-" + range.to);



      return string.Join(", ", parts);



      After renaming your function to Ranges, these tests ran successfully. So I was ready to refactor your code. I did not really do this since your code looked too complicated to start with. Instead, I remembered that I had successfully used the following pattern quite often:



      var start = ...;
      while (start < nums.Count)
      var end = ...;
      while (end < nums.Count)




      With this knowledge I wrote the following code:



      public static List<(int from, int to)> Ranges(List<int> nums)

      nums = nums.OrderBy(a => a).Distinct().ToList();

      var ranges = new List<(int from, int to)>();

      var start = 0;
      while (start < nums.Count)
      var end = start + 1; // the range is from [start, end).

      while (end < nums.Count && nums[end - 1] + 1 == nums[end])
      end++;


      ranges.Add((nums[start], nums[end - 1]));
      start = end; // continue after the current range


      return ranges;



      This code doesn't need any special cases for the last range, or anything else. A range either stops when the end of the numbers is reached, or when the following number is not consecutive. This sounds sensible, and this is exactly what the code is doing.



      I removed the check for nums == null since it is not necessary. Collections should never be null, and if they are, the code immediately throws an exception, which is fine.



      I also removed the special case for nums.Count == 0 since returning an empty list is much better than returning null. Again, expressions that have collection type should never be null. The test cases cover this case, so there's nothing to worry about.






      share|improve this answer











      $endgroup$















        1












        1








        1





        $begingroup$

        Your code is really complicated. First, your code should be abstracted a little more. It is not specific to feature IDs, therefore the terminology should not use these words. The same algorithm can be used to select which pages to print from a document, therefore the variables should be just nums and ranges. To test your current code, I wrote:



        [Test]
        public void TestRanges()

        Assert.AreEqual("", Str(Ranges(new List<int>())));
        Assert.AreEqual("1", Str(Ranges(new List<int> 1 )));
        Assert.AreEqual("1-5", Str(Ranges(new List<int> 1, 2, 3, 4, 5 )));
        Assert.AreEqual("1-3, 5", Str(Ranges(new List<int> 1, 2, 3, 5 )));
        Assert.AreEqual("1, 3, 5-6", Str(Ranges(new List<int> 1, 3, 5, 6 )));



        I wrote a helper function Str so that I don't have to construct a list of ranges for each test case:



        public static string Str(List<(int from, int to)> ranges)

        var parts = new List<string>();

        foreach (var range in ranges)
        if (range.from == range.to)
        parts.Add(range.from.ToString());
        else
        parts.Add(range.@from + "-" + range.to);



        return string.Join(", ", parts);



        After renaming your function to Ranges, these tests ran successfully. So I was ready to refactor your code. I did not really do this since your code looked too complicated to start with. Instead, I remembered that I had successfully used the following pattern quite often:



        var start = ...;
        while (start < nums.Count)
        var end = ...;
        while (end < nums.Count)




        With this knowledge I wrote the following code:



        public static List<(int from, int to)> Ranges(List<int> nums)

        nums = nums.OrderBy(a => a).Distinct().ToList();

        var ranges = new List<(int from, int to)>();

        var start = 0;
        while (start < nums.Count)
        var end = start + 1; // the range is from [start, end).

        while (end < nums.Count && nums[end - 1] + 1 == nums[end])
        end++;


        ranges.Add((nums[start], nums[end - 1]));
        start = end; // continue after the current range


        return ranges;



        This code doesn't need any special cases for the last range, or anything else. A range either stops when the end of the numbers is reached, or when the following number is not consecutive. This sounds sensible, and this is exactly what the code is doing.



        I removed the check for nums == null since it is not necessary. Collections should never be null, and if they are, the code immediately throws an exception, which is fine.



        I also removed the special case for nums.Count == 0 since returning an empty list is much better than returning null. Again, expressions that have collection type should never be null. The test cases cover this case, so there's nothing to worry about.






        share|improve this answer











        $endgroup$



        Your code is really complicated. First, your code should be abstracted a little more. It is not specific to feature IDs, therefore the terminology should not use these words. The same algorithm can be used to select which pages to print from a document, therefore the variables should be just nums and ranges. To test your current code, I wrote:



        [Test]
        public void TestRanges()

        Assert.AreEqual("", Str(Ranges(new List<int>())));
        Assert.AreEqual("1", Str(Ranges(new List<int> 1 )));
        Assert.AreEqual("1-5", Str(Ranges(new List<int> 1, 2, 3, 4, 5 )));
        Assert.AreEqual("1-3, 5", Str(Ranges(new List<int> 1, 2, 3, 5 )));
        Assert.AreEqual("1, 3, 5-6", Str(Ranges(new List<int> 1, 3, 5, 6 )));



        I wrote a helper function Str so that I don't have to construct a list of ranges for each test case:



        public static string Str(List<(int from, int to)> ranges)

        var parts = new List<string>();

        foreach (var range in ranges)
        if (range.from == range.to)
        parts.Add(range.from.ToString());
        else
        parts.Add(range.@from + "-" + range.to);



        return string.Join(", ", parts);



        After renaming your function to Ranges, these tests ran successfully. So I was ready to refactor your code. I did not really do this since your code looked too complicated to start with. Instead, I remembered that I had successfully used the following pattern quite often:



        var start = ...;
        while (start < nums.Count)
        var end = ...;
        while (end < nums.Count)




        With this knowledge I wrote the following code:



        public static List<(int from, int to)> Ranges(List<int> nums)

        nums = nums.OrderBy(a => a).Distinct().ToList();

        var ranges = new List<(int from, int to)>();

        var start = 0;
        while (start < nums.Count)
        var end = start + 1; // the range is from [start, end).

        while (end < nums.Count && nums[end - 1] + 1 == nums[end])
        end++;


        ranges.Add((nums[start], nums[end - 1]));
        start = end; // continue after the current range


        return ranges;



        This code doesn't need any special cases for the last range, or anything else. A range either stops when the end of the numbers is reached, or when the following number is not consecutive. This sounds sensible, and this is exactly what the code is doing.



        I removed the check for nums == null since it is not necessary. Collections should never be null, and if they are, the code immediately throws an exception, which is fine.



        I also removed the special case for nums.Count == 0 since returning an empty list is much better than returning null. Again, expressions that have collection type should never be null. The test cases cover this case, so there's nothing to worry about.







        share|improve this answer














        share|improve this answer



        share|improve this answer








        edited 16 mins ago

























        answered 24 mins ago









        Roland IlligRoland Illig

        12.2k12050




        12.2k12050




















            Aalawlx is a new contributor. Be nice, and check out our Code of Conduct.









            draft saved

            draft discarded


















            Aalawlx is a new contributor. Be nice, and check out our Code of Conduct.












            Aalawlx is a new contributor. Be nice, and check out our Code of Conduct.











            Aalawlx is a new contributor. Be nice, and check out our Code of Conduct.














            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.




            draft saved


            draft discarded














            StackExchange.ready(
            function ()
            StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f219204%2fget-consecutive-integer-number-ranges-from-list-of-int%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

            Disable / Remove link to Product Items in Cart Planned maintenance scheduled April 23, 2019 at 23:30 UTC (7:30pm US/Eastern) Announcing the arrival of Valued Associate #679: Cesar Manara Unicorn Meta Zoo #1: Why another podcast?How can I limit products that can be bought / added to cart?Remove item from cartHide “Add to Cart” button if specific products are already in cart“Prettifying” the custom options in cart pageCreate link in cart sidebar to view all added items After limit reachedLink products together in checkout/cartHow to Get product from cart and add it againHide action-edit on cart page if simple productRemoving Cart items - ObserverRemove wishlist items when added to cart

            Helsingin valtaus Sisällysluettelo Taustaa | Yleistä sotatoimista | Osapuolet | Taistelut Helsingin ympäristössä | Punaisten antautumissuunnitelma | Taistelujen kulku Helsingissä | Valtauksen jälkeen | Tappiot | Muistaminen | Kirjallisuutta | Lähteet | Aiheesta muualla | NavigointivalikkoTeoksen verkkoversioTeoksen verkkoversioGoogle BooksSisällissota Helsingissä päättyi tasan 95 vuotta sittenSaksalaisten ylivoima jyräsi punaisen HelsinginSuomalaiset kuvaavat sotien jälkiä kaupungeissa – katso kuvat ja tarinat tutuilta kulmiltaHelsingin valtaus 90 vuotta sittenSaksalaiset valtasivat HelsinginHyökkäys HelsinkiinHelsingin valtaus 12.–13.4. 1918Saksalaiset käyttivät ihmiskilpiä Helsingin valtauksessa 1918Teoksen verkkoversioTeoksen verkkoversioSaksalaiset hyökkäävät Etelä-SuomeenTaistelut LeppävaarassaSotilaat ja taistelutLeppävaara 1918 huhtikuussa. KapinatarinaHelsingin taistelut 1918Saksalaisten voitonparaati HelsingissäHelsingin valtausta juhlittiinSaksalaisten Helsinki vuonna 1918Helsingin taistelussa kaatuneet valkokaartilaisetHelsinkiin haudatut taisteluissa kaatuneet punaiset12.4.1918 Helsingin valtauksessa saksalaiset apujoukot vapauttavat kaupunginVapaussodan muistomerkkejä Helsingissä ja pääkaupunkiseudullaCrescendo / Vuoden 1918 Kansalaissodan uhrien muistomerkkim

            Adjektiivitarina Tarinan tekeminen | Esimerkki: ennen | Esimerkki: jälkeen | Navigointivalikko