-4

What are the most efficient ways to return a value from a loop? FlowId in the following example represents a concatenation of region names followed by "Flow". For example, "NortheastSoutheastFlow", "TexasSoutheastFlow", etc. regionNames represents an array of region names stored as a global variable like ['Northeast','Southeast','Texas']. The regionName function param represents a known region name and the goal of the function is return the other region name included in the flow:

function getOtherFlowRegionName(flowId, regionName)
{
    regionNames.forEach(function(otherRegionName)
    {
        if(flowId.indexOf(otherRegionName) > -1)
        return otherRegionName;
    }
}

Solution can be in either jquery or vanilla js.

user7066345
  • 427
  • 1
  • 5
  • 10
  • 1
    Is your solution not working? – spencer.sm Nov 18 '16 at 22:13
  • 3
    I have less to say about efficiency and more to say about your code structure and syntax: ALWAYS use curly braces around your `if` branches...even when there is only one expression to execute. ALWAYS place the opening curly brace at the end of the same line of the construct that requires it. – Scott Marcus Nov 18 '16 at 22:13
  • It really depends what you're trying to do, and what your concerns are. If you're not concerned about immediately exiting the loop when you hit your value, you can use `.filter()` or `.reduce()`, but if you are, you can use a `.some()` and set a local variable and return that variable. – mhodges Nov 18 '16 at 22:16
  • well, first you would need to find *a* way that actually works. Your current code doesn't work. – Kevin B Nov 18 '16 at 22:19
  • right I want to exit the loop when I hit the expected value. obviously that's the most efficient approach – user7066345 Nov 18 '16 at 22:20
  • 1
    Geez @ScottMarcus, it's great that you really like your coding style, but goodness... –  Nov 18 '16 at 22:21
  • @user7066345: You can use `.find()` to return the first match... `return regionNames.find(function(otherRegionName) { return flowId.indexOf(otherRegionName) > -1 })` –  Nov 18 '16 at 22:23
  • I guess in this case a standard js for loop would be most efficient. you would think that this simplicity would be kept in some way in the more modern looping constructs – user7066345 Nov 18 '16 at 22:23
  • what is `regionNames`? an array? there's a method for this specifically, meaning no need to loop at all. – Kevin B Nov 18 '16 at 22:23
  • @user7066345 the... "more modern" looping constructs would be slower than a simple for loop, fyi (assuming you're referring to things like .each and .some). it's more about what's more readable/maintainable now days. – Kevin B Nov 18 '16 at 22:24
  • @squint - find() is not supported by ie and my app needs to support ie9 – user7066345 Nov 18 '16 at 22:25
  • @user7066345 your question doesn't specify that. – Kevin B Nov 18 '16 at 22:26
  • @user7066345: It's trivial to patch. –  Nov 18 '16 at 22:26
  • @squint It's hardly *my* coding style. These are well-known and advocated JavaScript best-practices. – Scott Marcus Nov 18 '16 at 22:27
  • @kevin - don't a lot of websites still need to support ie? – user7066345 Nov 18 '16 at 22:27
  • @user7066345 sure, but modern IE (and edge) supports quite a bit now. – Kevin B Nov 18 '16 at 22:27
  • @ScottMarcus: I don't care whose it is... there's no need to get that upset over someone else's style. –  Nov 18 '16 at 22:28
  • @kevin - no version of ie supports find() – user7066345 Nov 18 '16 at 22:29
  • https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/find – user7066345 Nov 18 '16 at 22:29
  • 2
    @squint No one seems to be upset except you. This isn't just syntactic sugar. Not adopting these habits can cause bugs in edge cases. – Scott Marcus Nov 18 '16 at 22:29
  • @squint To Scott Marcus' point.. it's more than a stylistic thing. Just ask any celebrity who got their iCloud account hacked.. That breach was due to lack of braces around `if` logic – mhodges Nov 18 '16 at 22:32
  • @ScottMarcus: LOL, when you go shouting at someone because you don't like their style, yeah, that shows you're upset about it. Syntactic sugar? Of course not. I'm inclined to use braces with `if` statements to avoid bugs but others like to elide them in some cases. Putting the opening brace on the next line doesn't lead to bugs. –  Nov 18 '16 at 22:32
  • @squint/marcus this is so off topic to this question... can you clean this up? discuss it over in js chat if you wish. – Kevin B Nov 18 '16 at 22:32
  • 1
    @ScottMarcus: capitalising *always* was probably a mistake on your part, together with the wording (using *always* in the first place). While the advice is good, a more temperate tone, no capitalisation and eventually a good reference on the subject would have done a much better job at **advising**, which I happen to believe was your intention. Or where you really on the verge of a nervous breakdown over a stranger's coding style? – tao Nov 18 '16 at 22:33
  • @squint The use of caps in two places wasn't intended to convey that I'm upset, it was intended to convey importance. And yes, the placement of the curly brace can lead to bugs because of JavaScript's automatic semi-colon insertion. http://stackoverflow.com/questions/3641519/why-does-a-results-vary-based-on-curly-brace-placement – Scott Marcus Nov 18 '16 at 22:35
  • @ScottMarcus: That issue isn't specific to curly braces; it's an issue for any return type. But yes, that would be a problem. I was referring to the code in the question, where the braces denote a function body. There's no ASI in that case. –  Nov 18 '16 at 22:38
  • 2
    maybe you guys can start a separate thread about curly braces? – user7066345 Nov 18 '16 at 22:39
  • @user7066345: Why? –  Nov 18 '16 at 22:40
  • thanks guys i will be abandoning this thread now – user7066345 Nov 18 '16 at 22:41
  • @squint The issue isn't an issue at all (ever) if the curly braces are in the right place, so from where I sit, it seems like a curly brace issue. – Scott Marcus Nov 18 '16 at 22:41
  • @user7066345: Getting half a dozen solutions wasn't sufficient for you? I gave one above and someone else gave 5 below. Why aren't you happy? –  Nov 18 '16 at 22:42
  • @user7066345 I have updated my solution, can you please let me know if it's what you are looking for? – mhodges Nov 18 '16 at 22:43
  • @user7066345: Microsoft support for IE8 has ended. Please note IE9 support will end soon, too (in less than a year, I believe). When the manufacturer no longer supports a piece of software, any client insisting their website must work on that particular piece of software is kindly shown the door. I personally believe the current traffic rates of IE8 and 9 are hugely increased by people testing if a website works in that browser rather than people actually using those versions on a daily basis. – tao Nov 18 '16 at 23:00

2 Answers2

0

Here are 4 options that you can use to return a value from a loop like you have described.

Edit - After re-reading the quesiton, I realized you were looking for the parts of the flowId that did not match the region or "Flow". Is this what you're looking for?

Update - After re-re-reading your question, I am now under the understanding that you are wanting to pass in a regionName, rather than match the first one in the regionNames array.

function getOtherFlowRegionName(flowId, regionName) {
  var otherRegionName = "";
  flowId = flowId.replace(regionName, "");
  // option 1 (iterates over all elements in regionNames)
  regionNames.forEach(function(region) {
    if (flowId.indexOf(region) > -1) {
      otherRegionName = region;
    }
  });
  // option 2 (stops when it reaches the first true value)
  regionNames.some(function(region) {
    if (flowId.indexOf(region) > -1) {
      otherRegionName = region;
      return true;
    }
  });
  // option 3 (iterates over all elements in regionNames)
  otherRegionName = regionNames.reduce(function(name, region) {
    return flowId.indexOf(region) > -1 ? region : name;
  }, "");
  // option 4 (stops when it reaches the first true value)
  for (var i = 0; i < regionNames.length; i++) {
    var region = regionNames[i];
    if (flowId.indexOf(region) > -1) {
      otherRegionName = region;
      break;
    }
  }
  return otherRegionName;
}
var flow1 = "NortheastSoutheastFlow";
var flow2 = "TexasSoutheastFlow";
var flow3 = "TexasNortheastFlow";
var regionNames = ['Northeast', 'Southeast', 'Texas'];

console.log(getOtherFlowRegionName(flow1, "Northeast"));
console.log(getOtherFlowRegionName(flow1, "Southeast"));
console.log(getOtherFlowRegionName(flow2, "Texas"));
console.log(getOtherFlowRegionName(flow2, "Southeast"));
console.log(getOtherFlowRegionName(flow3, "Texas"));
console.log(getOtherFlowRegionName(flow3, "Northeast"));

In my opinion, the correct solution would be using the plain for loop which you can simplify like so:

function getOtherFlowRegionName(flowId, regionName) {
  flowId = flowId.replace(regionName, "");
  for (var i = 0; i < regionNames.length; i++) {
    if (flowId.indexOf(regionNames[i]) > -1) {
      return regionNames[i];
    }
  }
}
Community
  • 1
  • 1
mhodges
  • 10,938
  • 2
  • 28
  • 46
  • I was hoping I could simply return the value directly from my function but it looks like there's no way to avoid declaring a variable outside the loop as a placeholder for the return value unless I use an old school for loop – user7066345 Nov 18 '16 at 22:46
  • 2
    @user7066345 the "old school" for loop would be the simplest solution for looping over something until you find a certain value and then returning. Not sure why you wouldn't want to use it. – Kevin B Nov 18 '16 at 22:48
  • Yeah, not if you want to stop iteration when you find the first value. If you do not care about iterating over excess values (which unless you have a bunch of values in the array it doesn't matter), the reduce solution would work. rather than saying `otherRegionName = regionNames.reduce...` you can just say `return regionNames.reduce...` – mhodges Nov 18 '16 at 22:49
  • That being said, I 100% agree with @KevinB – mhodges Nov 18 '16 at 22:50
  • @AndreiGheorghiu I can see your point. What's strange is that the downvote happened not even 10 seconds after I posted the answer. Also, the question asks for the "most efficient ways", ways being plural. Hence why I posted several options. – mhodges Nov 18 '16 at 22:53
  • 1 problem with the some() approach is that it makes variable naming messier. For example: var otherRegionName1; regionNames.some(function(otherRegionName2){ if(flowId.indexOf(otherRegionName2) > -1) otherRegionName1 = otherRegionName2; return true; }); return otherRegionName1; as opposed to concise like this: regionNames.forEach(function(otherRegionName){ if(flowId.indexOf(otherRegionName) > -1) return otherRegionName; }); Much cleaner and more readable don't you think? Not sure why the js committee did not include a modern option for this – user7066345 Nov 18 '16 at 22:58
  • 1
    if it aint broken don't fix it? though that doesn't explain why they added `Class` – Kevin B Nov 18 '16 at 23:00
  • @user7066345 Except that you can't return a value from a `.forEach()` function. There are other functional approaches to doing so, as I have mentioned with `filter`, `map`, and `reduce`. Also, check my updated solution, I think I got it right this time in terms of what exactly you are trying to accomplish overall – mhodges Nov 18 '16 at 23:04
  • @mhodges I frequently have the impulse to downvote (or look for any tiny reason to downvote) any answer on a question I personally consider bad, the real reason being my belief bad questions should not be answered. I usually restrain myself from the downvote, but that is what probably happened here. If you want to avoid downvotes you should probably avoid answering questions with a negative score. – tao Nov 18 '16 at 23:06
  • as a developer I love new tech but the js committee did not do a good job with these new looping methods. going old school back to the for loop. jeez – user7066345 Nov 18 '16 at 23:06
  • @user7066345 It's about using the right tool for the job. The for loop has its place, just as the functional methods have their place. I personally find it rare that the plain for loop is the best solution, but for this specific example, I believe it is. – mhodges Nov 18 '16 at 23:09
  • 1
    @user7066345: They gave you the `.find()` method that does exactly what you seem to want, but you seem uninterested in that. They can't be expected to go back in time and provide it sooner, but you certainly can include a simple polyfill that'll work back to IE6. –  Nov 18 '16 at 23:10
  • ...and the `for` can be shortened by returning from inside the loop instead of assigning to a variable if it helps you warm up to the traditional approach. –  Nov 18 '16 at 23:11
  • @AndreiGheorghiu Yeah, I understand where you're coming from. I guess my viewpoint on it is what the title of the upvote/downvote buttons says, which is "This answer is useful" or "This answer is not useful". Maybe I'm biased here, but I would find it hard to definitively say that my answer is not useful to folks searching how to do this in the future. That's just my opinion, and I know it does not reflect in any way any sort of hard and fast rule for answer scoring. – mhodges Nov 18 '16 at 23:11
  • @user7066345: As I told you above twice now, you can polyfill it, and it'll work in very old versions. What's the problem? –  Nov 18 '16 at 23:12
  • function getOtherFlowRegionName(flowId, regionName) { for(var i=0; i -1) return regionNames[i]; } – user7066345 Nov 18 '16 at 23:13
  • @mhodges: The usefulness of an answer is directly proportional with the score of the question. Even if the question does chance in the list of related questions for some future ones, the lower the q-score the less chances of it being opened, thus hindering the overall usefulness (or shall I say contribution?) of the answer. A good answer on a bad question is, at best, a waste of resources. – tao Nov 18 '16 at 23:18
  • @AndreiGheorghiu That's a fair point. I didn't downvote because I thought it was a legit question, but I can see why others did downvote it. It's hard to me to judge because many times, people will just immediately downvote a question if they didn't quite understand it right off the bat - so I take question score with a bit of a grain of salt. Especially because it was only -1 or -2 at the time that I posted my answer. – mhodges Nov 18 '16 at 23:24
  • @mhodges If nothing else, you got yourself an up-vote and a little chat with yours truly. Take downvotes on your answers with a grain of salt too. From where I see it, true value (should read *happiness*) lies in the ability to offer/give rather than receive/take. Keep up the good work, man. – tao Nov 18 '16 at 23:30
  • @AndreiGheorghiu I appreciate that, and I agree. It is always my goal to actually help the OP and future visitors. I wear my "Unsung Hero" badge with pride =) – mhodges Nov 18 '16 at 23:33
0

Going old school. Most purposeful, concise, straightforward and performant:

function getOtherFlowRegionName(flowId, regionName)
{
    for(var i=0; i<regionNames.length; i++)
        if(flowId.indexOf(regionNames[i]) > -1) 
            return regionNames[i];
}
user7066345
  • 427
  • 1
  • 5
  • 10
  • See the bottom of my updated answer. You need to parse out the `regionName` from the flowId or you can get flawed results. – mhodges Nov 18 '16 at 23:20
  • Example: `getOtherFlowRegionName("NortheastSoutheastFlow", "Northeast")` will return "Northeast" when it should return "Southeast" – mhodges Nov 18 '16 at 23:26