Simple animation with Javascript and JQuery I'm learning Javascript and JQuery, and just finished a chapter on JQuery animations.So i decided to give it a try and implement a simple animation on Html document i had.

Here is the code:

jQuery(document).ready(function($) {

    var animInfo = {

        iter         : 1,      // Iterator
        elementCount : 4,      // Number of (

 ) structures to animate
        socAnimSpeed : 500,    // soc  - Section One Content Animation Speed
        socpAnimSpeed: 1000    // scop - Section One Content
 Animation Speed

    };

    animSoc( animInfo );       // Start the Animations

});

/*  Logic Behind the Functions

    function
    animSoc(obj) ---- If Iterator is <= to Number of Elements,<<<<<<
                |                      v                            ^
                |                      v                            ^
                 ---- Animate Section One Content( Iter value )     ^
                                   ( Width )                        ^
                                       v                            ^
    function                           v                            ^
    animSocp(obj) --- Animate Section One Content( Iter value )
 ^
                |                  ( FadeIn )                       ^
                |                      v                            ^
                |                      v                            ^
                 ---- Increase the Iterator by One   >>>>>>>>>>>>>>>
*/

function animSoc( obj ){

    if ( obj.iter <= obj.elementCount ){

        $( ".soc" + obj.iter ).animate( { width: "60%" },   obj.socAnimSpeed,
            function(){
                animSocp( obj );
            });
    }
    return;
}

function animSocp( obj ){

    $( ".soc" + obj.iter + " + p" ).fadeIn( obj.socpAnimSpeed,
        function() {
            obj.iter++;
            animSoc( obj );
        });
}

The things I am most concerned about :

Is this considered bad practice ? If yes why ?
Commenting of the Code Sample. Is it good or bad ?
Errors.
Your professional opinion.

Naming

Use meaningful names.

config sounds better than animInfo.
iter- Although the comment states what the variable contains it's better to name it iterator and remove the comment.
animSoc: What does it do? Animate the width of element to some %. This can be named as animateWidth.
Similarly, animSocp can be renamed to fadeInParagraphThis might need comment to explain its purpose

Globals. Required?

animSoc and animSocp are global variables. As, these functions are only called from ready they can be moved inside it. See Why are globals bad?

Once, these are moved inside ready the parameters to the function need not to be passed as they will be accessible.

Passing $ as argument to ready

This is done when there are other libraries which uses $ to access it's features or $ is global variable defined. When $ passed as argument to ready, instead of using jQuery each time $ can be used(saves typing). Here, $ is used outside of ready. This means, there are no conflicts on $ and thus using jQuery to refer to jQuery object is not required and passing $ as parameter is not required.

return;

At the end of animSoc, return statement is used but nothing is returned. Also, when this function is called the returned value is never used, so, it can be removed.

Static elementCount

What if tomorrow other developer(or future you) remove/add some elements in the page but forgot to update this value?. I'll suggest to use jQuery to get the number of elements in DOM.

elementCount: $('div.myClass').length,

$('div[class^="soc"]').length can also be used when there is only one class applied to the elements. As, corresponding HTML code is not shown, I'll suggest to add one more class to all elements and use it as selector. In such case eq can be used which works with index.

jslint/jshint

Use JSLint/JSHint regularly to check for errors and potential problems in code.

Here's code with above changes.

$(document).ready(function() {
    'use strict';

    var config = {
        iterator: 1,
        elementCount: $('div[class^="soc"]').length,
        socAnimSpeed: 500, // soc  - Section One Content Animation Speed
        socpAnimSpeed: 1000 // scop - Section One Content <p> Animation Speed
    };

    animateWidth(config);

    function animateWidth() {
        if (config.iterator <= config.elementCount) {
            $(".soc" + config.iterator).animate({
                width: "60%"
            }, config.socAnimSpeed, fadeInParagraph);
        }
    }

    function fadeInParagraph() {
        $(".soc" + config.iterator + " + p").fadeIn(config.socpAnimSpeed,
            function() {
                config.iterator++;
                animateWidth(config);
            });
    }
});


Draw sine wave going around a circle I have used d3 to draw a sine wave going around a circle. This is the very first time I've used d3 or drawn a SVG and I'm fairly new to JS as well, so I don't know if I've overcomplicated it/if there is a simpler way to achieve this. I'd appreciate some feedback - especially if there's any way to make my code more concise.

See my codepen.

const svg = d3.select('svg');
const margin = { top: 50, right: 50, bottom: 50, left: 50 };
const width = +svg.attr('width') - margin.left - margin.right;
const height = +svg.attr('height') - margin.top - margin.bottom;

// content area of your visualization
const vis = svg
.append('g')
.attr('transform', `translate(${margin.left+width/2},${margin.top+height/2})`);

// show area inside of margins
const rect = vis
.append('rect')
.attr('class', 'content')
.attr('width', width)
.attr('height', height)
.attr('transform', `translate(${-width/2},${-height/2})`);

// show scales
const xScale = d3
.scaleLinear()
.domain([-100, 100])
.range([-width/2, width/2]);
const yScale = d3
.scaleLinear()
.domain([100, -100])
.range([-height/2, height/2]);
vis.append('g').call(d3.axisTop(xScale));
vis.append('g').call(d3.axisLeft(yScale));

// draw circle
const pi = Math.PI
const radius = 63.66
const circle = vis
.append('circle')
.style('stroke-dasharray', '3, 3')
.style('stroke', 'black')
.style("fill", "transparent")
.attr("r", xScale(radius))
.attr("cx", 0)
.attr("cy", 0)

// get coordinates for a sine wave
const getSineWave = ({
  numWaves,
  wavelength,
  amplitude,
  phase,
  numPointsPerWave,
}) => {
  return (
    d3.range(numWaves*numPointsPerWave+1).map(function(k) {
      const x = k * wavelength/numPointsPerWave
      return [x, amplitude * Math.sin(phase + 2 * pi * x/wavelength)];
    })
  )
}

// tranform a coordinate from linear space to circular space
const rotate = (cx, cy, x, y, radius) => {
  const theta = x/radius,
        sin = Math.sin(theta),
        cos = Math.cos(theta),
        nx = cx + (radius + y) * sin,
        ny = cy + (radius + y) * cos
  return [nx, ny];
}

// generate sine wave
const numWaves = 4
const amplitude = 10
const phase = pi/2
const circumference = 2 * pi * radius
const wavelength = circumference / numWaves
const numPointsPerWave = 4
const sineWave = getSineWave({
  numWaves,
  numPointsPerWave,
  wavelength,
  amplitude,
  phase,
  wavelength
})
var rotatedSine = sineWave.map( d => {
  const rotatedCoords = rotate(0, 0, d[0], d[1], radius)
  return rotatedCoords
})
// remove the last point as it would overlap the first point of the circle
rotatedSine.pop()

// get Path commands for given coordinates
const getPath = d3.line()
.x(d => xScale(d[0]))
.y(d => yScale(d[1]))
.curve(d3.curveCardinalClosed)

// draw sine wave going around a circle
const wave = vis
.append('path')
.attr('d', getPath(rotatedSine))
.attr('fill', 'none')
.attr('stroke', 'black')
.attr('stroke-width', '1px')
svg {
        background-color: steelblue;
    }

    .content {
        fill: lightsteelblue;
    }
<script src="https://d3js.org/d3.v4.js"></script>
<svg width="1000" height="1000" </ svg>

Overall you have a good D3 code here. I'm fairly impressed with the questions I've seen here at C.O. lately, from people claiming "This is the very first time I've used d3 or drawn a SVG". Congrats.

However, before sharing my proposed alternative, I'd like to tell you that, unfortunately, you're using the wrong tool for the task!

As you can see in my answer here, the problem is that D3 is designed to create visualizations based on data, normally qualitative or discrete quantitative data sets. According to Mike Bostock, D3 creator:

  D3 is designed primarily for data visualization, mostly empirical datasets rather than continuous functions, and so there is no built-in method for generating abscissa values. (emphasis mine)

As you can see in your case, the line gets better if you push more datapoint into the array, increasing any of the two constants in...

d3.range(numWaves*numPointsPerWave+1)

In your particular case we can get a good line with numPointsPerWave = 10, which is not a big problem... however, the advice remains: D3 is not the correct tool here, you should look for a proper plotting library. As you can see in the linked answer above, in some situations we have to increase the data points a lot to have a good looking graph.

D3 radial line

All that being said, here is my proposed alternative: instead of all that complicated math and 2 functions to set the path's d attribute, use a D3 radial line generator.

In this answer I'll focus only on the use of the radial line generator, nothing more. I'm sure that other users will soon post answers regarding your JavaScript code (use of functions, constants, destructuring, currying etc...)

According to the API, d3.lineRadial():

  Constructs a new radial line generator with the default settings. A radial line generator is equivalent to the standard Cartesian line generator, except the x and y accessors are replaced with angle and radius accessors. Radial lines are always positioned relative to ⟨0,0⟩; use a transform (see: SVG, Canvas) to change the origin.

So, all we need is the line generator...

const radialGenerator = d3.lineRadial()
    .angle(d => d.angle)
    .radius(d => d.radius)
    .curve(d3.curveCardinalClosed);

And the adequate data:

const length = 100;
const amplitude = 20;
const radialScale = d3.scaleLinear()
  .domain([0, length])
  .range([0, Math.PI * 2]);

const data = d3.range(length).map(function(d) {
  return {
    angle: radialScale(d),
    radius: xScale(radius) + Math.sin(d) * amplitude
  }
});

Then, you append the path:

const wave = vis.append('path')
  .attr('d', radialGenerator(data))
  .attr('fill', 'none')
  .attr('stroke', 'black')
  .attr('stroke-width', '1px');

Have in mind that my function here is not as complex as yours, which accepts several different parameters: this answer is just to show you the existence of d3.lineRadial(), you can certainly improve it.

Here is the demo:

const svg = d3.select('svg');
const margin = {
  top: 50,
  right: 50,
  bottom: 50,
  left: 50
};
const width = +svg.attr('width') - margin.left - margin.right;
const height = +svg.attr('height') - margin.top - margin.bottom;

// content area of your visualization
const vis = svg.append('g')
  .attr('transform', `translate(${margin.left+width/2},${margin.top+height/2})`);

// show area inside of margins
const rect = vis.append('rect')
  .attr('class', 'content')
  .attr('width', width)
  .attr('height', height)
  .attr('transform', `translate(${-width/2},${-height/2})`);

// show scales
const xScale = d3.scaleLinear()
  .domain([-100, 100])
  .range([-width / 2, width / 2]);
const yScale = d3.scaleLinear()
  .domain([100, -100])
  .range([-height / 2, height / 2]);
vis.append('g').call(d3.axisTop(xScale));
vis.append('g').call(d3.axisLeft(yScale));

// draw circle
const pi = Math.PI
const radius = 63.66
const circle = vis.append('circle')
  .style('stroke-dasharray', '3, 3')
  .style('stroke', 'black')
  .style("fill", "transparent")
  .attr("r", xScale(radius))
  .attr("cx", 0)
  .attr("cy", 0);

const length = 100;
const amplitude = 20;

const radialGenerator = d3.lineRadial()
  .angle(d => d.angle)
  .radius(d => d.radius)
  .curve(d3.curveCardinalClosed)

const radialScale = d3.scaleLinear()
  .domain([0, length])
  .range([0, Math.PI * 2]);

const data = d3.range(length).map(function(d) {
  return {
    angle: radialScale(d),
    radius: xScale(radius) + Math.sin(d) * amplitude
  }
});

const wave = vis.append('path')
  .attr('d', radialGenerator(data))
  .attr('fill', 'none')
  .attr('stroke', 'black')
  .attr('stroke-width', '1px')
svg {
  background-color: steelblue;
}

.content {
  fill: lightsteelblue;
}
<script src="https://d3js.org/d3.v5.min.js"></script>
<svg width="1000" height="1000"></svg>


Calculator with order of operations I am trying to make an additional option in my beginning calculator program, which is based on the BigDecimal class: the simultaneous solution for the entire operation by order of operations. I want to create methods by myself, however it took me a long time just to create a sketch, and for a moment I wonder if I chose a right way.

Right now, my only concern is the logic of this task, which I will implement in my program later. Does it make any sense to base such options, which will use a variety of number types, on a simple "looking for brackets" and operations on Strings? Does anyone see any flaws in that method?

import java.util.*;

public class OrderOfOperations {
    public static void main(String[] args){
        Scanner scanner = new Scanner(System.in);
        String s = scanner.nextLine();
        String w  = result(s);
        System.out.println("Result: "+w);
    }
    public static String before(String s, int i){       //number before operation mark
        String str = s.substring(0,i);
        String num = "";
        for(int b=str.length()-1;b>=0;b--){
            if(b==0 || !Character.isDigit(str.charAt(b-1))&&str.charAt(b-1)!='.'){
                num=str.substring(b);
                break;
            }
        }
        return num;
    }

    public static String after(String s, int i){          //number after operation mark
        String str = s.substring(i+1);
        String num = "";
        for(int a=0;a<=str.length();a++){
            if(a==str.length() || !Character.isDigit(str.charAt(a))&&str.charAt(a)!='.'){
                num=str.substring(0,a);
                break;
            }
        }
        return num;
    }
    public static String result(String s){
        char o = '(';
        char c = ')';
        while(s.contains(Character.toString(o))||s.contains(Character.toString(c))){
            String in;
            if(s.contains(Character.toString(o))){
                System.out.println(s);
                in = s.substring(s.indexOf(o)+1);
                System.out.println("ino: "+in);
                s = s.substring(0, s.indexOf(o))+ result(in);
            }else if(s.contains(Character.toString(c))){
                in = s.substring(0, s.indexOf(c));
                System.out.println("inc: "+in);
                return count(in)+s.substring(s.indexOf(c)+1);
            }
        }
        return count(s);
    }
    public static String count(String s){
        String num1;
        double n1;
        String num2;
        double n2;

        for(int i=0; i<s.length();i++){
            if(s.charAt(i)=='*'||s.charAt(i)=='/'){
                num1 = before(s,i);
                System.out.println("nu1:" +num1);
                num2 = after(s,i);
                System.out.println("nu2:" +num2);
                n1 = Double.parseDouble(num1);
                n2 = Double.parseDouble(num2);
                double res = (s.charAt(i))=='*' ? n1*n2: n1/n2;
                s = s.substring(0,i-num1.length())+res+s.substring(i+num2.length()+1);

                i = 0;
            }
        }
        for(int i=0; i<s.length();i++){
            if(s.charAt(i)=='+'||s.charAt(i)=='-'){
                num1 = before(s,i);
                num2 = after(s,i);
                n1 = Double.parseDouble(num1);
                n2 = Double.parseDouble(num2);
                double res = (s.charAt(i))=='+' ? n1+n2: n1-n2;
                s = s.substring(0,i-num1.length())+res+s.substring(i+num2.length()+1);
                System.out.println("+:" + s);
                i=0;
            }
        }
        return s;
    }
}

Unfortunately, your algorithm is not correct. It evaluates an expression 1-(1+1)+1 to -2.0, but it is clear that the right answer is zero. It is not an implementation bug. The logic itself is wrong. So what can be done to avoid(or, at least, to minimize the risk of) ending up with an incorrect algorithm?

You shouldn't make arbitrary assumptions. The algorithm you are using is not a well-known one, it is rather complex so it is not obvious whether it is correct or not, but you probably did not prove its correctness. For instance, you made an assumption that we can remove the first opening parenthesis and evaluate the part of the expression which goes after it separately. Is this assumption based on something? Did you try to figure out why it is correct? As you can see, it turned out to be incorrect. Proofs give you confidence in the logic of your solution.
You can use already existing algorithms. Arithmetic expressions evaluation is a well-studied topic. There is an efficient shunting-yard algorithm, which runs in linear time and is proven to be correct. Reinventing a non-trivial algorithm is not a good idea(unless you deliberately decide to do it as an exercise, but in this case you should be able to reason about the correctness of your solution).
Test your code in a systematic manner. Writing unit-tests is a good practice. The counterexample to your algorithm is rather short and simple, so you probably could have spotted that it is not correct if you had tested your code carefully.


Make external js file in ASP.NET MVC with razor path I have js code , that I decided to move to external js.

In this code I have razor syntax (url path)

Here is code of those functions

 function getStops() {
    var url = $('#map').data('request-url');
    $.getJSON(url,
        function (data) {
            var marker = [];

            $.each(data,
                function (i, item) {
                    marker.push({
                        'location': new google.maps.LatLng(item.Latitude2, item.Longitude2),
                        'map': map,
                        'weight': item.Difference,
                        'radius': 10
                    });
                });
            var pointArray = new google.maps.MVCArray(marker);

            heatmap = new google.maps.visualization.HeatmapLayer({
                data: pointArray
            });
            heatmap.setMap(map);
        });
};

// get Driving and show on layer
function getDriving() {
    var url = $('#map').data('request-url2');
    $.getJSON(url,
        function (data) {
            var marker = [];

            $.each(data,
                function (i, item) {
                    marker.push({
                        'location': new google.maps.LatLng(item.Latitude2, item.Longitude2),
                        'map': map,
                        'weight': item.Speed,
                        'radius': 10
                    });
                });
            var pointArray = new google.maps.MVCArray(marker);

            heatmap = new google.maps.visualization.HeatmapLayer({
                data: pointArray
            });
            heatmap.setMap(map);
        });

I know about data-request-url that I can write in element. in my case it's div.

So I wrote it like this on font-end

<div id="map" data-request-url="@Url.Action("GetStops", "Home")" data-request-url2="@Url.Action("Driving", "Home")">

Is this right, or I can do it in other way?

It's good to use data-* attributes for this kind of thing. I would highly recommend avoiding generic names like data-request-url in favor of something more explicit. You are generating two different heat maps:

Stops
Driving

This feels like two explicitly named attributes would be better:

<div id="map"
     data-heatmap-stops-url="@Url.Action(...)"
     data-heatmap-driving-url="@Url.Action(...)">
</div>

Now looking at your HTML source code will give you a better idea about what these custom attributes mean.

Cleaning up repetitive code

The code to get the map points between the stops and driving points is very similar. In fact, there are only two differences:

The URL
The property within the response used to render the heat map

Consider pulling this out into its own method, and parameterizing it:

function getHeatmapData(url, propertyName) {
    $.getJSON(url,
        function (data) {
            var marker = [];

            $.each(data,
                function (i, item) {
                    marker.push({
                        'location': new google.maps.LatLng(item.Latitude2, item.Longitude2),
                        'map': map,
                        'weight': item[propertyName],
                        'radius': 10
                    });
                });
            var pointArray = new google.maps.MVCArray(marker);

            heatmap = new google.maps.visualization.HeatmapLayer({
                data: pointArray
            });
            heatmap.setMap(map);
        });
}

Then the getStops and getDriving methods are much shorter:

function getStops() {
    var url = $('#map').data('heatmap-stops-url');

    getHeatmapData(url, "Stops");
}

function getDriving() {
    var url = $('#map').data('heatmap-driving');

    getHeatmapData(url, "Speed");
}

Just plain getting crazy

You could utilize a function closure and import only the symbols you need, throw in a 'use strict'; directive.

This will:

Give you bragging rights because you used 'use strict';. There are other benefits, but from the standpoint of your script it basically does nothing. Bragging right. That's it.
Remove the need for fully qualified class names like google.map.x.Foo.
Allow your code to function even if some other JavaScript library defines a $ function

(function ($, MVCArray, HeatmapLayer, LatLng) {
    'use strict';

    function getHeatmapData(url, propertyName) {
        $.getJSON(url,
            function (data) {
                var marker = [];

                $.each(data,
                    function (i, item) {
                        marker.push({
                            'location': new LatLng(item.Latitude2, item.Longitude2),
                            'map': map,
                            'weight': item[propertyName],
                            'radius': 10
                        });
                    });
                var pointArray = new MVCArray(marker);

                heatmap = new HeatmapLayer({
                    data: pointArray
                });
                heatmap.setMap(map);
            });
    }

    this.getStops = function () {
        var url = $('#map').data('heatmap-stops-url');

        getHeatmapData(url, "Stops");
    }

    this.getDriving = function () {
        var url = $('#map').data('heatmap-driving');

        getHeatmapData(url, "Speed");
    }

})(this.jQuery, this.google.maps.MVCArray, this.google.maps.visualization.HeatmapLayer, this.google.maps.LatLng);

But at this point, we're really just being crazy.


Rot and polyalphabetic ciphers in Python 2.7 I'm writing a "crypto library" and gradually adding algortihms. Now it's just ancient cryptography, but I'm going to expand the library.
It doesn't depend on any external libraries.

class Crypto():
    def __init__(self, rotInt=3, letters=list("abcdefghijklmnopqrstuvwxyz")):
        self.letters = letters
        self.length = len(self.letters)
        self.rotInt = rotInt
    def rot(self, string, rotInt, letters, length): # To decrypt, use a negative number
        result = []
        for letter in string:
            if letter == " ": # Skip spaces
                result.append(" ")
            else:
                letterIndex = letters.index(letter)
                resultIndex = letterIndex + rotInt
                resultIndex = resultIndex % length-1
                result.append(letters[resultIndex])
        return "".join(result) # Return result as a string
    def getRotArr(self, string, negative=False): # Negative is for decryption
        result = []
        for char in string:
            if negative:
                result.append(0-(self.letters.index(char)+1))
            else:
                result.append(self.letters.index(char)+1)
        return result
    def polyalphabetic(self, string, rotArr, letters, length):
        result = []
        rotArrIndex = 0
        for letter in string:
            if letter == " ": # Skip spaces
                result.append(" ")
            else: # Spaces would mess up rotArrIndex
                if rotArrIndex > len(rotArr)-1: # Repeat when end of rotArr is reached
                    rotArrIndex = 0
                rotInt = rotArr[rotArrIndex]
                letterIndex = letters.index(letter)
                resultIndex = letterIndex + rotInt
                resultIndex = resultIndex % length
                result.append(letters[resultIndex])
                rotArrIndex += 1
        return "".join(result) # Return result as a string

rotInt, letters, length are defined in class. Use it.

Instead of for loop use map.

def rot(self, string, letters=None, rotInt=None):
    if letters is None:
        letters = self.letters
    length = len(letters)
    if rotInt is None:
        rotInt = self.rotInt
    return ''.join(map(lambda letter: letter in letters and letters[(letters.index(letter) + rotInt) % length] or letter, string))

and or

It's like bool ? a : b in C / C++. It's equivalent to:

if bool:
    return a
else:
    return b


Function to validate parameter and throw exception when invalid I've asked a question receiving excessive (in a good sense) feedback. After I've incorporated it into the code, it changed substantially and I felt that I'd need opinion on the new version. Posting revised code is against the rules, so I'm asking a new, follow-up question.

Choice of exception names. Are those informative enough?.
Introduction of tiered exceptions. Are more levels of inner exceptions needed?).
Omission of the values. Should I provide the faulty ones?

Code

private static void Validate(Guid userId, EntityState state, object data)
{
  if (userId == Guid.Empty)
    throw new IdentityNotMappedException("Token unrecognized!",
      new ArgumentException("Can't be Guid.Empty or null!", nameof(userId)));
  if (state == EntityState.Detached)
    throw new InvalidOperationException("Action unrecognized!",
      new ArgumentException("Must follow C_UD!", nameof(state)));
  if (data == null)
    throw new MappingException("Data unrecognized!",
      new ArgumentException("Must be deserializeable!", nameof(data)));
}

I'm generally strongly against unthrown exceptions. An exception without its stack trace is simply a named container for an error message. You're using this kind of exceptions for innerException parameter of the exceptions your really throw:

throw new IdentityNotMappedException("", new ArgumentException("", nameof(userId)));

There is no need to have an inner exception if outer one has not been originated by that. When you feel that needing you are, probably, using wrong exception.

There is no need for ArgumentException and userId cannot be null because it is a value type. IdentityNotMappedException alone itself is sufficient to "describe" your error condition (but note that, according to your true code, I'd use a more descriptive error message).

if (userId == Guid.Empty)
    throw new IdentityNotMappedException("User identity is unknown.");

"Action unrecognized" is not a very useful error message (same as "Must follow C_UD!"). Your error messages must quickly describe what's wrong (hints to solve the problem are welcome but optional).

if (state == EntityState.Detached)
    throw new ArgumentException("Entity can not be in Detached state.", nameof(state));

If state is a function argument then you should throw ArgumentException, if it's object state then you may throw InvalidOperationException:

if (state == EntityState.Detached)
    throw new InvalidOperationException("Entity can not be in Detached state.");

Let me stress again to use a better error description. You're checking for a parameter vs null; there may be multiple reasons for this (caller programming error, invalid object state), unless you're sure about what's going on then limit yourself to say what you actually know (argument is null).

if (data == null)
    throw new ArgumentNullException(nameof(data));

To answer your questions directly:

  Choice of exception names. Are those informative enough?.

Last exception is not right one (IMO) because you can be more specific. First one can be right if (and only if) you're actually working with System.Security.Principal stuff. If not then you'd better to do not use it because it carries extra context that it's not applicable in your case.

  Introduction of tiered exceptions. Are more levels of inner exceptions needed?).

No, you do not even need these inner exceptions, for sure you don't need more. As I already said an exception without stack trace is merely a named container for an error message and moreover you give a false information: IdentityNotMappedException has been originated because of a catched ArgumentException. If you want to give more information don't forget that each exception has a Data dictionary for this purpose:

if (state == EntityState.Detached) {
    var e = new ArgumentException("Cannot use detached entity state.", nameof(state));
    e.Data.Add("Hint", "Must follow C_UD");

    throw e;
}

  Omission of the values. Should I provide the faulty ones?

When value is implicit you don't need to provide it (for example if you're throwing because state is EntityState.Detached then you don't need to repeat, same when you throw ArgumentNullException). You have this doubt just because your error messages are not actually describing what is wrong.

<edit>
As you already noted by yourself these error messages may be as much cryptic as original ones. Context will help to clarify them when required, if - for example - entity state is an unknown implementation detail (from caller point of view) then it has no use to expose it and a more abstract error message is preferable.
</edit>

In your example there are no need to specify any faulty value but, for example, in this situation you should do it:

if (entity.State != EntityState.Unchanged)
    throw ArgumentException($"Invalid entity state {entity.State} (state must be Unchanged).", nameof(entity));


Check if string is or can be made into a "valid string" The Challenge:

  A valid string is one in which all distinct characters occur the same
  number of times. Given a string S, check if the string can be made
  valid by removing less than or equal to one character. If yes, output
  "YES". Otherwise output "NO".

  Constraints:

  \$1 \le \lvert S \rvert \le 10^5\$
  String S contains lowercase letters only (a-z)

I'd appreciate any comments and suggestions as to how my code can be improved.

function processString(str) {
    if(str.length === 1){
        console.log("YES");
    }
    else{
        const letterCount = buildCount(str.trim());
        console.log(checkValid(letterCount) ? "YES" : "NO");
    }
}

function buildCount(str){
    const letterCount = {};
    const len = str.length;
    for (let i = 0; i < len; i++) {
        letterCount[str[i]] = (letterCount[str[i]] || 0) + 1;
    }
    return letterCount;
}

function checkValid(letterCount){
    const totals = [];
    for(let key of Object.keys(letterCount)){
        totals.push(letterCount[key]);
    }
    //only one can be different- and it has to be more than the others
    //if always compare to the first,
    //  doesn't work if that is the one that is more
    const compareVal = Math.min(totals[0], totals[1]);
    const diffOnes = totals.filter(function(val){
        return val !== compareVal;
    });
    if(diffOnes.length > 1
        || diffOnes[0] > (compareVal + 1)){
        return false;
    }
    return true;
}

Your logic is fine. I think that a function of this sort does not necessarily need to be broken down into different functions. This function would still serve a single responsibility and without any wider need of such really specific functions in a wider application context, I see no reason to break it apart like this.

I think what you have can be simplified to something like this:

function isValidString(str) {
    var strArray = str.split('');
    var counter = strArray.reduce(
        (cnt, letter) => { cnt[letter] = (cnt[letter] || 0) + 1; return cnt; }
        ,{}
    );
    var i = 0;
    var compareVal;
    var diffFound = false;
    for (letter in counter) {
        i++;
        if (i === 1) {
            compareVal = counter[letter];
            continue;
        }
        if (counter[letter] === compareVal)
            continue;
        if (diffFound)
            return 'NO';
        diffFound = true;
        if (Math.abs(counter[letter] - compareVal) > 1)
            return 'NO';
        if (counter[letter] < compareVal) {
            if (i === 2) {
                compareVal = counter[letter];
            } else {
                return 'NO';
            }
        }
    }
    return 'YES';
}

Note here we use reduce function to build the letter counter object, eliminating need for separate function. We also slightly optimize the logic to iterate and evaluate the counter, allowing for early exit from that iteration as soon as an invalid condition is encountered.

I probably would not include console logging within the function itself, as I think the function should do only one thing, which is determine if string is valid, not output it as well. Obviously, it is trivial to log the output of this function like

console.log(isValidString(testString));

Be careful in ensuring your function and variables names are meaningful. Your functions are vaguely named and do not specifically convey what caller should expect to happen. 'letterCount' as a variable name would lead me to expect an integer value to be contained therein, not an object with letter counts in it. A simple change to 'letterCounter' would make tremendous difference in conveying meaning of this variable.


Filter array elements using variable number of filters This is a question generated from a coding challenge, and I solved it, but I'm just not sure it's a great solution, nor am I sure why the first way I was trying didn't work.

Challenge: Implement a function called destroyer().  "You will be provided with an initial array (the first argument in the destroyer function), followed by one or more arguments. Remove all elements from the initial array that are of the same value as these arguments."

Some test cases:
- destroyer([1, 2, 3, 1, 2, 3], 2, 3) should return [1, 1]
- destroyer(["tree", "hamburger", 53], "tree", 53) should return ["hamburger"]

So here is the function that passed the test.

  function destroyer(arr) {
    // make a temp array to store iterations of the loop used to filter
    var newArr = [];

    // get the items to filter from the arguments passed.
    // EDIT: changed this for loop to use slice
    var filterItems = Array.from(arguments).slice(1);

    // use the array of items to filter, to filter the 'arr'
    // NOTE:I get a warning about calling a function in a loop. but it works in my lab environment anyhow.
    for (i=0; i < filterItems.length; i++){
      newArr = arr.filter(function(el){ // cannot assign directly to arr here for some reason???
       return el !== filterItems[i] }
     );
    arr = newArr;
    }

    return newArr;
  }

Okay, so calling the function with this, the array passed in argument 1 gets filtered, removing any numbers passed later.

destroyer([1, 2, 3, 1, 2, 3], 2, 3);

What didn't work, is using arr = arr.filter... to iterate over the array passed into the function.  Instead, I first had to output the filter into the temp newArray and then reassign it in the for loop.  That kind of confuses me. if I can change arr with arr = newArr.  Why does that work and not the other way where I output the arr.filter right back into arr?

What didn't work, is using arr = arr.filter... to iterate over the array passed into the function

It works. But you would then need to return arr instead of newArr. Also, it is less readable as you re-assign the function's return value to the input parameter, which is confusing.

Further suggested improvements:

Naming:

Rename newArr to e.g. result as this makes its role more clear.

Style:

Array.from(arguments).slice(1) creates an unnecessary internal copy. Use e.g. Array.prototype.slice.call(arguments, 1). Some people prefer [].slice.call(arguments, 1)
Don't make an unnecessary local copy of the arguments if performance
is key.
Since you already make a local copy, replace the for-loop with the functional style Array.includes for consistency's sake, since you already use Array.filter.
Having a function call within a loop body is slow as the function call overhead is not marginal in JavaScript.
Declare the loop iterator i as a local variable for (var i = ...

Applying those suggestions to your code gives us:

function destroyer(array) {
  var filter = Array.prototype.slice.call(arguments, 1);
  return array.filter(function(el) {
    return !filter.includes(el);
  });
}

console.log(destroyer([1, 2, 3, 1, 2, 3], 2, 3)); // [1, 1]
console.log(destroyer(["tree", "hamburger", 53], "tree", 53)) // ["hamburger"]

However, this function is inefficient for larger inputs as it iterates the filter array for each array element. It's time complexity is in O(mn) where m and n are the lengths of array and filter. You can improve on that by using a Set and come up with a linear time solution. Using rest parameters allows us to get rid of the argument slicing:

function destroyer(array, ...filter) {
  filter = new Set(filter);
  return array.filter(element => !filter.has(element));
}

console.log(destroyer([1, 2, 3, 1, 2, 3], 2, 3)); // [1, 1]
console.log(destroyer(["tree", "hamburger", 53], "tree", 53)) // ["hamburger"]


Efficiently sample a single row of an array at random I have the following C++ code snippet that randomly samples a single row in an array called pop with num_specs columns and perms rows. In addition, K = 1. The triply-nested for loop uses a pointer for referencing.

Some of the below syntax (such as IntegerVector) is from Rcpp, an R package to integrate C++ code with R code.

// [[Rcpp::depends(RcppArmadillo)]]
// [[Rcpp::depends(RcppProgress)]]
#define ARMA_DONT_PRINT_OPENMP_WARNING
#include <RcppArmadillo.h>
#include <RcppArmadilloExtensions/sample.h>
#include <set>
using namespace Rcpp;

int sample_one(int n) {
  return n * unif_rand();
}

int sample_n_distinct(const IntegerVector& x,
                      int k,
                      const int * pop_ptr) {

IntegerVector ind_index = RcppArmadillo::sample(x, k, false);
std::set<int> distinct_container;

for (int i = 0; i < k; i++) {
    distinct_container.insert(pop_ptr[ind_index[i]]);
}

return distinct_container.size();
}

// [[Rcpp::export]]
arma::Cube<int> accumulate(const arma::Cube<int>& pop,
                     const IntegerVector& specs,
                     int perms,
                     int K) {

int num_specs = specs.size();
arma::Cube<int> res(perms, num_specs, K);

IntegerVector specs_C = specs - 1;
const int * pop_ptr;
int i, j, k;

for (i = 0; i < K; i++) {
    for (k = 0; k < num_specs; k++) {
        for (j = 0; j < perms; j++) {
            pop_ptr = &(pop(sample_one(perms), 0, sample_one(K)));
            res(j, k, i) = sample_n_distinct(specs_C, k + 1, pop_ptr);
        }
    }
}
return res;
}

While loops in compiled languages aren't bad, it is possible to write slow code.

I'm not a native C++ programmer, so I don't know all the tricks of the trade.

Is there a way to reduce the number of levels in the triply-nested for loop above, possibly by employing modular arithmetic in order to see a gain in speed for large input values?

The R code is below:

## Set up container to hold the identity of each individual from each permutation ##
num.specs <- N

## Create an ID for each tag ##
tags <- 1:h

## Assign individuals (N) ##
specs <- 1:num.specs

## Generate permutations. Assume each permutation has N individuals, and sample those
# individuals' tags from the probabilities ##
  gen.perms <- function() {
      sample(tags, size = num.specs, replace = TRUE, prob = probs)
  }

  pop <- array(dim = c(perms, num.specs, K))

  for (i in 1:K) {
    pop[,, i] <- replicate(perms, gen.perms())
  }

## Perform accumulation ##
HAC.mat <- accumulate(pop, specs, perms, K)

## Example
K <- 1
N <- 100
h <- 5
probs <- rep(1/h, h)
perms <- 100

I can only comment on the C++ code, as I've never used R nor Rcpp.

For an ignorant reader such as myself, it would have helped to omit the using namespace Rcpp;, so that I could see which names come from that library (I did quite a bit of external reading to even understand the code).

Assuming I'm right that sample() returns an array of length k, then we can use range-based for to iterate through its values (since IntegerVector has suitable begin() and end()):

// sample k values from x, without replacement
const Rcpp::IntegerVector ind_index = Rcpp::RcppArmadillo::sample(x, k, false);

// how many different values do they index in pop_ptr?
std::set<int> distinct_container;
for (int i: ind_index) {
    distinct_container.insert(pop_ptr[i]);
}

In accumulate() (which name I dislike, because it sounds like std::accumulate()), we can reduce the scope of i, j, k and pop_ptr quite simply:

for (int i = 0; i < K; ++i) {
    for (int k = 0;  k < num_specs;  ++k) {
        for (int j = 0;  j < perms;  ++j) {
            const int *const pop_ptr = &(pop(sample_one(perms), 0, sample_one(K)));
            res(j, k, i) = sample_n_distinct(specs_C, k + 1, pop_ptr);
        }
    }
}

Also, I'd be inclined to show that the creation of specs_C is more than just a conversion from specs - 1 - that's actually a constructor argument specifying the length of the new vector:

IntegerVector specs_C(specs - 1);

I don't see any code that ever modifies this vector; if it's supposed to remain full of zeros, then it can be declared const.

With those changes, and an automated re-indent, we get something like this for those two functions (I've compiled, but not tested this):

int sample_one(int n)
{
    return n * R::unif_rand();
}

int sample_n_distinct(const Rcpp::IntegerVector& x,
                      int k,
                      const int * pop_ptr)
{
    // sample k values from x, without replacement
    const auto ind_index = Rcpp::RcppArmadillo::sample(x, k, false);

    // how many different values do they index in pop_ptr?
    std::set<int> distinct_container;
    for (int i: ind_index) {
        distinct_container.insert(pop_ptr[i]);
    }

    return distinct_container.size();
}

// [[Rcpp::export]]
arma::Cube<int> accumulate(const arma::Cube<int>& pop,
                           const Rcpp::IntegerVector& specs,
                           int perms,
                           int K)
{
    auto const num_specs = specs.size();
    arma::Cube<int> res(perms, num_specs, K);

    Rcpp::IntegerVector specs_C(specs - 1);

    for (int i = 0;  i < K;  ++i) {
        for (int k = 0;  k < num_specs; ++k) {
            for (int j = 0;  j < perms; ++j) {
                const auto& sampled = pop(sample_one(perms), 0, sample_one(K));
                res(j, k, i) = sample_n_distinct(specs_C, k + 1, &sampled);
            }
        }
    }
    return res;
}

Sadly, I don't have the domain knowledge to suggest more meaningful improvements other than these fairly mechanical changes that improve readability.

In particular, I don't understand why we have to pass specs_C to accumulate() instead of sampling directly from pop_ptr.

I'm also very wary of taking the address of the element returned by pop() - that seems to assume a memory layout that could vary between platforms, for instance.  I'd expect to use one of the subcube view operations to construct the population:

int sample_n_distinct(const Rcpp::IntegerVector& population, int k)
{
    // sample k values from population, without replacement
    const auto sampled = Rcpp::RcppArmadillo::sample(population, k, false);

    // how many different values do have?
    std::set<int> distinct(sampled.begin(), sampled.end());

    return distinct.size();
}

// [[Rcpp::export]]
arma::Cube<int> accumulate(const arma::Cube<int>& pop,
                           const Rcpp::IntegerVector& specs,
                           int perms,
                           int K)
{
    auto const num_specs = specs.size();
    arma::Cube<int> res(perms, num_specs, K);

    for (int i = 0;  i < K;  ++i) {
        for (int k = 0;  k < num_specs; ++k) {
            for (int j = 0;  j < perms; ++j) {
                const auto& sampled = pop.tube(sample_one(perms), sample_one(K));
                const auto sampled_vector
                    = Rcpp::IntegerVector(sampled.begin(), sampled.end());
                res(j, k, i) = sample_n_distinct(sampled_vector, k + 1);
            }
        }
    }
    return res;
}

I may be wrong on exactly what subcube view is required, so pop.tube() may need to be a different call, but you get the idea.  See how much simpler life is when you can use iterators instead of grubbing around with pointers?


Mingw, wcout and locales I've recently had to print some national symbols in windows console using Mingw and found, that I got nothing in the output, if I use wide strings.

So, I studied the problem and found out that it is locale problem. I was unable to find somewhere complete solution, so I had to write something, that seems to work.

So, I decided to publish it here for review, or may be I should publish it somewhere it it worth.

I've tested in Mingw 4.7.0 using Russian locale.
It should probably correctly convert wide strings to national OEM encoded strings.

Code:

#ifndef CUSTOM_LOCALE_H
#define CUSTOM_LOCALE_H

#if (__cplusplus >= 201103L)
#define CUSTOM_LOCALE_OVERRIDE override
#else
#define CUSTOM_LOCALE_OVERRIDE
#endif

#include <locale>
#include <iostream>
#include <windows.h>

namespace custom_locale
{

class CustomLocale : public std::codecvt <wchar_t, char, std::mbstate_t> {
  public:
    explicit CustomLocale ( size_t r = 0 ) : std::codecvt <wchar_t, char, std::mbstate_t> (r) {}

  protected:
    result do_in (state_type&, const char* from, const char* from_end,
                  const char*& from_next, wchar_t* to, wchar_t* to_end, wchar_t*& to_next ) const CUSTOM_LOCALE_OVERRIDE
    {
        std::size_t size = from_end - from;
        std::size_t buffer_size = to_end - to;
        std::size_t written = MultiByteToWideChar(CP_OEMCP, 0, from, size, to, buffer_size);
        to_next = to + written;
        if (written == 0) {
            return error;
        }
        else if (written != buffer_size) {
            return partial;
        }
        else {
            return  ok;
        }
    }

    result do_out (state_type&, const wchar_t* from, const wchar_t* from_end,
                   const wchar_t*& from_next, char* to, char* to_end, char*& to_next ) const CUSTOM_LOCALE_OVERRIDE
    {
        std::size_t size = from_end - from;
        std::size_t buffer_size = to_end - to;
        std::size_t written = WideCharToMultiByte(CP_OEMCP, 0, from, size, to, buffer_size, 0, 0);
        to_next = to + written;
        if (written == 0) {
            return error;
        }
        else if (written != buffer_size) {
            return partial;
        }
        else {
            return  ok;
        }
    }

    result do_unshift ( state_type&, char*, char*, char*& ) const CUSTOM_LOCALE_OVERRIDE { return ok; }
    int do_encoding () const throw () CUSTOM_LOCALE_OVERRIDE { return 1; }
    bool do_always_noconv () const throw () CUSTOM_LOCALE_OVERRIDE { return false; }

    int do_length ( state_type& state, const char* from, const char* from_end, size_t max ) const CUSTOM_LOCALE_OVERRIDE
    {
      return std::codecvt <wchar_t, char, std::mbstate_t>::do_length ( state, from, from_end, max );
    }

    int do_max_length () const throw () CUSTOM_LOCALE_OVERRIDE
    {
      return std::codecvt <wchar_t, char, std::mbstate_t>::do_max_length ();
    }
};

inline void init()
{
    std::locale loc ( std::locale(), new CustomLocale() );
    std::ios_base::sync_with_stdio (false);
    std::wcout.imbue(loc);
    std::wcin.imbue(loc);
}

}

#endif

Example:

#include "custom_locale.h"

int main ()
{
    custom_locale::init();
    std::wstring s(L"привет");
    std::wcout << s << '\n';
    std::wstring u;
    std::wcin >> u;
    s += L' ' + u;
    std::wcout << s << '\n';
    return 0;
}

This is a somewhat old question, but I have an alternative that may work for this.  Related to this question, if you turn off the synchronization with the underlying stdio, you can use imbue to set the locale for both wcout and wcin:

#include <locale>
#include <string>
#include <iostream>

int main ()
{
    // this code turns off the sync with stdio
    std::ios_base::sync_with_stdio(false);
    // now create the appropriate locale without setting the global one
    std::locale ru("ru_RU.utf8");
    // use imbue to allow the Russian words to be properly displayed
    std::wcout.imbue(ru);
    // ... and input
    std::wcin.imbue(ru);

    // the rest of the code is essentially unchanged from the original
    std::wstring s(L"привет");
    std::wcout << s << '\n';
    std::wstring u;
    std::wcin >> u;
    s += L' ' + u;
    std::wcout << s << '\n';
 }

Using it in this way means that no custom class is required.


Rendering and re-rendering a Mustache template I want to ask whether my following implementation is good in the view of good practice and performance. I have an app where I have to show another page or template html in an iframe. I get that template html via ajax and render it in the iframe. Iframe must be used because the template must be isolated.I am now storing that template html in a javscript variable

var tmp;
$.get('/template/1', function(data){  tmp = $(data); });
// tmp = '<html><body>....etc...</body></html'; holds whole template

and then I have my div container in that template where I use mustache for changing content.

var render = tmp.find('#container').html();
var rendered = Mustache.to_html(render, some_data);
//then
tmp.find('#container').html(rendered);
// and add tmp to iframe

I am storing whole html in tmp so that it can be used later also and when my some_data change I again process tmp and re-render template in iframe. I may need re-render template many times later so I am storing in in variable. Is it good to store whole template html in javascript variable like that or it's better use other ways...if there are other ways what are they...

I don't think this will cause you any problems; it may offer superior performance in many scenarios, especially over re-rendering the template. The only other option that comes to mind is to keep this invisibly in the DOM somewhere and use JQuery's clone() to re-render it as needed. I use this latter technique especially if the original template is pre-rendered from another source. Just bear in mind that if you do use clone() JQuery also clones element IDs if you have any, which may cause your selectors on those IDs to behave unexpectedly if you don't change them after the clone (or refactor the source template).


Using Span and Memory to read UTF8 from a socket I am experimenting with Span<> and Memory<> and checking how to use it for example for reading UTF8 text from a NetworkStream using Read(Span<Byte>). I created a fake method that simulates the byte read, and although this code works for the test, I am wondering if this is the right way of doing the most with these new structures in terms of efficiency and memory.

static void Main(string[] args)
{
    Memory<byte> byteMemory = new byte[3];
    Memory<char> charMemory = new char[1024];

    var byteMemoryPos = 0;
    var byteMemoryReaded = 0;
    var charMemoryPos = 0;
    var readed = -1;

    var decoder = Encoding.UTF8.GetDecoder();

    do
    {
        // pass the sliced part of the memory where I want to write as span
        readed = FakeNetworkStreamRead(byteMemory.Slice(byteMemoryPos + byteMemoryReaded).Span);
        Console.WriteLine($"Readed {readed} bytes");

        byteMemoryReaded += readed;

        // pass the sliced part of the memory I want to parse, and the part
        // of the char buffer where I want to write
        decoder.Convert(byteMemory.Slice(byteMemoryPos, byteMemoryReaded).Span,
                        charMemory.Slice(charMemoryPos).Span,
                        false, out int bytesUsed, out int charsUsed, out bool completed);

        // update offsets and counts
        byteMemoryPos -= bytesUsed;
        charMemoryPos += charsUsed;
        byteMemoryReaded -= bytesUsed;

        // reset offset if nothing to read
        if (byteMemoryReaded == 0)
            byteMemoryPos = 0;
    }
    while (readed != 0);

    Console.WriteLine(new string(charMemory.Slice(0, charMemoryPos).Span));
}

static int _position = 0;
static byte[] _data = Encoding.UTF8.GetBytes("Hi this is test!!");

/// <summary>
/// Pretends to be NetworkStream.Read, that only manages to read 5 bytes each time
/// </summary>
/// <param name="span"></param>
/// <returns></returns>
static int FakeNetworkStreamRead(Span<byte> span)
{
    var pending = Math.Min(_data.Length - _position, span.Length);
    pending = Math.Min(pending, 5);
    for (int i = _position, j = 0; i < _position + pending; j++, i++)
    {
        span[j] = _data[i];
    }
    _position += pending;
    return pending;
}

I have no experience with Span<T> or Memory<T> so the following is just some general considerations on coding etc.

In FakeNetworkStreamRead(...): why have a internal limit of 5 bytes? Why not let the length of the Span<T> argument determine the maximum bytes to read?

Why are you using a Memory<T> as buffer, when reading from the stream?:

Memory<byte> byteMemory = new byte[3];

Why not just a Span, because you only use it as a such and not holding or collecting any data in it:

Span<byte> byteSpan = new byte[3];

IMO you should change the do {} while(...) loop to a while (...) loop, because, if the stream is empty you do a "lot" of work in the first round trip for no reason:

  int read = 0;

  while ((read = FakeNetworkStreamRead(byteSpan)) > 0)
  {
    ...
  }

With the above in mind I've tried to refactor your code to:

static int FakeNetworkStreamRead(Span<byte> span)
{
  int stop = Math.Min(_position + span.Length, _data.Length);
  int i = 0;

  for (; _position < stop; i++, _position++)
  {
    span[i] = _data[_position];
  }

  return i;
}

static void Main(string[] args)
{
  Span<byte> byteSpan = new byte[3];
  Memory<char> charMemory = new char[1024];

  var charMemoryPos = 0;
  var decoder = Encoding.UTF8.GetDecoder();
  int read = 0;

  while ((read = FakeNetworkStreamRead(byteSpan)) > 0)
  {
    Console.WriteLine($"Read {read} bytes");

    // pass the sliced part of the memory I want to parse, and the part
    // of the char buffer where I want to write
    decoder.Convert(byteSpan.Slice(0, read),
                    charMemory.Slice(charMemoryPos).Span,
                    false, out int bytesUsed, out int charsUsed, out bool completed);

    // update offsets and counts
    charMemoryPos += charsUsed;
  }

  Console.WriteLine(new string(charMemory.Slice(0, charMemoryPos).Span));
}

In order to flush the decoder state, the do {} while(...) loop is useful anyway:

static void Main(string[] args)
{
  int bufferLength = 3;
  Span<byte> byteSpan = new byte[bufferLength];
  Memory<char> charMemory = new char[1024];

  var charMemoryPos = 0;
  var decoder = Encoding.UTF8.GetDecoder();
  int read = 0;

  do
  {
    read = FakeNetworkStreamRead(byteSpan);

    decoder.Convert(byteSpan.Slice(0, read),
                    charMemory.Slice(charMemoryPos).Span,
                    read == 0, out int bytesUsed, out int charsUsed, out bool completed);

    Console.WriteLine($"Read {read} bytes -> Used: {bytesUsed} -> Completed: {completed} -> CharUsed: {charsUsed} -> {decoder.FallbackBuffer.Remaining}");

    charMemoryPos += charsUsed;
  } while (read > 0) ;

  Console.WriteLine(new string(charMemory.Slice(0, charMemoryPos).Span);
}

Here the read == 0 argument to decoder.Convert(...) becomes true at the end of the stream, which will flush the state and clears the internal buffer of decoder


Multiplying a Matrix with its Transpose Currently, I am in the process of optimizing a MIPS assembly program that takes a n x n matrix and multiplies it with its transpose. I am trying to optimize my matrix calculation algorithm so that it completes in as few clock cycles as possible. I have been given the A matrix, with its values stored in RAM. I must then calculate B=A*transpose(A).

There are a few caveats:

The matrix multiplication must be the dot product of the ith row of A and the jth column of B. It is not meant to be the element-wise multiplication. See the Wikipedia article.
I am not to make my algorithm more mathematically efficient then the un-modified example I will show below. I.e. I cannot exploit the symmetric nature resulting when you multiply a matrix with its transpose.

Here is the pseudo code example I have been given:

// Given array A which is unsigned int A[n*n] (ie word or 32 bit form)
// Reset array B which is unsigned int B[n*n] (ie word or 32 bit form)
for(int i = 0; i < (n * n); i++)
{
    B[i] = 0;
}

// Matrix Multiplicaiton B = A*A'
for (int i = 0; i < n; i++)
{
    for (int j = 0; j < n; j++)
    {
        for (int k = 0; k < n; k++)
        {
            B[i + n * j] = B[i + n * j] + A[i + n * k] * A[j + n * k];
        }
    }
}

Here is my attempt at optimizing the above example:

// Given array A which is unsigned int A[n*n] (ie word or 32-bit form)

// Matrix Multiplicaiton B = A*A'
for(int i = 0; i < n; i++)
{
    for (int j = 0; j < n; j++)
    {
        temp = 0;
        n_times_i = n * i;

        for (int k = 0; k < (n*n); k+=n)
        {
            temp += A[j + k] * A[i + k];
        }

        B[j + n_times_i] = temp;
    }
}

As you can see, I have swapped things around to avoid unnecessary calculations where possible.

However, I was wondering whether anyone can see any other way of speeding things up? I.e. Cleverly swapping the order of the loops, etc.

Any help would be greatly appreciated!

Barring any compiler heroics, you are computing n*n a total of \$n^3\$ times.  You might want to cache that result.

const int nn = n*n;

B[j + n_times_i] is a linearly increasing address location, given that j increases by 1 for each middle loop, and and i increases once for each outer loop, which is n increases of j.  Taking advantage of that, you can skip the j + n*i calculation, and B[ ] indexing.

int *pB  = &B;
// ... loops & calculation of temp omitted for brevity.
      *pB++ = temp;

Result:

const int nn = n*n;
int *pB = B;

for (int i = 0; i < n; i++) {
    for (int j = 0; j < n; j++) {
        temp = 0;
        for (int k = 0; k < nn; k += n) {
            temp += A[i+k] * A[j+k];
        }
        *pB++ = temp;
    }
}

You may find that you can get additional speed by using pointer arithmetic for A[i+k] and A[j+k].

int *pAi = A + i;
int *pAj = A + j;
for (k=0; k<n; k++) {   // Note: n.  The nn variable is no longer needed.
    temp += *pAi * *pAj;
    pAi += n;
    pAj += n;
}

But, you’ll need to profile to find out.  It depends on the number of free registers you have ... and the compiler/optimizers these days are pretty darn good.


WPF: Model to View Model data binding to implement Two Step View I am trying to use Two Step View pattern in WPF. The problem is about keeping things in sync – I need a way to data-bind models to view models. Are there any good existing solutions? My approach is to introduce Property<T> to be used this way:

public abstract class TextField : ObservableObject
{
    public Property<bool> IsEnabled { get; set; } = true;
    public Property<string> Text { get; set; } = false;
    // etc.
}

Where property supports databinding updates:

public class Property<T> : ObservableObject
{
    public static implicit operator Property<T>(T value) => new Property<T>(() => value);
    public static implicit operator T(Property<T> property) => property.Value;

    public Property(Func<T> getter, params INotifyPropertyChanged[] dataSources)
        : this(getter, v => { throw new NotSupportedException(); }, dataSources)
    {
    }

    public Property(Func<T> getter, Action<T> setter, params INotifyPropertyChanged[] dataSources)
    {
        Getter = getter;
        Setter = setter;
        foreach (var dataSource in dataSources)
            dataSource.PropertyChanged += (s, e) => RaisePropertyChanged("Value");
    }

    public T Value
    {
        get { return Getter(); }
        set { Setter(value); }
    }

    Func<T> Getter { get; }
    Action<T> Setter { get; }
}

I use helper method Bind in the following way for view model construction:

        var model = new DocumentModel();
        var viewModel = new TextField
        {
           Text = model.Bind(m => m.FullName),
           IsEnabled = model.Bind(m => m.FullName != "")
        };

Where bind defined as:

public static class Binding
{
    public static Property<TTarget> Bind<TSource, TTarget>(this TSource source, Expression<Func<TSource, TTarget>> mapper)
        where TSource : INotifyPropertyChanged
    {
        Mapping<TSource, TTarget> mapping = mapper;
        return new Property<TTarget>(() => mapping[source], v => mapping[source] = v, source);
    }
}

Where Mapping compiles getter and setter from the expression:

class Mapping<TSource, TTarget>
{
    public static implicit operator Mapping<TSource, TTarget>(Expression<Func<TSource, TTarget>> mapping) =>
        new Mapping<TSource, TTarget>(mapping);

    [DebuggerHidden]
    public Mapping(Expression<Func<TSource, TTarget>> mapper)
    {
        Field = mapper.Parameters[0].Name;
        Getter = mapper.Compile().Invoke;

        try
        {
            var newValue = Expression.Parameter(mapper.Body.Type);
            var assign = Expression.Lambda<Action<TSource, TTarget>>(
                Expression.Assign(mapper.Body, newValue),
                mapper.Parameters[0], newValue);

            Setter = assign.Compile().Invoke;
        }
        catch(ArgumentException)
        {
        }
    }

    public string Field { get; }

    public TTarget this[TSource source]
    {
        get { return Getter(source); }
        set { Setter(source, value); }
    }

    Func<TSource, TTarget> Getter { get; }
    Action<TSource, TTarget> Setter { get; }
}

It works somehow:

 <DataTemplate DataType="{x:Type f:TextField}">
    <TextBox
        Text="{Binding Path=Text.Value, UpdateSourceTrigger=PropertyChanged}"
        IsEnabled="{Binding Path=IsEnabled.Value}"/>
 </DataTemplate>

Does it look ugly? Two Step View helps me a lot…

Here is how I redefined it now. Demo:

public MainWindow()
{
    InitializeComponent();

    var dvm = new DemoViewModel();
    var dm = new DemoModel();
    dvm.Text = dm.Select(m => m.Text); // bidirectional
    dvm.Text2 = dm.Select(m => m.Text); // bidirectional
    dvm.IsReadOnly = dm.Select(m => m.Text.Length > 4); // calculable
    DataContext = dvm;
}

Where:

class DemoModel : Model
{
    string _text = "";

    public string Text
    {
        get { return _text; }
        set { Set(() => Text, ref _text, value); }
    }
}

And (too verbose, but it is a stupid WPF thing):

class DemoViewModel : ViewModel
{
    public static readonly DependencyProperty TextProperty =
        DependencyProperty.Register("Text", typeof(string), typeof(DemoViewModel));

    public static readonly DependencyProperty Text2Property =
        DependencyProperty.Register("Text2", typeof(string), typeof(DemoViewModel));

    public static readonly DependencyProperty IsReadOnlyProperty =
        DependencyProperty.Register("IsReadOnly", typeof(bool), typeof(DemoViewModel));

    public Binding<string> Text
    {
        get { return (string)GetValue(TextProperty); }
        set { SetBinding(TextProperty, value); }
    }

    public Binding<string> Text2
    {
        get { return (string)GetValue(Text2Property); }
        set { SetBinding(Text2Property, value); }
    }

    public Binding<bool> IsReadOnly
    {
        get { return (bool)GetValue(IsReadOnlyProperty); }
        set { SetBinding(IsReadOnlyProperty, value); }
    }
}

Library classes:

class Model : ObservableObject
{
}

And:

Outdated - see below:

class ViewModel : FrameworkElement
{
    protected BindingExpressionBase SetBinding<T>
        (DependencyProperty dp, Binding<T> property)
    {
        var binding = new Binding("Value")
            { Source = property, Mode = BindingMode.TwoWay };

        return SetBinding(dp, binding);
    }
}

Now

public class ViewModel : DependencyObject
{
    protected BindingExpressionBase SetBinding<T>(DependencyProperty dp, Binding<T> property)
    {
        var binding = new Binding("Value")
        {
            Source = property,
            Mode = BindingMode.TwoWay
        };

        return BindingOperations.SetBinding(this, dp, binding);
    }
}

And:

public static class ModelBinding
{
    public static Binding<T> Select<TM, T>(this TM m, Expression<Func<TM, T>> mapper)
    {
        Mapping<TM, T> mapping = mapper;
        return new Binding<T>(() => mapping[m], v => mapping[m] = v, m);
    }
}

And:

public class Binding<T> : ObservableObject
{
    public static implicit operator Binding<T>(T value) => new Binding<T>(() => value);
    public static implicit operator T(Binding<T> property) => property.Value;

    public Binding(Func<T> getter, params object[] dataSources)
        : this(getter, v => { throw new NotSupportedException(); }, dataSources)
    {
    }

    public Binding(Func<T> getter, Action<T> setter, params object[] dataSources)
    {
        Getter = getter;
        Setter = setter;
        foreach (var npc in dataSources.OfType<INotifyPropertyChanged>())
            npc.PropertyChanged += (s, e) => RaisePropertyChanged("Value");
    }

    public T Value
    {
        get { return Getter(); }
        set { Setter(value); }
    }

    Func<T> Getter { get; }
    Action<T> Setter { get; }
}

And:

class Mapping<TSource, TTarget>
{
    public static implicit operator Mapping<TSource, TTarget>(Expression<Func<TSource, TTarget>> mapping) =>
        new Mapping<TSource, TTarget>(mapping);

    [DebuggerHidden]
    public Mapping(Expression<Func<TSource, TTarget>> mapper)
    {
        Getter = mapper.Compile().Invoke;

        try
        {
            var newValue = System.Linq.Expressions.Expression.Parameter(mapper.Body.Type);
            var assign = System.Linq.Expressions.Expression.Lambda<Action<TSource, TTarget>>(
                System.Linq.Expressions.Expression.Assign(mapper.Body, newValue),
                mapper.Parameters[0], newValue);

            Setter = assign.Compile().Invoke;
        }
        catch (ArgumentException)
        {
        }
    }

    public TTarget this[TSource source]
    {
        get { return Getter(source); }
        set { Setter(source, value); }
    }

    Func<TSource, TTarget> Getter { get; }
    Action<TSource, TTarget> Setter { get; }
}


Pack and unpack bytes to strings I need to write a function that "packs" an array of bytes (integers between 0 and 255) into a string. I also need to be able to perform the reverse operation, to get my byte array from the string that it was packed into. This needs to be done as fast as possible. Seeing as JavaScript has 16-bit strings, I packed two bytes per character. Here is my code and tests:

function pack(bytes) {
    var str = "";
    for(var i = 0; i < bytes.length; i += 2) {
        var char = bytes[i] << 8;
        if (bytes[i + 1])
            char |= bytes[i + 1];
        str += String.fromCharCode(char);
    }
    return str;
}

function unpack(str) {
    var bytes = [];
    for(var i = 0; i < str.length; i++) {
        var char = str.charCodeAt(i);
        bytes.push(char >>> 8);
        bytes.push(char & 0xFF);
    }
    return bytes;
}

var tests = [
    [],
    [126, 0],
    [0, 65],
    [12, 34, 56],
    [0, 50, 100, 150, 200, 250]
];

console.log("starting tests");
tests.forEach(function(v) {
    var p = pack(v);
    console.log(v, p, unpack(p));
});

And the output to that is:

starting tests
[] "" []
[126, 0] "縀" [126, 0]
[0, 65] "A" [0, 65]
[12, 34, 56] "ఢ㠀" [12, 34, 56, 0]
[0, 50, 100, 150, 200, 250] "2撖죺" [0, 50, 100, 150, 200, 250]

I have a few things I'd like feedback on:

This was the first time I used bitwise operators. Is this how it should be done?
Are there any speed improvements that could be made?
Can you guys think of any way to discard that last 0 byte when encoding then decoding an array with an odd number of bytes? (see test #4)

function pack(bytes) {
    var str = "";
// You could make it faster by reading bytes.length once.
    for(var i = 0; i < bytes.length; i += 2) {
// If you're using signed bytes, you probably need to mask here.
        var char = bytes[i] << 8;
// (undefined | 0) === 0 so you can save a test here by doing
//     var char = (bytes[i] << 8) | (bytes[i + 1] & 0xff);
        if (bytes[i + 1])
            char |= bytes[i + 1];
// Instead of using string += you could push char onto an array
// and take advantage of the fact that String.fromCharCode can
// take any number of arguments to do
//     String.fromCharCode.apply(null, chars);
        str += String.fromCharCode(char);
    }
    return str;
}

function unpack(str) {
    var bytes = [];
    for(var i = 0; i < str.length; i++) {
        var char = str.charCodeAt(i);
// You can combine both these calls into one,
//    bytes.push(char >>> 8, char & 0xff);
        bytes.push(char >>> 8);
        bytes.push(char & 0xFF);
    }
    return bytes;
}

so to put it all together

function pack(bytes) {
    var chars = [];
    for(var i = 0, n = bytes.length; i < n;) {
        chars.push(((bytes[i++] & 0xff) << 8) | (bytes[i++] & 0xff));
    }
    return String.fromCharCode.apply(null, chars);
}

function unpack(str) {
    var bytes = [];
    for(var i = 0, n = str.length; i < n; i++) {
        var char = str.charCodeAt(i);
        bytes.push(char >>> 8, char & 0xFF);
    }
    return bytes;
}


Comparison between strings This review is very short and very straight-forward. There are two people in the team and the checked-in code looks as follows.

string parameter = GetUnitType();
string target = "PM4000";
if(parameter.CompareTo(target) == 0)
  CarryOutProcess();

I got stuck here and needed to re-check the usage of CompareTo. I can't for my life understand any advantage of the above in comparison to:

string parameter = GetUnitType();
if(parameter == "PM4000")
  CarryOutProcess();

I'm pretty sure it's just a case of intentional obfuscation or, at the very least, lack of understanding a copied code.

Is the former inferior to the latter in every possible way?

The author of the first claims that since both work, they're comparably good and the difference is just a matter of personal taste.

There is a subtle difference between == and CompareTo: the latter is current-culture-aware (or you can pass another one in), the former is not.

However, as Ben points out in the comments, the official recommendation is to use Equals instead of CompareTo outside of sorting operations.

Your devs might also be coming from a Java background where == and equals are a bit more complicated and may very well produce different results. This isn't a concern in C#, so for the vast, vast majority of your equality checks the == will be more than sufficient.


Two sum problem implementation with Hashtable Problem statement:

  Given an array of integers, return indices of the two numbers such
  that they add up to a specific target. You may assume that each input
  would have exactly one solution, and you may not use the same element
  twice.

  Example:

Given nums = [2, 7, 11, 15], target = 9,
Because nums[0] + nums[1] = 2 + 7 = 9,
return [0, 1]

I have a working Java implementation with both time and space complexity of \$O(n)\$. However, I am looking for feedbacks on how to write better, more elegant code. Any suggestions on style, simplicity,  comments, etc., are welcome.

Note: function signature is provided to me in the interview, not mine. Otherwise I would return a Pair instead of int[].

import java.util.Hashtable;

public class Solution {
    public int[] twoSum(int[] nums, int target) {
        int loadFactor = 3;
        //key: num, value: num's parter's idx, if exists
        Hashtable<Integer, Integer> parterIdxHash = new Hashtable<Integer, Integer>(loadFactor*nums.length);
        for(int i = 0; i < nums.length; i++){
            int curVal = nums[i];
            Integer parterIdx = parterIdxHash.get(curVal);

            //found its parter
            if(parterIdx != null)
                return new int[]{parterIdx, i}; //parterIdx first because it's eariler in the array

            int parterVal = target - curVal;
            parterIdxHash.put(parterVal, i);

        }

        //should never reach here!
        return new int[]{-1, -1};
    }
}

Interesting solution for the problem. My first question as interviewer to you would be: are you sure it's O(n) time complexity? The worst case for hash lookup is O(n) so doesn't that make your total time complexity O(n²)?

Hint to answer this: when does that worst case occur, and why is this no problem for you? (Given how you initialised the map in your code you probably don't even need this hint to answer it correctly though).

You don't check your input values. What happens if nums is null?
I'm not saying your solution is wrong, but this should usually be a consious decission.
Your current choice is that the user of this method is responsible for passing non-null arguments or that we should start the fun journey of debugging where that NPE comes from.

The same reasoning goes for the target value. What if it's an impossible sum? Take for example the input nums = [2,3] and target=4. I partially like that you return an int[] result with negative indices instead of throwing an error. But the comment documenting it feels wrong.

I would have prefered that you would have handled it like an actual valid case and added a javadoc on top of the method that states that passing an impossible target results in [-1,-1].

Since you handle an impossible target that way, you could also choose to handle a null input for nums the same way. Add it to the javadoc and start your method with this simple check

if(nums == null){
    return new int[]{-1, -1};
}

Alternatively you can also throw explicit errors to force users to handle edge cases correctly.

It's generally better during an interview if you make those choices explicit. There's no real wrong choice, as long as you can tell why you picked a specific one. (A possible answer is: it's the easiest to implement, which to chooce often depends on what the method is for and how the company usually handles edge cases).

Why do you want an O(n) space and time complexity?
Wouldn't it be better to use O(1) space at the cost of O(n²) time if the implementation is easier?

For example:

private int[] twoSum(int[] nums, int target){
    if(nums == null) {
        return new int[]{-1, -1};
    }
    for (int i = 0; i < nums.length - 2; i++) {
        for (int j = i + 1; j < nums.length; j++) {
            if(nums[i]+nums[j]==target){
                return new int[]{i,j};
            }
        }
    }
    return new int[]{-1, -1};
}

Using this approach, you could ask the interviewer if the array is sorted. If it is you can replace the inner for loop with a search of the target in the remaining array. This turns the total time complexity into O(n*log(n)) instead.

Note that I wouldn't immediatly implement this for the interview. I would mention that these kind of optimisations are useful and possible, but that I would only do them after profiling shows these are the methods that take most time.

If it's a method that is only used a couple times per hour and on relatively small inputs then it's not worth putting in effort to optimise the implementation.
If it's a major function that runs millions of times per hour then it's worth optimising the hell out of it, even going as far as bit-trickery if wanted.

parter isn't the right word in this context. I'm assuming you meant partNer. Typos happen and not being native english myself I too make spelling mistakes. But this one is repeated too often to ignore in your piece of code.

You mention that you would have prefered to return a Pair. What would you do if we also wanted the methods threeSum fourSum or a general nSum?

In this example it doesn't matter too much, but it's best practice if you code against the interface, not the implementation. More specifically this line of code:

Hashtable<Integer, Integer> parterIdxHash = new Hashtable<Integer, Integer>(loadFactor*nums.length);

should have been:

Map<Integer, Integer> partnerIndices = new Hashtable<>(loadFactor*nums.lenth);

Since you didn't do this in your little example solution, the interviewer might conclude that you never do this. For local variables it doesn't change that much, but if you also use concrete classes as return types or as parameter inputs you violate the encapsulation principle.

It's also good practice to always put curly braces after each for/if/while/... statement. With maybe an exception when putting the entire statement on the same line for reasons explained here

As a final note I would like to emphasize that with interview solutions it's more important that you can tell what your solution is good at, what can still be improved and that your coding style is consistent. With this in mind you can always implement the easiest solution and tell the interviewer what the alternatives are without actually having to write them correctly.


Comments on SASS from CSS for a SASS beginner I've leaped into SASS and am loving it. I'm asking for comments here on whether my SASS is well-formatted.

Here's the original CSS based on Zurb's Foundation Pagination styles, but adapted for use with Laravel's built-in pagination:

div.pagination ul { display: block; height: 24px; margin-left: -5px; }
div.pagination ul li { float: left; display: block; height: 24px; color: #999; font-size: 14px; margin-left: 5px; }
div.pagination ul li a { display: block; padding: 1px 7px 1px; color: #555; font-weight: normal}
div.pagination ul li:hover a,
div.pagination li a:focus { background: $bgltblue; }
div.pagination ul li.disabled a { cursor: default; color: #999; }
div.pagination ul li.disabled:hover a,
div.pagination li.disabled a:focus { background: transparent; }
div.pagination ul li.active a { background: $headblue; color: white; font-weight: normal; cursor: default; }
div.pagination ul li.active a:hover,
div.pagination li.active a:focus { background: $headblue; }

Here's my manual conversion to SASS:

div.pagination {
  ul {
    display: block; height: 24px; margin-left: -5px;
    li {
      float: left; display: block; height: 24px; color: #999; font-size: 14px; margin-left: 5px;
      a {
        display: block; padding: 1px 7px 1px; color: #555; font-weight: normal;
      }
      &:hover a {
        background: $bgltblue;
      }
      &.disabled {
        a {
          cursor: default; color: #999;
        }
        &:hover a {
          background: transparent;
        }
      }
      &.active {
        a {
          background: $headblue; color: white; font-weight: normal; cursor: default;
          &:hover {
            background: $headblue;
          }
        }
      }
    }
  }
  li {
    a:focus {
      background: $bgltblue;
    }
    &.disabled {
      a:focus {
        background: transparent;
      }
    }
    &.active {
      a:focus {
        background: $headblue;
      }
    }
  }
}

I'd like to know if you think I have reduced it correctly, and where I might make improvements. I plan to replace the colours with vars already.

Some comments on your style of selecting:

div.pagination is overqualified. A container is almost always going to be a div, so you just can write .pagination
In many cases, there is no use for an extra container. You might as well apply the pagination class to the list itself: <ul class="pagination">
You also have a lot of redundancy in your selectors because you always use ul and li in the chains. This is not necessary and could be made easier:

.pagination ul {}

/* li's can only appear in lists, no need to add it to the selector */
.pagination li {}

/* The only links inside your pagination are usually in relation to the pagination */
.pagination a {}

If you have lists inside your pagination that need separate styling, use separate classes or place them outside of the actual pagination
Don't overqualify the .active and .disabled classes as well. They will end up being on the list items anyway

Conclusion:

You nest far too deep. Avoid nesting selectors deeper than 2–3 levels. Otherwise you get bloated CSS like this.

Compare my SCSS to yours:

.pagination {
    display: block;
    height: 24px;
    margin-left: -5px;

    li {
        float: left;
        display: block;
        height: 24px;
        color: #999;
        font-size: 14px;
        margin-left: 5px;

        &:hover a {
            background: $bgltblue;
        }

        &.disabled {
            a {
                cursor: default;
                color: #999;

                &:hover,
                &:focus {
                    background: transparent;
                }
            }
        }

        &.active {
            a {
                background: $headblue;
                color: white;
                font-weight: normal;
                cursor: default;

                &:hover,
                &:focus {
                    background: $headblue;
                }
            }
        }
    }

    a {
        display: block;
        padding: 1px 7px 1px;
        color: #555;
        font-weight: normal;

        &:focus {
            background: $bgltblue;
        }
    }
}


Don't MiTM me please To first understand how MiTM works I've made a few MiTM scripts, but this time I wanted to defend myself against such attacks. I'm unsure if my idea even works. The code seems to be working but I make a few assumptions and I'm not sure if those are correct.

Assumptions

When I do a traceroute the IP of the Attacker is in the route.
When I trace the route, and there is not a router device in my route it means an attacker(laptop) is listening on my connection
When I the attacker does an ARP-Spoof, the ARP-table has duplicate values.

Code issues

I have this long python -c "command" it feels a bit silly to do in bash

Any review is welcome!

Plus I would be happy if anyone has additional information on how to prevent MiTM attacks.

#!/bin/bash

route_checker() {
    private_route=$(python -c 'import re; import subprocess; from ipaddress import IPv4Address; print([ip for ip in re.findall(r"\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}", subprocess.Popen(["traceroute", "-n", "8.8.8.8"], stdout=subprocess.PIPE).communicate()[0]) if IPv4Address(bytearray(ip)).is_private])' | tr --delete []\')
    echo "Your private route = $private_route"
    echo "Checking the safety of route: $private_route"

    echo ""
    echo "Starting nmap scan"
    OLD_IFS=$IFS
    IFS=', ' read -ra IP <<< "$private_route"
    for ip in "${IP[@]}"; do
    echo "Checking ip adress $ip:"
    os_scan=$(nmap -A -T4 "$ip")
    if [[ "$os_scan" =~ "broadband router" ]]
    then
        echo "[*] Warning! No router found."
    else
        echo "[*] Safe! A router was found."
    fi
    done
    IFS=$OLD_IFS
}

arp_checker() {
    if [[ $(arp -an | awk '{print $4}' | sort | uniq -c | grep -v ' 1 ') ]]
    then
    echo "[*] Warning! You are being spoofed"
    else
    echo "[*] Safe! Did not find any duplicate ARP values"
    fi
}

main() {
    echo "@Ludisposed"
    echo ""
    echo "Checking for ARP spoofing"
    arp_checker

    echo ""
    echo "Checking if we have a safe private route"
    route_checker
}

main

A few words about assumptions:

You're assuming that arp is installed. This is definitely not true for my system. arp as well as other "core network tools" like ifconfig have been superseded by ip. The more modern equivalents are ip neigh show and ip addr. The obsolescense is explicitly mentioned on the manpage
IIUC (because traceroute isn't available on my arch) similar considerations apply for traceroute. Also do note that you should be able to use the same idea as for the arp-check in the creation of private_route:

route = $(tracepath -n 8.8.8.8 | awk '{print $2}')
private_route = # consider ipcalc or use $route as a smaller input to python

Final considerations:

You're inconsistent in the indentation of block scopes. In arp_checker the then and else block are indented, in route_checker they are, but the for loop body is not.
I strongly recommend indenting every block to increase readability.

Aside from that: Good job of not falling into common traps like:

not double-quoting variables
not using subshells
using unsafe condition checks
missing a shebang

From what I can see, this is a well-crafted script :)


Scala TryWith that closes resources automatically I'm quite new to Scala and was surprised that there doesn't seem to be anything like Java's try-with-resources block. So of course I wrote my own little construct. Is there anything I could improve?

import scala.util.control.NonFatal
import scala.util.Try
import scala.util.Failure
import scala.util.Success

object TryWith {
  def apply [Closable <: { def close() }, Result]
      (resGen: => T) (r: (Closable) => Result): Try[Result] = {

    Try(resGen) match {
      case Success(closable) => {
        try Success(r(closable)) catch {
          case NonFatal(e) => Failure(e)
        } finally {
          try {
            closable.close()
          } catch {
            case e: Exception => {
              System.err.println("Failed to close Resource:")
              e.printStackTrace()
            }
          }
        }
      }
      case Failure(e) => Failure(e)
    }
  }
}

Examples:

val res = TryWith(Source.fromFile("foo.bar")) {
  _.mkString
}

res match {
  case Success(str) => println(str)
  case Failure(e) => e.printStackTrace()
}

--

TryWith(new BufferedReader(new FileReader("foo.bar")))((reader)=> {
  var line = reader.readLine()
  while(line != null) {
    println(line)
    line = reader.readLine()
  }
})

Right now there is no way to react to or even detect a failed attempt to close the resource. I can see four ways to make that possbile but I'm not that happy with any of them:

Instead of catching the Exception, let it through. It can now be dealt with but kind of defeats the purpose of the whole TryWith block.
Return a Failure[Result] if closing fails. I don't like this because the action was actually a success and there is a valid result. By returning a Failure that result is lost.
Return a pair of Trys, i.e. (Try[Result], Try[Unit]). The first one is the result of the operation, the second one is the result of the closing.
Pass a (Closable) => Unit error handler as argument. This would also allow trying to close the resource again (which most likely won't change anything though).

Is handling a failed close even important enough to think about? I know it can happen but usually there is no way to recover from it.

There are a couple of places where you can improve.

First the pedantic stuff.

Closable should probably be Closeable
Cleaning up your imports can make it easier to get to the parts of your program that matter. In this case you can consolidate your imports to a much simpler version:

import scala.util.control.NonFatal
import scala.util.{Failure, Success, Try}

Make sure it compiles before posting. In this case, there was a missed rename, and it needed an additional import to compile without warnings (needed by the reflected call to #close on a structurally typed value).
Several of your pairs of curly braces are optional, and removing them makes it much easier to read.
Scala convention favors single character for type parameters. I'm not sure that I 100% agree with this particular convention for classes with type parameters, but for a single method it's best to stick to the convention.

Ok, now that's out of the way, to the good stuff :)

Use interfaces if they are already there

Scala's interoperability with Java is one of it's strengths, so re-inventing an interface isn't really needed. Additionally, using java.io.Closeable gets rid of the structural typing, which is a nifty feature, but it's better to avoid if it's not strictly needed. This simplifies things considerably, particularly the type signature.

FlatMap

Try is a Monad, which means a bunch of stuff to category theory buffs - which I don't particularly care about. What I do care about Monads is that they have two methods defined on them that can make life much easier. flatMap is the first, and you've basically defined the content of this method in your outermost pattern match.

Refactored Version 1

import java.io.Closeable
import scala.util.control.NonFatal
import scala.util.{Failure, Success, Try}

object TryWith {
  def apply [C <: Closeable, R](resGen: => C) (r: Closeable => R): Try[R] =
    Try(resGen).flatMap(closeable => {
      try {
        Success(r(closeable))
      }
      catch {
        case NonFatal(e) => Failure(e)
      }
      finally {
        try {
          closeable.close()
        }
        catch {
          case e: Exception =>
            System.err.println("Failed to close Resource:")
            e.printStackTrace()
        }
      }
    })
}

Principle of Least Surprise

So far we haven't changed how the method actually works. In the possibilities you mentioned for how to handle a failure to close you missed the most important option: mimic the behavior of the Java try-with-resources block.

This is important for two reasons: the first is that it's a pretty sane way of handling this case, the second is that it will be the behavior expected by anyone coming from a Java background. Acting in an intuitive manner is a very powerful thing.

Java handles failures to close in this manner:

If the block throws an exception, give priority to this exception. The exception thrown by #close is added as a suppressed exception, and the modified exception is thrown.
If the block did not throw an exception, allow the exception from close to propagate upward.

The basic premise is that exceptions are exceptional, so they should never be swallowed.

Refactored Version 2

import java.io.Closeable
import scala.util.control.NonFatal
import scala.util.{Failure, Try}

object TryWith {
  def apply[C <: Closeable, R](resource: => C)(f: C => R): Try[R] =
    Try(resource).flatMap(resourceInstance => {
      try {
        val returnValue = f(resourceInstance)
        Try(resourceInstance.close()).map(_ => returnValue)
      }
      catch {
        case NonFatal(exceptionInFunction) =>
          try {
            resourceInstance.close()
            Failure(exceptionInFunction)
          }
          catch {
            case NonFatal(exceptionInClose) =>
              exceptionInFunction.addSuppressed(exceptionInClose)
              Failure(exceptionInFunction)
          }
      }
    })
}

The Test Suite

import java.io.Closeable
import org.scalatest.{ Matchers, WordSpec }
import scala.util.{ Failure, Success }

class TryWithSpec extends WordSpec with Matchers {
  // Exceptions and errors here so we don't pay the stack trace creation cost multiple times
  val getResourceException = new RuntimeException
  val inFunctionException = new RuntimeException
  val inCloseException = new RuntimeException
  val getResourceError = new OutOfMemoryError
  val inFunctionError = new OutOfMemoryError
  val inCloseError = new OutOfMemoryError

  val goodResource = new Closeable {
    override def toString: String = "good resource"
    def close(): Unit = {}
  }

  "TryWith" should {
    "catch exceptions getting the resource" in {
      TryWith(throw getResourceException)(println) shouldBe Failure(getResourceException)
    }

    "catch exceptions in the function" in {
      TryWith(goodResource){
        _ => throw inFunctionException
      } shouldBe Failure(inFunctionException)
    }

    "catch exceptions while closing" in {
      TryWith(new Closeable {
        def close(): Unit = throw inCloseException
      })(_.toString) shouldBe Failure(inCloseException)
    }

    "note suppressed exceptions" in {
      val ex = new RuntimeException
      val result = TryWith(new Closeable {
        def close(): Unit = throw inCloseException
      })(_ => throw ex)

      result shouldBe Failure(ex)
      val Failure(returnedException) = result
      returnedException.getSuppressed shouldBe Array(inCloseException)
    }

    "propagate errors getting the resource" in {
      intercept[OutOfMemoryError] {
        TryWith(throw getResourceError)(println)
      } shouldBe getResourceError
    }

    "propagate errors in the function" in {
      intercept[OutOfMemoryError] {
        TryWith(goodResource){
          _ => throw inFunctionError
        }
      } shouldBe inFunctionError
    }

    "propagate errors while closing" in {
      intercept[OutOfMemoryError] {
        TryWith(new Closeable {
          def close(): Unit = throw inCloseError
        })(_.toString)
      } shouldBe inCloseError
    }

    "return the value from a successful run" in {
      TryWith(goodResource)(_.toString) shouldBe Success("good resource")
    }
  }
}


HackerRank leaderboard challenge I have been doing this challenge in which I need to determine and return one's (i.e. alice) rank on a leaderboard by comparing her scores to the others'. I made this work in all but this one case where there are 200000 scores on the board, so I believe my program is not effective at large scale. I am receiving a timeout error. I tried to shorten the program as much as possible but I think loops are still too much of a problem. I would appreciate any suggestions.

#include <bits/stdc++.h>

using namespace std;

vector <int> climbingLeaderboard(vector <int> scores, vector <int> alice) {
    vector<int> result;
    reverse(alice.begin(), alice.end());
    int lastIndex = 0;
    int lastRank = 1;

    for (int i = 0; i < alice.size(); i++) {

        int rank = lastRank;
        for (int j = lastIndex; j < scores.size(); j++) {
            if (alice[i] >= scores[j]) {
                lastIndex = j;
                break;
            }

            else if (alice[i] < scores[j]) {
                rank++;
            }

            if (scores[j] == scores[j+1] && rank != 1) {
                rank--;
            }
        }
        result.push_back(rank);
        lastRank = rank;
    }
    reverse(result.begin(), result.end());

    return result;
}

int main() {
    int n;
    cin >> n;
    vector<int> scores(n);
    for(int scores_i = 0; scores_i < n; scores_i++){
       cin >> scores[scores_i];
    }
    int m;
    cin >> m;
    vector<int> alice(m);
    for(int alice_i = 0; alice_i < m; alice_i++){
       cin >> alice[alice_i];
    }
    vector <int> result = climbingLeaderboard(scores, alice);
    for (ssize_t i = 0; i < result.size(); i++) {
        cout << result[i] << (i != result.size() - 1 ? "\n" : "");
    }
    cout << endl;

    return 0;
}

Sample Input

7
100 100 50 40 40 20 10
4
5 25 50 120

Sample Output

6
4
2
1

Explanation

The second row of input is composed of the scores on the leaderboard and the fourth row contains the scores of Alice. The score she gets during each attempt is compared with other scores on the leaderboard to determine her rank. Dense Ranking is used here. So from the sample input, the first two 100s on the leaderboard rank 1st and then 50 ranks the 2nd, both 40s rank 3rd and so on.

don't use using namespace std

You should not use using namespace std because it eliminates some important information, which may lead to some conflicts with your classes or classes of other libraries. It is not uncommon to have a vector class, which has a totally other meaning than the std::vector. And to be honest, it isn't that much more work, to type std:: in front of the classes/functions, is it?

use std algorithms

You do everything with for loops, which is bloated and error prone. Use the algorithms of the std namespace instead. I provide you with a short an clean example, how you could do the above task (and more) with less lines.

your algorithm itself

You should think about logical improvements. Is it necessary to store every score in a vector, when equal scores produce the same rankings, and the following scores just increments by one? Therefore I took the std::unique algorithm, which returns a forward iterator. std::unique doesn't erase anything itself. It just moves duplicates to the end of the range (our std::vector); you have to erase them manually.
But, before you can use std::unique you have to order your elements. I don't know if it's guaranteed to get the score input in a descending order; thus I simply use std::sort to ensure this.
After that, it's a simple std::lower_bound (which is a binary search instead a linear search) to get the first iterator to an element, which is not less than the provided score (ok, I had to pass an other function object, because we want to check for greater and not for less).
Iterators starts with a zero index. This means, we have to add 1 to our index, to get the official ranking.
Easy, huh?

#include <iostream>
#include <vector>
#include <algorithm>
#include <functional>

int main()
{
    std::vector<int> scores { 100, 100, 50, 40, 40, 20, 10 };
    std::vector<int> aliceScores { 5, 25, 50, 120 };
    // sort and erase duplicates
    std::sort(std::begin(scores), std::end(scores), std::greater<int>());
    scores.erase(std::unique(std::begin(scores), std::end(scores)), std::end(scores));

    // just a lambda to make the output part more readable
    auto locateRanking = [](const auto& _scores, int _newScore){
        auto itr = std::lower_bound(std::begin(_scores), std::end(_scores), _newScore, std::greater<int>());
        return std::distance(std::begin(_scores), itr) + 1;
    };

    // output part
    for (auto newScore : aliceScores)
    {
        std::cout << locateRanking(scores, newScore) << std::endl;
    }
}

tl;dr

Sort the vector, remove the duplicates and use a binary search instead of your linear approach and you will discover a huge performance benefit.