Sierpinski’s Gasket Triangle in JavaScript I wrote the Sierpinski’s Gasket Triangle in JavaScript, but I feel the code can be better, especially from L32 to L47. Could you make it more organized?

var canvas = document.getElementById('chaos');
var ctx = canvas.getContext('2d');

const GenerateRand = () => Math.floor(Math.random() * 7);
const updateDot = (x, y, point) => {
  let X = Math.min(x,point.x)+(Math.max(x,point.x)-Math.min(x,point.x))/2;
  let Y = Math.min(y,point.y)+(Math.max(y,point.y)-Math.min(y,point.y))/2;
  return {x: X, y: Y};
}
const createDot = (obj) => {
  ctx.beginPath();
  ctx.arc(obj.x, obj.y, 1, 0, 2 * Math.PI, false);
  ctx.lineWidth = 1;
  ctx.strokeStyle = '#fc3';
  ctx.stroke();
}

const pA = {x: canvas.width/2, y: 5};
const pB = {x: 5, y: canvas.height-5}
const pC = {x: canvas.width-5, y: canvas.height-5}

createDot(pA);
createDot(pB);
createDot(pC);

const begin = (iterations) => {
  let x = canvas.width/4;
  let y = canvas.height/2;
    for(let i=0;i<iterations;i++) {
    createDot({x, y});
    let randN = GenerateRand();
    if(randN == 1 || randN == 2) {
      const currentDot = updateDot(x, y, pA);
      x = currentDot.x;
      y = currentDot.y;
    }
    else if(randN == 3 || randN == 4) {
      const currentDot = updateDot(x, y, pB);
      x = currentDot.x;
      y = currentDot.y;
    }
    else if(randN == 5 || randN == 6){
      const currentDot = updateDot(x, y, pC);
      x = currentDot.x;
      y = currentDot.y;
    }
  }
}

let time=0;
let timer = setInterval(() => {
   if(time >= 500) return clearInterval(timer)
   begin(500);
   time++;
}, 200);
<div>
      <canvas id="chaos" width="500" height="500"></canvas>
</div>

Performance

For positive numbers < 2 ^ 31 use num | 0 (bitwise or zero) to floor
You are drawing an arc that is 1 pixel in radius, with the stroke width of 1 the diameter is 3 pixels. This covers an area much greater than the point you sample. Use fillRect to draw a single pixel as its much quicker. Better yet as they are all the same color create a single path and use ctx.rect to add to it. Render all rect in one pass at the end of begin function.
Avoid creating objects needlessly. Create a working object and use that to hold intermediate values. This can greatly reduce memory allocation and GC overheads. Eg the object you return in updateDot is a waste of memory and time.
If you test two numbers to find the max or min, knowing either means you also know the other and thus do not need to test for it. The long lines Math.min(p.y, p1.y) + (Math.max(p.y, p1.y) - Math.min(p.y, p1.y)) / 2 can be reduced by a single test and give significant performance improvement.

Style

Use const for constants. Eg canvas and ctx should be const.
Capitals only for names of objects that are instantiated with the new token. Eg GenerateRand should be generateRand
Avoid repeated code by using functions. Eg you create many instances of an object {x,y}, would be better as a function.
Spaces between operators, commas, etc.
Use === rather than ==
else on the same line as the closing }
The final statement in function begin does not need the test (randN == 5 || randN == 6) (assuming you want a new point each iteration)

Code
The random number generated is from 0 to 6 and you ignore 0, redrawing the same point 1 in 7 times. You can reduce the random to give 3 values 0,1,2 and perform the correct calculation on that or use a counter and cycle the points.
You could also put the points pA, pB, pC in an array and index them directly via the random number.
Rather than use setInterval, use setTimeout. That way you don't need to clear the timer each time.
Put magic numbers in one place and name them as constants.
You reset the start point each time delay is called (first two lines). Better to just let it keep going. It may also pay to stop the rendering after a fixed amount of points have been rendered.
The rewrite.
This is just an example of the various points outlined above.
Also a few modifications

Automatically adjust number of points rendered to keep the GPU load steady.
Stop rendering after a fixed number of points rendered.
The starting points pA,pB,pC are in an array.
Magic numbers as constants.
Using a single render path to draw all points per render cycle.
Using a working point wPoint to hold coordinates rather than create a new point for each point rendered.

const ctx = canvas.getContext('2d');

const padding = 5;
const renderDelay = 200;
const maxTime = 2;  // time in ms allowed to render points.
const maxPointsToDraw = canvas.width * canvas.height * (1 / 3);

var pointsPerRender = 500; // points to render per render pass
var totalPoints = 0; // count of total points drawn
ctx.fillStyle = '#fc3';

const generateRand = () => Math.random() * 3 | 0;
const point = (x, y) => ({x, y});
const drawDot = p => ctx.rect(p.x, p.y, 1, 1);

const updateDot = (p, p1) => {
    p.x = p.x < p1.x ? p.x + (p1.x - p.x) / 2 : p1.x + (p.x - p1.x) / 2;
    p.y = p.y < p1.y ? p.y + (p1.y - p.y) / 2 : p1.y + (p.y - p1.y) / 2;
    return p;
}

const points = [
    point(canvas.width / 2, padding),
    point(padding, canvas.height - padding),
    point(canvas.width - padding, canvas.height - padding)
];
const wPoint = point(canvas.width / 4, canvas.height / 2); // working point

const renderPoints = iterations => {
    totalPoints += iterations;

    const now = performance.now();
    ctx.beginPath();
    while (iterations --) { drawDot(updateDot(wPoint, points[generateRand()])) }
    ctx.fill();
    const time = performance.now() - now;

    // use render time to tune number points to draw
    // Calculates approx time per point and then calcs number of points
    // to render next time based on that speed.
    // Note that security issues mean time is rounded to much higher
    // value than 0.001 ms so must test for 0 incase time is zero
    pointsPerRender = maxTime / ((time ? time : 0.1)/ pointsPerRender);

    if (totalPoints < maxPointsToDraw) {
        setTimeout(renderPoints, renderDelay, pointsPerRender | 0);
    }
}
renderPoints(pointsPerRender);
<canvas id="canvas" width="500" height="500"></canvas>


Calculating length with different units We have an exercise to design a simple library, which can calculate some length with different unit, e.g.

2m*2 + 20cm/2 - 5mm

and the result can be any of:

4.095m
409.5cm
4095mm

I want to design it in Object-oriented, and have a solution like this:

public class Length {

    private final double value;
    private final Unit unit;

    public Length(double value, Unit unit) {
        this.value = value;
        this.unit = unit;
    }

    @Override
    public boolean equals(Object obj) {
        Length length = (Length) obj;
        return this.unit.toMM(this.value) == length.unit.toMM(length.value);
    }

    public Length add(Length added) {
        return new Length(this.unit.toMM(this.value) + added.unit.toMM(added.value), Unit.mm);
    }

    public Length subtract(Length another) {
        return new Length(this.unit.toMM(this.value) - another.unit.toMM(another.value), Unit.mm);
    }

}

enum Unit {
    m(1000), cm(10), mm(1);
    private final int rate;

    Unit(int rate) {
        this.rate = rate;
    }

    public double toMM(double value) {
        return rate * value;
    }
}

It works, but I have several questions:

Is there anything wrong from the OO point of view?
Unit has a method toMM which is bind to a concrete type of unit mm(millimeter), I don't feel good about it, is there any way to improve it? Or it's just acceptable since we have to choose one?
Length has chosen mm for all kinds of unit, is it good? I'm looking for a way to avoid it, but not found

These questions are mostly based on this possible requirement change: don't want to support mm anymore. Most of the code has to be modified in this case.

Update: from this article, seems it's not a good idea to use enum here for different units?

An alternative approach would be to say that a length is measured in SI units (meters).

Also, if performance is not a concern, using a BigDecimal instead of a double would avoid potential rounding errors.

It could look like:

public class Length {
  private final BigDecimal meters;
  public Length(BigDecimal meters) { ... }
  public Length(BigDecimal value, Unit unit) { /*convert to m*/}

  public BigDecimal getLength(Unit unit) { /*convert to unit*/ }
  public BigDecimal getLength() { /*return length in SI, i.e. meters*/ }

  public int hashCode() { return meters.hashCode(); }
  public boolean equals(Object o) {
    /*usual checks */
    return meters.compareTo(other.meters) == 0;
  }
  //other methods
}

This also avoids having to arbitrarily choose a "base" unit (mm in your example).

Finally I would suggest defining an interface for Unit:

public interface Unit {
  BigDecimal toMeters(BigDecimal length);
  //and possibly additional helper methods:
  default BigDecimal convert(BigDecimal length, Unit sourceUnit) { ... }
}

Then your enum would be:

public enum StandardUnit implements Unit {
  MM("0.001"); //etc.

  private final BigDecimal multiplier;
  StandardUnit(String mulltiplier) { this.multiplier = new BigDecimal(multiplier); }
  public BigDecimal toMeters(BigDecimal length) { return length.multiply(multiplier); }
}

This will allow a more flexible design where you can add units by implementing the interface or use the provided standard units.


Loading more items for an activity feed that uses a global page counter I have this script which is for a news feed. A button click will grab more data from the server if there is any and then add it to the box. It doesn't allow any more than 10 pages of results currently and how to properly store that page counter is what this question is about.  loadMoreFeedItems() is the function attached to the onclick event of the aforementioned button.

<script type="text/javascript">

    window.page = 0;

    function loadMoreFeedItems() {
        var options = {};
        options.page = window.page;
        $.ajax({
            url: "home/feed",
            data: options,
            dataType: "html",
            success: function (data) {
                // Code to append the data received into the feed element
                window.page++;
                // Code to hide "load more" button if window.page exceeds 10
            }
        });
    }

    (function () {
        loadMoreFeedItems();
    })();

</script>

So I did a little reading about global variables in JavaScript[1][2][3]  and while this works, I got the vibe that this could be problematic, so I'm wondering if there is a better way. One alternative I thought of was to create some kind of hidden  that stores the value in page rather than the browser, and then I'd modify/retrieve it using a jQuery selector, but that seems even dirtier.

This doesn't have anything to do with ajax or the options exactly, just about "counting up" so i will show my example using a function called countUp which logs ever increasing numbers.

In short you can make page a member of countUp. You will recall that in javascript that functions are very much first class citizens and they are objects. Its easier to see this of course if you write your function definitions like:

var countUp = function () { /* .. function body */ }

So the variable countUp can have in it any number of members. The member we are interested in is page or more precisely this.page. So lets put this all together.

var countUp = function ()
{
    this.page = (typeof this.page !== "undefined") ? this.page : 0;
    this.page++;
    console.log(this.page);
}

countUp();
countUp();
countUp();
// logs 1 2 3


Letter Changes coderbyte I've solved the problem below just would like some feedback?

  The function LetterChanges(str) takes a str parameter

  Replace every letter in the string with the letter following it in the
  alphabet (ie. c becomes d, z becomes a). Then capitalize every vowel
  in this new string (a, e, i, o, u) and finally return this modified
  string.

function LetterChanges(str) {
  var newString = '';
  var newCode = 0;
  var len = str.length;
  var i = 0;

  for (; i < len; i++){
  /*Using ASCII code: this if statement return true if character at
  str[i] is a - y*/
  if (str.charCodeAt(i) >= 97 && str.charCodeAt(i) <= 121) {
      //Assign ASCII code plus 1 to a variable newCode
      newCode = str.charCodeAt(i) + 1;
      /*Check to see newCode variable is a lower case vowel from ASCII
      and if so, add capital of that vowel to newString variable, else
      use newCode variable to add character to newString*/
      if (newCode == 101) {
          newString += String.fromCharCode(69);
      } else if (newCode == 105) {
          newString += String.fromCharCode(73);
      } else if (newCode == 111) {
          newString += String.fromCharCode(79);
      } else if (newCode == 117) {
          newString += String.fromCharCode(85);
      } else {
          newString += String.fromCharCode(newCode);
      }

  /*Using ASCII code: this if statement return true if character at
  str[i] is z, in that case adding capital A*/
  } else if (str.charCodeAt(i) == 122){
      newString += String.fromCharCode(65);

  /*else just add to newString i.e uppercase(question didn't specify),
  symbols, number...*/
  } else {
      newString += str[i];
  }
 }
return newString;

}

DRY (Don't Repeat Yourself)

You have a list of else if statements that all do the same thing. JavaScript has the .toUpperCase method so hardcoding character codes to perform capitalization is unnecessary. You can do newString += str[i].toUpperCase().

Use variables for constant values

Instead of constant values directly in your code, use a variable to store them. You know you want to capitalize vowels, so you can create an array var vowels = ['a', 'e', 'i', 'o', 'u'] to store your constants. This makes it easier to modify in the future - like if the requirements changed and now y is a vowel.

Putting these two together, your capitalization code can be pretty concise:

// vowels.indexOf(str[i]) !== -1 would work, too
if(vowels.includes(str[i]){
  newString += str[i].toUpperCase();
}


Form validation function for React I need to validate multiple input fields using React. I am using a simple if-else block for each input field but I would like to optimize my code and make it better. I feel there can be a better way to validate multiple input fields.

  validateForm = () => {
    const {
      firstName, lastName, mobileNo, city, empType, salary
    } = this.state;

    if (firstName !== '') {
      this.setState({
        firstNameMsg: '',
        validFirstName: false
      });
    } else {
      this.setState({
        firstNameMsg: 'Please enter a vaild first name.',
        validFirstName: true
      });
    }
    if (lastName !== '') {
      this.setState({
        lastNameMsg: '',
        validLastName: false
      });
    } else {
      this.setState({
        lastNameMsg: 'Please enter a vaild last name.',
        validLastName: true
      });
    }

    if (mobileNo.match(/^(\+\d{1,3}[- ]?)?\d{10}$/) && !(mobileNo.match(/0{5,}/))) {
      this.setState({
        mobileNoMsg: '',
        validMobileNo: false
      });
    } else {
      this.setState({
        mobileNoMsg: 'Please enter a vaild mobile number.',
        validMobileNo: true
      });
    }

    if (city !== '') {
      this.setState({
        cityMsg: '',
        validCity: false
      });
    } else {
      this.setState({
        cityMsg: 'Please enter a vaild city.',
        validCity: true
      });
    }

    if (empType !== 'Self-Employed') {
      this.setState({
        empTypeMsg: '',
        validEmpType: false
      });
    } else {
      this.setState({
        empTypeMsg: 'Some msg',
        validEmpType: true
      });
    }

    if (salary !== '') {
      this.setState({
        salaryMsg: '',
        validSalary: false
      });
    } else {
      this.setState({
        salaryMsg: 'Please enter a vaild salary.',
        validSalary: true
      });
    }
  }

Validation is so common that I recommend generalizing it a little bit (not too much though, there are too many special cases).

One way is to create validations, schema, and data separate from each other. For example:

const validators = {
    required: (config, value) => value !== '',
    mobileNo: (config, value) => value.match(/^(\+\d{1,3}[- ]?)?\d{10}$/) && !(value.match(/0{5,}/)),
    equal: (config, value) => value === config.value
}

const validate = (data, schema) => {
    const messages = {}

    for(let [name, rules] of Object.entries(schema)){
        for(let rule of rules){
            if(!validators[rule.type](rule, data[name])){
                if(!messages.hasOwnProperty(name)) messages[name] = []
                messages[name].push(rule.msg)
            }
        }
    }

    return messages
}

const schema = {
    firstName: [{type: 'required', msg: 'Please enter a valid first name.'}],
    lastName: [{type: 'required', msg: 'Please enter a valid last name.'}],
    mobileNo: [{type: 'required', msg: 'Please enter a mobile number'}, {type: 'mobileNo', msg: 'Please enter a valid mobile number.'}],
    city: [{type: 'required', msg: 'Please enter a vaild city.'}],
    empType: [{type: 'equal', value: 'Self-Employed', msg: 'Some msg'}],
    salary: [{type: 'required', msg: 'Please enter a vaild salary.'}],
}

// In react component
validateForm(){
    const validationMessages = validate(this.state.data, schema)
    this.setState({validationMessages})
}

You can take this much further by adding templated validation messages, and fancy rules of all sorts. But I recommend keeping it simple. You can make multiple variants of the validate function for different needs as they come.


Prime Number Checker This is my prime number checker in C++.  I know that calculating the square root of a number can be very costly, so I don't do it unless the number is larger than 10k, where we would have to check 2450 extra values \$5k-\sqrt{10k}\over2\$.

void primeChecker(std::vector<int> &primes, int val)
{
    int max = val > 10000 ? sqrt(val) : val / 2;
    for (int i = 0; primes[i] <= max; i++)
    {
        if (val % primes[i] == 0)
        {
            return;
        }
    }
    primes.push_back(val);
    std::cout << val << std::endl;
}

Running it like this, it takes a few seconds to calculate the top 1,000,000 primes:

int main()
{
    std::vector<int> primes;
    primes.push_back(2);

    for (int i = 3; i < 1000000; i+=2)
    {
        primeChecker(primes, i);
    }

    return 0;
}

Am I breaking any coding standards, and are there any ways to improve this?

Instead of hard-coding 1000000000, use a constant:

const int maxPrimes = 1000000000;

Then, use it in the loop like this:

for (int i = 3; i < maxPrimes; i+=2)

This will make it clear what this value is.

Regarding the timing, you could use something from <ctime> or another suitable library to time certain code sections. For instance, you may time the loop in main() or just the one in primeChecker() for testing purposes.  This may be OS-dependent, so I'll let you decide on this if you wish to try it.


Bash script to replace string in database How secure is my script?

#!/usr/bin/env bash

echo "Input database name"
read databaseName
echo "The database name is set to $databaseName"

echo "Input database host"
read databaseHost
echo "The database host is set to $databaseHost"

echo "Input database user"
read user
echo "The database user is set to $user"

stty -echo
echo "Input database user password"
read password
stty echo

echo "Input url to search for"
read searchUrl
echo "The search url is set to $searchUrl"

echo "Input url to replace with the searched url"
read replaceUrl
echo "The replace url is set to $replaceUrl"

mysql --user="$user" --password="$password" --host="$databaseHost" --database="$databaseName" -e "UPDATE messages SET message = REPLACE(message, '$searchUrl', '$replaceUrl')"

Is it bad practise to get the user his password like this? Or is there anything I can improve?

You can make Bash's read built emit a prompt by using its -p option:

read -p "Database name: " databaseName

and you can make it turn off echoing with -s:

read -s -p "Database password for $user: " password

Avoid writing "$password" into the mysql command line like that - as the man page says:

  Specifying a password on the command line should be considered insecure. See Section 6.1.2.1, “End-User Guidelines for Password Security”. You can use an option file to avoid giving the password on the command line.

It also says

  If you omit the password value following the --password or -p option on the command line, mysql prompts for one.

So I'd recommend not reading the password in this script, but passing --password to tell mysql to do its own prompting.

The other thing that's dangerous is using user input to construct the SQL query string - even if your users are trusted (and you know they have permission to connect and execute this command), it's worth validating the strings to prevent accidents.  An alternative could be to use Bash to quote the arguments:

command=$(printf 'UPDATE messages SET message = REPLACE(message, %q, %q') \
                 "$searchUrl", "$replaceUrl")

mysql --user="$user" --password \
   --host="$databaseHost" --database="$databaseName" \
   -e "$command"

It may be worth writing the script in a different language (or passing these variables to a sub-script in the different language), to enable the use of prepared statements.  I thought there might be a way to define variables for the mysql command like you can for Awk, and then use them in statements, but I couldn't find any provision for that, unfortunately.

An enhancement I would make is to ask for confirmation after reading all the interactive inputs.  Right now, if I make a mistake in replaceUrl (e.g. I manage to press \ and Enter together), then I don't get a chance to interrupt the update.  At the very least sleep 2 && mysql so I can Control+C before it's too late.


Function to generate all valid parentheses of n*2 For a leetcode problem to generate the valid parentheses of length 2n, I have written this solution. It is working for smaller inputs but not for larger inputs. Please suggest a way to optimise my code.

I have read the Stack Overflow questions but was unable to implement the same concept in my code.

 //I have writte a bruteforce approch and got all the possible combinations..
//Then filter all the combinations to check if every one of them is a valid combination

public class Solution {

    public ArrayList<String> generateParenthesis(int a) {

        ArrayList<String> allCombinations = getAllPossibleCombinations(2*a,new ArrayList<String>());
        ArrayList<String> prunedCombinations = new ArrayList<String>();

        for(String comb : allCombinations)
        {
            if(isCombinationValid(comb))
            {
                prunedCombinations.add(comb);
            }
        }

        return prunedCombinations;
    }

    //Bruteforce way to get all the combinations...
    public ArrayList<String> getAllPossibleCombinations(int num,ArrayList<String> presentCombinations)
    {
        //templist is used to populate the result and pass the values
        ArrayList<String> tempList = new ArrayList<String>();
        if(num == 0) //num is zero, so return the combinations
        {
            return presentCombinations;
        }
        else
        {
            //In first call, just add ( and )
            if(presentCombinations.isEmpty())
            {
                tempList.add("(");
                tempList.add(")");
            }
            else //From second call, add ( and ) for current values in stack
            {
                for(String presentComb : presentCombinations)
                {
                    tempList.add(presentComb+"(");
                    tempList.add(presentComb+")");
                }
            }
        }
        //To pass the intermediate values and num-1
       return getAllPossibleCombinations(num-1,tempList);
    }

    //Check if an individual combination is valid..
    //Use stack to match the paranthesis
    public boolean isCombinationValid(String comb)
    {
            Stack<Character> st = new Stack<Character>();

            for(int i=0 ; i < comb.length() ; i++)
            {
                char current = comb.charAt(i);
                if(current == '(')
                {
                    st.push('(');
                }
                else
                {
                    if(!st.isEmpty() && st.peek() == '(')
                    {
                        st.pop();
                    }
                    else
                    {
                        st.push(')');
                    }
                }
            }

            if(st.isEmpty())
            {
                return true;
            }
            else
            {
                return false;
            }

    }

}

isCombinationValid

          for(int i=0 ; i < comb.length() ; i++)
          {
              char current = comb.charAt(i);

Since you never use i for anything other than generating current, you could just

          for (char current : comb.toCharArray())
          {

This way you wouldn't have to manage i at all.

        if(st.isEmpty())
        {
            return true;
        }
        else
        {
            return false;
        }

This is more complicated than need be.

        return st.isEmpty();

This has the same result in seven fewer lines.

            if(current == '(')
            {
                st.push('(');
            }
            else
            {
                if(!st.isEmpty() && st.peek() == '(')
                {
                    st.pop();
                }
                else
                {
                    st.push(')');
                }
            }

Instead, you can say

            if (current == '(')
            {
                st.push('(');
            }
            else if (st.isEmpty())
            {
                return false;
            }
            else
            {
                st.pop();
            }

You know that anything in the stack must be a '(', so you don't need to peek to check.  And if you ever find an extra ')', you can quit immediately.  You already know that it's not well-formed.  So if the first character is a ')', you don't even have to go through the rest of the string.

I prefer not to use an else with an if !.  I just find the logic easier to follow this way.  And I switched to the more common else if rather than

getAllPossibleCombinations

        if (presentCombinations.isEmpty())
        {
            temp.add("(");
            temp.add(")");
        }
        else //From second call, add ( and ) for current values in stack
        {

You are doing this with each recursive call, but that's not necessary.  Rather than do this in getAllPossibleCombinations, replace

        ArrayList<String> allCombinations = getAllPossibleCombinations(2*a,new ArrayList<String>());
        ArrayList<String> prunedCombinations = new ArrayList<String>();

        for(String comb : allCombinations)

with

        List<String> initialCombinations = new ArrayList<>();
        initialCombinations.add("(");

        List<String> prunedCombinations = new ArrayList<>();
        for (String comb : getAllPossibleCombinations(2*a, initialCombinations)))

You don't need the ")" one at all, because no valid version can start with a closing parenthesis.

I also changed your use of ArrayList<String> as the variable type to just use List<String>.  Unless you use some functionality specific to ArrayList, it is generally better to use the interface (List) rather than the implementation (ArrayList) as the type.  This makes it easier to change the implementation in the future.

I also changed the right hand side to use the newer <> notation.  It's possible that your Java doesn't support that.  If so, just leave it as <String>.

One might argue that this makes getAllPossibleCombinations less reusable, as you have to pass a specific opening parameter.  This is true, but it was also true of your original.  To get around this, you can specify a version that does not require a List to be passed:

    public List<String> getAllPossibleCombinations(int maximumPairCount) {
        List<String> initialCombinations = new ArrayList<>();
        initialCombinations.add("(");

        return getAllPossibleCombinations(maximumPairCount - 1, initialCombinations);
    }

Now you could declare the two parameter version private, as all consumers should call this version instead.  And you can get rid of your isEmpty check, as it will never be called that way.  This won't shorten your code, but it will make it run faster, as you save checking isEmpty for nearly every call.

Optimization

The code that you link checks for validity as it builds the string.  If you do that, you don't have to save invalid combinations like "))))".  Since there are \$2^{2*n}\$ combinations but only a smaller number of these are valid, this should make your program take longer to run out of memory.

Note that if you do this, you don't return false if the stack has extra '(' in it.  It's valid for a partial expression to have unmatched '('.  What's invalid is unmatched ')'.


Fluent interface for manipulating employee records I wanted to try experimenting with fluent interface design in the C programming language. That's why I wanted to ask you, Dear Code Review users, comments on the way I have implement this simple employee record interface.

Please note that for maximum simplicity I am resorting to as simple data structures as possible.

#include <stdio.h>
#include <assert.h>
/* if this weren't just an example I'd make a linked list */
#define MAX_EMPLOYEE_COUNT 10

/* global this pointer, there is no way around this */
void* this;

typedef struct Employee {
    struct Employee* (*set_salary)(int);
    struct Employee* (*set_employee_id)(int);
    char* name;
    int salary;
    int employee_id;
} Employee;

Employee* do_set_salary(int s)
{
    ((Employee*)this)->salary = s;
    return this;
}

Employee* do_set_employee_id(int id)
{
    ((Employee*)this)->employee_id = id;
    return this;
}

typedef struct Record {
    Employee* (*add_employee)(char*);
    Employee employees[MAX_EMPLOYEE_COUNT];
    int cnt;
} Record;

Employee* do_add_employee(char *n)
{
    Record* this_record = (Record*)this;
    assert(this_record->cnt < MAX_EMPLOYEE_COUNT);
    this_record->employees[this_record->cnt].set_salary = do_set_salary;
    this_record->employees[this_record->cnt].set_employee_id =
        do_set_employee_id;
    this_record->employees[this_record->cnt].name = n;
    this = &this_record->employees[this_record->cnt];
    this_record->cnt++;
    return this;
}

void init_record(Record* r)
{
    r->cnt = 0;
    r->add_employee = do_add_employee;
}

Record* edit(Record* r)
{
    this = r;
    return r;
}

void print_record(Record* r)
{
    int i;
    for (i = 0; i < r->cnt; i++) {
        printf("%s, salary %d, id %d\n",
               r->employees[i].name,
               r->employees[i].salary,
               r->employees[i].employee_id);
    }
}

int main(void)
{
    Record record;
    init_record(&record);

    /* behold, fluent interface design! */
    edit(&record)->
        add_employee("Alice")->
        set_salary(1500)->
        set_employee_id(10201);

    edit(&record)->
        add_employee("Bob")->
        set_salary(2000)->
        set_employee_id(10202);

    print_record(&record);

    return 0;
}

I would drop the edit function and make add_employee the entry point to the API.  I've written a number of fluent APIs in C++ and found that placing the required arguments (e.g., employee name) in the API entry point invaluable.  From the usage point of view, something like:

int main(void) {
    Record db;
    init_record(&db);
    add_employee_to(&db, "Bob")
        ->having_salary(2000)
        ->with_id(10202);
    return 0;
}

The goal of the API is to create a new employee and append them, so make that the entry point.  If you can't have a valid employee record with a name, then this becomes a first-class parameter of the entry point.  Not sure about the requirements, so it may make sense for ID to be a required parameter as well.  I also changed the names to make them More Fluent™.


Filter Producs by any property I have Products table which looks like this:

I have some data in this table and I want to filter it depending on this columns (Filtering with columns type of string isn't problem). For this I have the code (in ASP.NET Core Web API Project) shown below. I know it's not a good approach. I have another table which has more foreign keys, so it will be more difficult to do this kind of work on it.

In SearchProduct action method I am getting the product object, which properties have specific values. For example, If Name="Box", then I want to get products from the product list, which Name is "Box" too.

The consumer of this service is WPF application, where I have the window, where the controls (textboxes and comboboxes{ for productType and ProductUnit}) are placed and the user will be able to search data with any criteria. For example, if he wants to search only with Name, he will write something only in Name textbox. If he wants to search product which name is "Box" and also the productType is "A", then he will write "Box" in name textbox and will choose A type in comboBox.

P.S When product is added in this table, if user doesn't write anything in the Name textbox for example, the value of Name will be "" and not null.

    [HttpPost("SearchProduct")]
    public IEnumerable<Product> SearchProduct([FromBody]Product product)
    {
        IEnumerable<Product> data;

        if(product.ProductTypeID != null && product.ProductUnitID != null)
        {
            data = _context.Products.Where(x =>
          x.Name.Contains(product.Name) &&
          x.Code.Contains(product.Code) &&
          x.Barcode.Contains(product.Barcode) &&
          x.InnerCode.Contains(product.InnerCode) &&
          (x.ProductTypeID == product.ProductTypeID &&
          x.ProductUnitID == product.ProductUnitID))

           .Include(x => x.ProductType)
           .Include(x => x.ProductUnit);
        }
        else if(product.ProductTypeID == null && product.ProductUnitID == null)
        {
            data = _context.Products.Where(x =>
        x.Name.Contains(product.Name) &&
        x.Code.Contains(product.Code) &&
        x.Barcode.Contains(product.Barcode) &&
        x.InnerCode.Contains(product.InnerCode) ||
        (x.ProductTypeID == product.ProductTypeID &&
        x.ProductUnitID == product.ProductUnitID))

         .Include(x => x.ProductType)
         .Include(x => x.ProductUnit);
        }
        else if(product.ProductTypeID != null)
        {
            data = _context.Products.Where(x =>
        x.Name.Contains(product.Name) &&
        x.Code.Contains(product.Code) &&
        x.Barcode.Contains(product.Barcode) &&
        x.InnerCode.Contains(product.InnerCode) &&
        x.ProductTypeID == product.ProductTypeID)

         .Include(x => x.ProductType)
         .Include(x => x.ProductUnit);
        }
        else
        {
            data = _context.Products.Where(x =>
        x.Name.Contains(product.Name) &&
        x.Code.Contains(product.Code) &&
        x.Barcode.Contains(product.Barcode) &&
        x.InnerCode.Contains(product.InnerCode) &&
        x.ProductUnitID == product.ProductUnitID)

         .Include(x => x.ProductType)
         .Include(x => x.ProductUnit);
        }

        return data;
    }

You code seems to contradict your text. You say:

  For example, if he wants to search only with Name, he will write something only in Name textbox. If he wants to search product which name is "Box" and also the productType is "A", then he will write "Box" in name textbox and will choose A type in comboBox.

And yet each method seems to contain multiple Where clauses.

Wouldn't it be more flexible to construct an IQueryable<T>? Something along these lines:

private IQueryable<Product> GetProductsQueryable(Product request)
{
    var queryable = _dbContext.Products.AsQueryable();

    if (!string.IsNullOrEmpty(request.Name))
    {
        queryable = queryable.Where(x => x.Name.Contains(request.Name));
    }

    if (!string.IsNullOrEmpty(request.Code))
    {
        queryable = queryable.Where(x => x.Code.Contains(request.Code));
    }

    if (!string.IsNullOrEmpty(request.Barcode))
    {
        queryable = queryable.Where(x => x.Barcode.Contains(request.Barcode));
    }

    return queryable;
}


Sum of successive powers of digits Some numbers have funny properties. For example:

$$\eqalign{89 &→ 8^1 + 9^2 &= 89 &= 89 × 1 \\ 695 &→ 6^2 + 9^3 + 5^4 &= 1390 &= 695 × 2 \\ 46288 &→ 4^3 + 6^4 + 2^5 + 8^6 + 8^7 &= 2360688 &= 46288 × 51}$$

Given a positive integer \$n\$ written in decimal as \$abcd\dotso\$, and a positive integer \$p\$, we want to find a positive integer \$k\$, if it exists, such that the sum of the digits of \$n\$ taken to the successive powers \$p\$, \$p+1\$, \$p+2\$, …, is equal to \$nk\$. In other words:

  Is there an integer \$k\$ such that \$ a^p + b^{p+1} + c^{p+2} + d^{p+3} + \dotsb = nk\$?

If it is the case we will return \$k\$, if not return \$-1\$.

Note: \$n\$, \$p\$ will always be given as strictly positive integers.

dig_pow(89, 1) should return \$1\$ since \$8^1 + 9^2 = 89 = 89 × 1\$.
dig_pow(92, 1) should return \$-1\$ since there is no \$k\$ such that \$9^1 + 2^2\$ equals \$92k\$
dig_pow(695, 2) should return \$2\$ since \$6^2 + 9^3 + 5^4 = 1390 = 695 × 2\$
dig_pow(46288, 3) should return \$51\$ since \$4^3 + 6^4 + 2^5 + 8^7 + 8^6 = 2360688 = 46288 × 51\$

import math

def dig_pow(n,p):

    '''
    Formula :

    (a ^ p + b ^ (p+1) + c ^(p+2) + d ^ (p+3) + ...) = n * k

    '''

    ''' calculate LHS '''
    digits, temp = [], n
    for i in range(len(str(n))):
        digits.append(temp%10)
        temp = temp / 10

    Tsum = 0
    for i in reversed(digits):
        Tsum = Tsum + math.pow(i,p)
        p = p + 1

    ''' Calculate RHS '''
    if Tsum % n == 0:
        return Tsum / n
    else:
        return -1

print dig_pow(46288, 3)
print dig_pow(89, 1)
print dig_pow(695, 2)
print dig_pow(92, 1)

Please feel free to suggest better/faster approaches.

The name could be more meaningful. It's usually a good idea to name a function after its result, which in this case is the multiplier \$k\$, so something like digit_power_multiplier is needed.
The docstring doesn't explain the arguments to the function, or what it returns. Write it from the point of view of a user who is trying to figure out how to call it.
Only the first string in a function definition becomes the docstring: the others are ignored. So make them comments instead.
Returning exceptional values to indicate failure (here, -1 when no \$k\$ is found) is risky: the caller might forget to check for the exceptional value. It's more reliable to raise an exception.
Python has an exponentiation operator ** so there is no need to use math.pow.
When splitting n into its digits you use len(str(n)) to find the number of digits. But if you're going to call str(n) you may as well extract the digits from the result, as suggested by vertere in another answer.
The division operator / changed its meaning in Python 3: it's now floating-point division. To make your code portable to Python 3, use the floor division operator // when you want the integer part of the quotient.
When simultaneously iterating over a sequence and an index, use enumerate. So instead of:

Tsum = 0
for i in digits:
    Tsum = Tsum + math.pow(i,p)
    p = p + 1

write:

Tsum = 0
for q, d in enumerate(digits, p):
    Tsum = Tsum + d ** q

[Thanks to Josay in comments for improving this.]
Take advantage of the sum function and write:

Tsum = sum(d ** q for q, d in enumerate(digits, p))

Putting all this together:

def digit_power_multiplier(n,p):
    """Given a positive integer n with decimal digits a, b, c, d, ..., and
    a positive integer p, return k such that:

        a ** p + b ** (p+1) + c ** (p+2) + d ** (p+3) + ... = n * k

    If there is no such k, raise ValueError. For example:

        >>> digit_power_multiplier(695, 2)
        2

    since 6**2 + 9**3 + 5**4 = 1390 = 695 * 2.

    """
    lhs = sum(int(d) ** q for q, d in enumerate(str(n), p))
    if lhs % n == 0:
        return lhs // n
    else:
        raise ValueError("no k such that {} = {}*k".format(lhs, n))


creating a hash based sorting algorithm For experimental and learning purposes. I was trying to create a sorting algorithm from a hash function that gives a value biased on alphabetical sequence of the string, it then would ideally place it in the right place from that hash.

The reasoning is that theoretically if done right this algorithm can achieve O(n) speeds or nearly so.

So here is what I have worked out in python so far:

letters = {'a':0,'b':1,'c':2,'d':3,'e':4,'f':5,'g':6,'h':7,'i':8,'j':9,
'k':10,'l':11,'m':12,'n':13,'o':14,'p':15,'q':16,'r':17,
's':18,'t':19,'u':20,'v':21,'w':22,'x':23,'y':24,'z':25,
'A':0,'B':1,'C':2,'D':3,'E':4,'F':5,'G':6,'H':7,'I':8,'J':9,
'K':10,'L':11,'M':12,'N':13,'O':14,'P':15,'Q':16,'R':17,
'S':18,'T':19,'U':20,'V':21,'W':22,'X':23,'Y':24,'Z':25}

def sortlist(listToSort):
    listLen = len(listToSort)
    newlist = []
    for i in listToSort:
        k = letters[i[0]]
        for j in i[1:]:
            k = (k*26) + letters[j]
        norm = k/pow(26,len(i)) # get a float hash that is normalized(i think thats what     it is called)

        # 2nd part
        idx = int(norm*len(newlist)) # get a general of where it should go
        if newlist: #find the right place from idx
            if norm < newlist[idx][1]:
                while norm < newlist[idx][1] and idx > 0: idx -= 1
                if norm > newlist[idx][1]: idx += 1
            else:
                while norm > newlist[idx][1] and idx < (len(newlist)-1): idx += 1
                if norm > newlist[idx][1]: idx += 1
        newlist.insert(idx,[i,norm])# put it in the right place with the "norm" to ref     later when sorting
    return newlist

I think that the 1st part is good, but the 2nd part needs help. so the Qs would be what would be the best way to do something like this or is it even possible to get O(n) time (or near that) out of this?

The testing I did with an 88,000 word list took prob about 5 min, 10,000 took about 30 sec it got a lot worse as the list count went up.

If this idea actually works out then I would recode it in C to get some real speed and optimizations.

Thank for any advice that you could give.

You have reinvented a form of radix sort.
See that page for how to get rid of the two nested O(n) loops.

It is possible to get O(n) if the maximum length of the words to be sorted is assumed to be a constant.


Modular multiplicative inverse in Ruby I implemented an algorithm to find the modular multiplicative inverse of an integer. The code works, but it is too slow and I don't know why. I compared it with an algorithm I found in Rosetta Code, which is longer but way faster.

My implementation:

def modinv1(a, c)
  raise "#{a} and #{c} are not coprime" unless a.gcd(c) == 1
  0.upto(c - 1).map { |b| (a * b) % c }.index(1)
end

Rosetta Code's implementation:

  def modinv2(a, m) # compute a^-1 mod m if possible
  raise "NO INVERSE - #{a} and #{m} not coprime" unless a.gcd(m) == 1
  return m if m == 1
  m0, inv, x0 = m, 1, 0
  while a > 1
    inv -= (a / m) * x0
    a, m = m, a % m
    inv, x0 = x0, inv
  end
  inv += m0 if inv < 0
  inv
end

Benchmark results (used benchmark-ips):

Warming up --------------------------------------
        Rosetta Code   141.248k i/100ms
                Mine   462.000  i/100ms
Calculating -------------------------------------
        Rosetta Code      2.179M (± 6.5%) i/s -     10.876M in   5.022459s
                Mine      4.667k (± 3.7%) i/s -     23.562k in   5.055259s

Comparison:
        Rosetta Code:  2179237.4 i/s
                Mine:     4667.4 i/s - 466.90x  slower

Why is mine so slow? Should I use the one I found in Rosetta Code?

As the comment says on the less-obfuscated version on Rosetta Code, their implementation is based on the Extended Euclidean Algorithm.  Your implementation works by brute force to test every element in the field, so of course it's going to be slow.


Add the first non-null item from one list to another list Given these two methods, is it defensible that one is better than the other?
In what terms? Performance? Elegance? Readability?

public static void AddFirstNotNull(List<object> onlyNotNull, List<object> source)
{
  var item = source.FirstOrDefault(v => v != null);
  if (item != null)
  {
    onlyNotNull.Add(item);
  }
}

public static void AddFirstNotNull(List<object> onlyNotNull, List<object> source)
{
  onlyNotNull.AddRange(source.Where(v => v != null).Take(1));
}

I agree with @RickDavin that the first function expresses its intent in a more obvious way.

My main concern, though, is that neither function provides feedback as to whether the operation actually resulted in any change.  The function should return a boolean.  It's easy to modify the first function to return a boolean, but not the second.


Job Scheduling Algorithm 2 I am looking for feedback to improve code readability and improve the algorithm's efficiency. I have already posted a question on code review about this. The feedback was helpful, and I did rethink the data structures.
Link to the previous question: Job Scheduling Algorithm
Info about classes:
Item is the item the machines work on.
Machine is a single machine that can be used from start time to end time.
A single task is a single item on a machine.
Job is a complete job, and can have more than one task.
The algorithm schedules the tasks, re-orders them for the specific machine, and adds to total profit is a job is finished within the schedule.

class ItemModel:
    def __init__(self, name:str, starts:int, ends:int)->None:
        self.name = name
        self.start = start
        self.ends = ends

class JobModel:
    def __init__(self, tasks_needed:int, tasks_done:int, profit:int)->None:
        self.tasks_needed = tasks_needed
        self.tasks_done = tasks_done
        self.profit = profit

class MachineModel:
    def __init__(self, starts:int, ends:int)->None:
        self.starts = starts
        self.ends = ends

class View:
    def output_profit(profit:int)->None:
        print("profit: " + str(profit), end = "\n")

    def output_sublist(sublist:[int])->None:
        for item in sublist:
            print(str(item), end = " ")
        print('\n')

def algorithm(items:[ItemModel], jobs:[JobModel], machines:[MachineModel])->(int, [ItemModel]):
    total_profit = 0
    for start in range(len(items)):
        for end in range(len(items)):
            for job_row in range(len(jobs)):
                if (time >= jobs[job_row].start_time) and (time <= jobs[job_row].end_time):
                    jobs[job_row].task_done += 1

            profit = 0
            for job_row in range(len(jobs)):
                if jobs[row].tasks_needed == jobs[row].tasks_done:
                    profit += jobs[row].profit

            yield (profit, sublist)

def main():
    items = [ItemModel("a", 0, 2), ItemModel("b", 3, 4)]
    jobs = [JobModel(0, 0, 12), JobModel(0, 0, 10)]
    machines = [MachineModel(0, 1), MachineModel(0, 7)]
    View view = View()
    for pair in algorithm(items, jobs, machines):
        view.output_profit(pair[0])
        view.output_sublist(pair[1])

if __name__ == "__main__":
    main()
 ```

'algorithm' is not correct at all and will fail at runtime. please make sure your code works before posting it. start and end are never read. time is never defined. 'row' is never defined--it is typo-d as 'job_row' the second time. use 'for job in robs', not 'for job_row in range(len(jobs)) together with 'jobs[job_row]'. I would generally recommend a linter, which should catch all these errors.

Add a docstring to "algorithm", describing what its inputs are, and what it returns. Make sure to describe what it returns in terms of MEANING, and not what the algorithm does. ("returns the best way to schedule..." and not "loops through...")
Change the name of 'algorithm' to reflect what it does (ex. 'schedule').

I personally think it's fine to put the contents of 'main' directly in the if __name__ == "__main__" block, but I've seen it both ways.

For the output_ methods, consider returning a string, and printing the string, as two steps. " ".join() may help.


Insert rows in the datagridview with a dictionary I would like to improve the function below without calling twice InserisciRigaNote.

Before I fill my dictionary. When I compare idNota with the current database id I check if it is equal or not, if different means that the dictionary is filled and I can proceed with the insertion of the row in DataGridView

I have a query that return this output:

I have to fill a DataGridView in that way

string query = ""; // Query MySQL
using (MySqlDataReader reader = db.ExecuteReader(query))
{
    if (reader.HasRows)
    {
        int idNota = 1;
        var note = new Dictionary<string, string>();
        while (reader.Read())
        {
            if (idNota != Convert.ToInt32(reader["id_nota_lavaggio"]))
            {
                InserisciRigaNote(note, idNota); // Function that insert row in the DataGridView

                idNota = Convert.ToInt32(reader["id_nota_lavaggio"]);
                note.Clear();
            }
            // Create a dictionary made by a language key and a description
            note[reader["id_lingua"].ToString()] = reader["descrizione_nota_lavaggio"].ToString();
        }
        if (note.Count > 0) // If there is something in the dictionary
        {
            InserisciRigaNote(note, idNota);
            note.Clear();
        }

    }
}

N.B.
I did not use Pivot to make the query because I need it's all completely dynamic

Your code is really confusing, not in the least because it is partly in Italian.

It does seem to me that you've coupled your UI and your back-end, and that's something to avoid. Look into developing an n-tier application so your db related code is in a back-end and all you're exposing to the UI is a collection of some sort with data objects.

And the you can bind that collection to your DataGridView, which is a much nicer solution instead of inserting one row at a time. Ideally you'd abandon WinForms, instead look into more modern technologies like WPF and apply MVVM. But if you're stuck with WinForms there are still ways to apply something like MVP.

That is of course a massive leap from the code you have here, so try refactoring one bit at the time.

Construct a class that can contain the results of your db-query, and fill a collection, so you can properly bind your data instead of inserting rows.
Beware of strings like "id_nota_lavaggio" and consider defining them as const so you can easily update them if necessary.
Twice you do Convert.ToInt32(reader["id_nota_lavaggio"]): instead, do it once and store the result in a variable.
I don't know about how well Entity Framework works with MySQL, but there are other ORMs available anyway, so when possible consider abandoning writing ADO and instead work with an ORM.
Comments should tell us why, not what. // Query MySQL is superfluous, and something like // Create a dictionary made by a language key and a description should be told by the code, not the comments (ditto // If there is something in the dictionary).


Asking for Fibonacci numbers, up till 50 I am learning Python, and this is one of the program assignments I have to do.

I need to write code to prompt user to enter Fibonacci numbers continuously until it's greater than 50. If everything correct, screen show 'Well done', otherwise 'Try again'.

While my code is working correctly, I would like to know how I should write it better.

My code:

# Init variables
t = 0
prev_2 = 1
prev_1 = 0
run = 1

# Run while loop to prompt user enter Fibonacci number
while (run):
   text = input('Enter the next Fibonacci number >')
   if (text.isdigit()):
      t = int(text)
      if t == prev_2 + prev_1:
         if t <= 50:
            prev_2 = prev_1
            prev_1 = t
         else:
            print('Well done')
            run = 0
      else:
         print('Try again')
         run = 0
   else:
      print('Try again')
      run = 0

A few minor tips on making the code more compact:

Remove unneeded variables/initializations.  In particular your t variable doesn't need to be initialized outside the loop since its value is always set within the loop before it's used.
The run variable can also be eliminated; make the loop while True and use a break to escape it when you're done.
Instead of doing text.isdigit() just do the int conversion inside a try block.
Combine your two "Try again" cases into a single block of code rather than repeating yourself.

With those changes the code looks like:

# Init previous two values of Fibonacci sequence
prev_2 = 1
prev_1 = 0

# Run while loop to prompt user enter Fibonacci number
while True:
    try:
        t = int(input('Enter the next Fibonacci number >'))
        if t != prev_2 + prev_1:
            raise ValueError('not the next Fibonacci number!')
        if t > 50:
            print('Well done')
            break
        # move to next value in the sequence
        prev_2, prev_1 = prev_1, t
    except ValueError:
        print('Try again')
        break

A larger tip on improving the code would be to separate the two distinct tasks the code is doing: computing the Fibonacci sequence up to 50, and quizzing the user on it.  Here's how I might write that:

# Init Fibonacci sequence up to first value past 50 (shfifty-five)
fib = [0, 1]
while fib[-1] <= 50:
    fib.append(fib[-2] + fib[-1])

# Prompt the user for all those values (after the initial 0, 1)
for n in fib[2:]:
    try:
        if int(input('Enter the next Fibonacci number >')) != n:
            raise ValueError('not the next Fibonacci number!')
    except ValueError:
        print('Try again')
        break
else:
    print('Well done')

Now the code is in two smaller parts that are each a lot simpler than the previous version -- the first few lines compute the Fibonacci sequence in a very straightforward way, and the rest of the code simply iterates over the sequence and checks the input against it.

In a larger project you'd probably take these two "chunks" of code and define them as functions (you'd have one function that generates the sequence and another that handles the quizzing), but even within a function it's valuable to think about how you can break a problem does into distinct simple parts (and once you've done that, the next step of refactoring it into smaller functions becomes easy).


Defining an OOP structure for a Student class in NodeJS Most of my coding experience is in C#. The way object oriented programming is laid out in C# is a bit different from NodeJS, hence this is my first NodeJS OOP for a Student class.

I'm trying to translate this C# structure of creating a class, object and methods to NodeJS:

class Student
    {
        private int _age;

        public int Age
        {
            get { return _age; }
            set { _age = value; }
        }

        private string _name;

        public string Name
        {
            get { return _name; }
            set { _name = value; }
        }

        private string _id;

        public string ID
        {
            get { return _id; }
            set { _id = value; }
        }
   }

Sample usage:

Student student = new Student();
student.Age = 12;
student.Name= "Tolani";
student.ID = "Pokemon1234";

NodeJS code:

// Constructor
   function Student(name, age, id)
    {
        // always initialize all instance properties
        this.name = name;
        this.age = age;
        this.id = id;
    }
    // Get the student Name
    Student.prototype.getStudentName = function()
    {
        return this.name;
    };

    // Gets the student Age
    Student.prototype.getStudentAge = function()
    {
        return this.age;
    };

    // Gets the student's ID
    Student.prototype.getStudentId = function()
    {
        return this.id;
    };
    // export the class
    module.exports = Student;
    var student = new Student('Tolani', 23, 'ddr1234');
    console.log('The student name is ' + student.getStudentName());

In general, can this be improved?

In modern versions of node.js (v6.0 or v4.x in strict mode) or when using an ES6 transpiler, you can use the ES6 class keyword:

class Student {
    constructor(name, age, id) {
        // always initialize all instance properties
        this.name = name;
        this.age = age;
        this.id = id;
    }
    getStudentName() {
        return this.name;
    }
    getStudentAge() {
        return this.age;
    }
    getStudentId() {
        return this.id;
    }
}

This creates the same underlying .prototype methods as your original code, but is obviously a bit cleaner syntax.  Usage of the Student constructor and methods is identical.

Note: You don't need accessor methods for these properties.  They are directly accessible as properties of the object.

If you don't want them directly accessible as properties (e.g. you only want methods to be able to access them), you can do this:

function Student(name, age, id) {
   this.getStudentName = function() {
       return name;
   }
   this.getStudentAge = function() {
       return age;
   }
   this.getStudentID = function() {
       return id;
   }
}

This "hides" the name, age and id instance variables in a constructor closure so they are only accessible to methods defined within the constructor.  This does not use the prototype for methods and may (depending upon JS implementation) consume a bit more memory per object, but does give you property privacy.


Multi-threaded utility that logs some audit trails I'm trying to write a multi-threaded utility that logs some audit trails to the DB every 30 minutes, or whenever my storage data structure (a list in this case) exceeds a certain limit.

The code works well, but having said that, my manager said there could potentially be some issues in the method LogUserUsageData(...) where one thread tries to add a UserUsage object while another thread is in the lock trying to initialize the same list.

Should I be declaring my list as volatile?
Should I be thinking about double-locking techniques?

Can anybody throw more light on this?

namespace Utilities
{
    public class UserUsageStatistics
    {
        private SynchronizedCollection<UserUsage> _statisticsList;
        private readonly Thread _persistenceThread;
        private readonly string _flag = String.Empty;

        public UserUsageStatistics()
        {
            _statisticsList = new SynchronizedCollection<UserUsage>();
            _persistenceThread = new Thread(ThreadWork);
            _persistenceThread.Start();
        }

        public void LogUserUsageData(UserUsage userUsage)
        {
            if (_statisticsList.Count >= 100) PersistToDataStore();
            _statisticsList.Add(userUsage);
        }

        void ThreadWork()
        {
            while (true)
            {
                PersistToDataStore();
                Thread.Sleep(1800000);
            }
        }

        /// <summary>
        /// Persist the data to the data store.
        /// </summary>
        void PersistToDataStore()
        {
            if (_statisticsList.Count == 0) return;
            try
            {
                SynchronizedCollection<UserUsage> tempList;
                lock (_flag)
                {
                    if (_statisticsList.Count == 0) return;

                    tempList = _statisticsList;
                    _statisticsList = new SynchronizedCollection<UserUsage>();
                }
                //TODO: Save to database
            }
            catch (Exception ex)
            {
            }
        }

    }
}

DO NOT LOCK ON EMPTY STRING

In .NET every string is stored only once in the AppDomain so if you are locking on empty string everything will stop until you release the lock.

The correct syncroot can be:

private readonly object _flag = new object();

And yes you should use double check locking. The other thing is thati would use a ConcurrentQueue instead of SynchronizedCollection.


Determining coefficient in geometric progression For an assignment at Uni we had to code a program that takes numbers as input and determines the coefficient if they form a geometric progression once you declare EOF (ctrl+z). If the user inputs a number that doesn't belong to an already established progression the program breaks, telling us there's no geometric progression.

#include <stdio.h>
#include <math.h>

int main(){
int prevNum;
int Num;
scanf_s("%i", &prevNum);
scanf_s("%i", &Num);
int r = Num / prevNum;
prevNum = Num;
while ((scanf("%i", &Num) != EOF))
{
    if (Num / prevNum != r)
    {
        printf("No progression \n");
        return 0;
    }

    else prevNum = Num;

}
    printf("Progression with r=%i \n", r);
    return 0;
}

What I'd like to ask is whether there's a better, more practical way to fulfill the same task.

In the if statement inside the while loop,
since the method returns when the condition is true,
you can drop the else.

In the condition of the while, you have pointless parentheses.

When printing stuff without variables,
I prefer puts instead of printf because it's shorter.

scanf_s doesn't compile with my compiler gcc.
I don't know what it is, but replacing with scanf seems to work fine for me.

Notice in your question that the syntax highlighter made Num have a different color from other variables.
It's because CamelCase is usually used for class names,
and camelCase for variable names.
So I'd rename this to num, or thisNum (to sort of harmonize with prevNum).

Pay attention to formatting. It makes the code more readable. For example:

You didn't indent the main body, then after the while loop you indented.
There are pointless blank lines in the while loop that just look strange.

With the above suggestions applied:

#include <stdio.h>
#include <math.h>

int main() {
    int prevNum;
    scanf("%i", &prevNum);
    int thisNum;
    scanf("%i", &thisNum);
    int r = thisNum / prevNum;
    prevNum = thisNum;
    while (scanf("%i", &thisNum) != EOF)
    {
        if (thisNum / prevNum != r)
        {
            puts("No progression");
            return 0;
        }
        prevNum = thisNum;
    }
    printf("Progression with r=%i \n", r);
    return 0;
}


Getting Facebook comments count via FQL I'm currently getting the Facebook comments count over a query request to the Facbeook api.

The problem is, there are about 15 queries on each site which makes it real slow to load (up to 6 seconds)

The next step is to get the comments count from the array object, as there's no native PHP function for that i wrote my own.

Is there any possibility to speed things up?

Is there something wrong with my code, which makes it slow, or is this up to the facebook query request?

Here's my code:

function objectToArray($object)
{
    if(!is_object($object) && !is_array($object))
    return $object;

    $array=array();
    foreach($object as $member=>$data)
    {
        $array[$member]=objectToArray($data);
    }
    return $array;
}

function count_comments($url)
{
    $fql  = "SELECT commentsbox_count FROM ";
    $fql .= "link_stat WHERE url = '".$url."'";

    $apifql='https://api.facebook.com/method/fql.query?format=json&query='.urlencode($fql);
    $json=file_get_contents($apifql);
    $obj = json_decode($json);
    $obj= objectToArray($obj);
    $obj = $obj[0]['commentsbox_count'];

    if($obj == 1)
    {
        echo '1 comment';
    }
    else if($obj == 0)
    {
        echo 'No comments yet';
    }
    else
    {
        echo $obj.' comments';
    }
}

Here I'm calling the function:

count_comments('http://example.com'.$data["id"].'');

Thanks in advance for any hints, tips and solutions!

Please use braces {} to wrap your code, even those one line if statements, makes it so much easier to read.

First off, you do know that JSON has an option to output an array right? json_decode( $json, TRUE ) returns an array. No need to create a function for it.

"there's no native PHP function for that" - If it uses the countable interface, which is any data object I've come across (XML, JSON, SQL, etc...) then you can just use count(). If you have an object that holds the comments I would just try using count() on that first before creating a new query just to get a count.

Make sure you are not calling the "same" object twice. I imagine you can just get the whole table into an array then perform array operations on it to get the desired results without having to query the server multiple times. This is probably the biggest reason your program runs so slow. Hope this helps!


Simulating coin flips and plotting a histogram I started playing around recently with F# and I find it quite elegant and succinct language.

A common problem I like to solve in every language I start to learn is the coin flip problem, with the code below being the solution to this particular problem.
What the code does is to flip a coin N times and perform M tests for each flip series. Finally, it prints in console a binomial distribution visual.

I am new to F# and I might being hasty here, I probably will learn this myself much later, but is anybody that can suggest a refactoring? Is it possible to make this even more succinct?

I solved the problem with code very similar to imperative style and I refactored my way to the following code, trying keep it as much as declarative as I could.

open System

let heads (random: Random) =
    random.Next(0, 2) = 1

let add x y = x + y

let rec calculateHeads n random =
    match n with
    | 0 -> 0
    | _ ->
        let result = calculateHeads (n - 1) random
        let addToResult v = add v result
        match heads (random) with
        | true -> addToResult 1
        | false -> addToResult 0

let rec fill frequency n m random =
    match m with
    | v when v < 0 -> frequency
    | _ ->
        let count = calculateHeads n random
        Array.set frequency count (frequency.[count] + 1)
        fill frequency n (m - 1) random

let getFrequencies n m =
    let random = new Random()
    let frequency = Array.create (n + 1) 0
    fill frequency n m random

let rec displayAsterisk length =
    match length with
    | v when v = 1 -> "*"
    | v when v < 1 -> ""
    | _ -> "*" + (displayAsterisk (length - 10))

let rec displayRecursive list =
    match list with
    | [] -> ()
    | head::tail ->
        displayRecursive tail
        printfn "%d%s -- (%d)" head (displayAsterisk (head)) head

let displayCoin (frequency:int[]) =
    displayRecursive (frequency |> Array.toList)

[<EntryPoint>]
let main argv =
    let frequency = getFrequencies 32 1000
    displayCoin frequency
    0

I had some smaller points to make on several aspects of your code but I won't mention them since the whole thing can be written more idiomatic and simple style without using any recursion or mutation.

Note the use of Seq.sumBy and Array.countBy to cut down on a lot of the work:

let random = System.Random()

let heads () = random.Next(0, 2) = 1

let calculateHeads n =
    { 1 .. n } |> Seq.sumBy (fun _ -> if heads () then 1 else 0)

let getFrequencies n m =
    let counts =
        Array.init m (fun _ -> calculateHeads n)
        |> Array.countBy id
        |> Map
    Array.init (n + 1) (fun i -> counts.TryFind i |> Option.defaultValue 0)

let displayCoin frequency =
    for c in frequency do
        printfn "%d%s -- (%d)" c (String.replicate (c / 10) "*") c

let frequency = getFrequencies 32 1000
displayCoin frequency

I also made random a module level value instead of passing it around everywhere. The downside of this is you can't test individual functions deterministically.

If you're not already doing so, you should send code to F# interactive in your IDE to get a much faster feedback cycle for trying out ideas and exploring data.


Printing bowTie pattern in efficient and pythonic way I've posted  code/library to GitHub for first time over here.

The code is pretty simple. It generates a BowTie pattern of given size and characters.

I did attempt to implement DP by calculating only half of shape matrix and getting its mirror image from matrix itself.

The code works fine. I do need help with someone analyzing to see if certain logic/lines can be done in more pythonic  and efficient way [a.k.a. Code Review].

Any help is appreciated.

Source-Code

from __future__ import print_function

class BowTie(object):
    def __init__(self, size=5, fill_value = '*', empty_value = ' '):
        self.size = size
        self.fill_value = fill_value
        self.empty_value = empty_value

    def  create_shape(self):
        ''' creates shape as per input values and returns list of list of values'''
        star = self.fill_value
        dot = self.empty_value
        m = (self.size * 2) - 1 #Get center row

        #get  top half list
        th = []
        for idx,k in enumerate(xrange(1,self.size+1)): #run through 1 - size
            row = idx + 1
            tmplst = []
            if row % 2 != 0:
                tmplst.append(i for i in xrange(1,row + 1) if i % 2 != 0)
                tmplst.append(i for i in xrange(m, m-row, -1) if i % 2 != 0)
            else:
                tmplst.append(i for i in xrange(1,row + 1) if i % 2 == 0)
                tmplst.append(i for i in xrange(m, m-row, -1) if i % 2 == 0)

            #append each row value to  top half list.
            th.append(sorted(set([j for i in tmplst for j in i])))

        #create mirror-image of top half which is our bottom half
        th = th + th[len(th) -2::-1]

        def get_list(bound = self.size, alist = []):
            ''' expects upper bound and an integer list
            returns a list of fill_values '''
            tmp_list = []
            for i in xrange(1,bound + 1):
                tmp_list.append(star if i in alist else dot)
            return tmp_list

        #create list of * and blanks or as per fill and empty value
        row_values = [get_list(bound = m, alist = i) for i in th]
        return row_values

    def print_shape(self):
        ''' print given list of list of values'''
        for i in self.create_shape():
            print(' '.join(i))

Don't use a class, there's no need.
You can remove the tmplst.append if you move the comprehensions into the th.append.

To do this you can use (i + idx) % 2 == 1 as the check. And tmplst = [xrange(1,row + 1), xrange(m, m-row, -1)].
It doesn't make sense to build a set, to then convert it to a list. Sets have \$O(1)\$ lookup where lists have \$O(n)\$.
You don't need get_list, just use a comprehension.

This can get something like:

def bow_tie(self, size=5, fill_value='*', empty_value=' '):
    star = fill_value
    dot = empty_value
    center_row = size * 2 - 1
    th = [
        {
            j
            for nums in (
                xrange(1, i + 2),
                xrange(center_row, center_row - i - 1, -1)
            )
            for j in nums
            if (j + i) % 2 == 1
        }
        for i in range(size)
    ]
    th += th[len(th) -2::-1]

    return [
        ' '.join([
            star if i in indexes else dot
            for i in range(1, size * 2)
        ])
        for indexes in th
    ]

However, this isn't that good an approach. Instead you can make a list containing the pattern. And show a 'view'. The view that you want could be:

     *
    **
   ***
  ****
 *****
******

To achieve this, you can make the following list:

'     ******'

With the following moving view:

#     *######
##    **#####
###   ***####
####  ****###
##### *****##
######******#

You can also invert the view to get the other side of the bow tie. And so using the above, we just need to think of what the pattern of the list needs to be for your bowtie.

Since it's pretty much the same as the above, but with a space in-between the dots you could use something like:

'     * * * '

And to create that, just make the first n chars space, with the following n chars flip between space and star:

face = [empty_value] * (size - 1) + [fill_value, empty_value] * (size // 2 + 1)

After this combine the above slider, and chain the top and bottom starts, and you should get:

from itertools import chain

def bow_tie(size=5, fill_value='*', empty_value=' '):
    face = [empty_value] * (size - 1) + [fill_value, empty_value] * (size // 2 + 1)
    starts = chain(range(size), reversed(range(size-1)))
    for i in starts:
        yield ' '.join(face[i:i+size][::-1][:-1] + face[i:i+size])


Solving a practical problem involving square of root of a number A student has a book containing 411 pages.
  She read a certain number of pages on the first day and created a rule to work out how many pages she had to read on each succeeding day.
  She decided that the number of pages to be read on the next day should be equal to the square of the sum of the digits of the page she ended at.
  For example, if she ended on page 36, then she should read 81 pages on the next day as this is the square of 6 + 3.
  She found that on the sixth day, the number of pages she had set herself to read took her exactly to the final page of the book.
  How many pages did she read each day?

Here's what I did:

import java.util.*;
import java.lang.*;
import java.io.*;

class ADG {
  public static void main(String[] args) throws java.lang.Exception {
    for (int firstpage = 1; firstpage <= 411; firstpage++) {
      int sum = firstpage;
      int day = 1;
      do {
        sum += squareofroot(sum);
        day++;
        if (sum == 411) {
          System.out.println(firstpage + "&" + day + "\\\\");
          break;
        }
      } while (sum <= 411);
    }
  }
  public static int squareofroot(int pnum) {
    int root = 0;
    int j = 0;
    do {
      root += pnum % 10;
      pnum = pnum / 10;
      j = j + 1;
    } while (pnum != 0);
    return (root * root);
  }

}

Any reviews?

Numbers that end up with 411 pages, not necessarily in 6 days:

$$\begin{array}{c|c}
\text{pages read $\\$on first day}&\text{days required to$\\$ read exactly 411 pages}\\\hline
13&5\\
16&4\\
29&4\\
\huge \color{red}{61}&\huge \color{red}{6}\\
65&3\\
110&5\\
114&4\\
150&3\\
186&2\\
190&3\\
241&3\\
290&2\\
\end{array}$$

You don't usually need to import java.lang, as it is built in, but maybe this is a special case. Either way, It's best to only import the classes you need. Regardless, java.io, and java.util are unused.

Did you import java.io for exceptions? In general's, it's preferable to throw a specific Exception, this alone doesn't really say much. As it is I think it's entirely unnecessary, and wonder why you're throwing any exception in the first place.

You don't need to break when you reach the sum, your while loops condition is already checking if sum reaches 411.

411 is a magic number. Looking at the code alone in the future it would serve you to instantiate the value in a final field.

In java it's conventional for variables and methods to be camelCase, so as an example, in your code firstpage should be firstPage and squareofroot should be squareOfRoot. Additionally, class names are PascalCase, so your class ADG would be Adg (Though I'd pick a better class name), doesn't hurt to be as descriptive as possible. I'd wager if you stepped back for a few months and just saw a file named ADG you'd wonder what it is. Try to keep these standards in mind going forward, they will help keep your code readable not only to yourself but anyone else you present it to.

Lastly, you don't really answer the question. The challenge asks

  How many pages did she read on each day?`

Answering that would be trivial with your code, you actually went one further and found out different ways she could have finished the book using her method.

Suggested Implementation:

import java.util.Map;
import java.util.LinkedHashMap;

public class Test {
    public static void main(String[] args) {
        final int pageTotal = 411;
        final int daysRequired = 6;
        Map<Integer, Integer> pagesReadPerDay = new LinkedHashMap<>();

        for (int firstPage = 1; firstPage <= pageTotal; firstPage++) {
            int sum = firstPage;
            int day = 1;

            while (sum <= pageTotal) {
                sum += squareSumDigit(sum);
                day++;

                if (sum == pageTotal && day == daysRequired) {
                    int pagesRead = firstPage;
                    int currentPage = pagesRead;

                    for (int onDay = 1; onDay <= daysRequired; onDay++) {
                        pagesReadPerDay.put(onDay, pagesRead);
                        pagesRead = squareSumDigit(currentPage);
                        currentPage += pagesRead;
                    }
                }
            }
        }

        for (Map.Entry<Integer, Integer> entry : pagesReadPerDay.entrySet()){
            System.out.println(
                "Day: " + entry.getKey() +
                ", Pages Read: " + entry.getValue() + "."
            );
        }
    }

    public static int squareSumDigit(int num) {
        int sum = 0;

        while (num > 0) {
            sum += num % 10;
            num /= 10;
        }

        return sum * sum;
    }
}

Output:

  Day: 1, Pages Read: 61.
  Day: 2, Pages Read: 49.
  Day: 3, Pages Read: 4.
  Day: 4, Pages Read: 36.
  Day: 5, Pages Read: 36.
  Day: 6, Pages Read: 225.


Swift API controller I've just started iOS development and learning Swift. My code below works, but I'm interested to know in ways I can improve it having no experience in iOS Swift prior to this attempt.

import UIKit
class NewsViewController: UIViewController, UITableViewDataSource, UITableViewDelegate {

var tableView: UITableView!
var titleItems: NSMutableArray = []
var descritpionItems: NSMutableArray = []

/*
class to get data from API
*/
class RemoteAPI {

    // base url
    let baseUrl = "http://api.dev/web/"
    var newsTitle: NSMutableArray = []
    var newsDescription: NSMutableArray = []

    func getData(completionHandler: ((NSArray!, NSError!) -> Void)!) -> Void {
        // get news feed url
        let url = NSURL(string: baseUrl + "news")
        // create session
        let session = NSURLSession.sharedSession()
        // set task
        let task = session.dataTaskWithURL(url!, completionHandler: {data, response, error -> Void in
            if (error != nil) {
                return completionHandler(nil, error)
            }

            var error: NSError?
            // convert data object to json
            let json = JSON(data: data)

            for var i = 0; i < json.count; ++i {
                // get news title from json object
                var title = json[i]["title"].string
                var blurb = json[i]["description"].string
                // add to dictionary
                self.newsTitle.addObject(title!)
                self.newsDescription.addObject(blurb!)
            }

            if (error != nil) {
                return completionHandler(nil, error)
            } else {
                return completionHandler([self.newsTitle,self.newsDescription], nil)
            }
        })
        task.resume()

    }
}

// get data
var api = RemoteAPI()

override func viewDidLoad() {
    super.viewDidLoad()
    // Do any additional setup after loading the view.
    self.view.frame = CGRect(x: 0, y: 50, width: 320, height: 480)
    self.tableView = UITableView(frame:self.view!.frame)
    self.tableView!.delegate = self
    self.tableView!.dataSource = self
    self.tableView!.registerClass(UITableViewCell.self, forCellReuseIdentifier: "cell")
    self.view?.addSubview(self.tableView)

    api.getData({data, error -> Void in
        if (data != nil) {
            self.titleItems = self.api.newsTitle
            self.descritpionItems = self.api.newsDescription
            self.tableView!.reloadData()
        } else {
            println("API failed :-(")
            println(error)
        }
    })
}
func tableView(tableView: UITableView, numberOfRowsInSection section: Int) -> Int{

    return self.titleItems.count
}

func tableView(tableView: UITableView, cellForRowAtIndexPath indexPath: NSIndexPath) -> UITableViewCell{

    var cell:UITableViewCell=UITableViewCell(style: UITableViewCellStyle.Subtitle, reuseIdentifier: "mycell")

    if let newsTitle = self.titleItems[indexPath.row] as? NSString {
        cell.textLabel?.text = newsTitle
    }
    if let newsDesc = self.descritpionItems[indexPath.row] as? NSString {
        cell.detailTextLabel?.text = newsDesc
    }
    return cell
}
}

There's quite a lot to address here, but I'm going to start with my absolute largest concern--one that will cause problems for anyone who uses this:

func getData(completionHandler: ((NSArray!, NSError!) -> Void)!) -> Void

There are places where it's okay to use the implicitly unwrapped optional !, but this is not one of those places by any means.

First of all, you use it on the completion handler as a whole, which means it's somehow okay for the user to call:

getData(nil)

But you never check whether or not a completion handler was passed to you.  You just assume one was and call it anyway, which means any time someone calls this method and passes nil, there will be a crash.

But the arguments the completion handler takes also should not be implicitly unwrapped options.  They should just be regular optionals: ?.  This is especially true here as we are explicitly passing nil into the completion handler in some cases.  Implicitly unwrapped optionals are for very special cases.  You need to use the regular optional which tells any user "YOU MUST CHECK THIS FOR NIL!".  Implicitly unwrapped optionals say "This object is never nil so never bother checking it!  It's only an optional because of some extraordinarily special scenario that's handle entirely by the internal class in initialization."

If I start using your RemoteAPI class, I'd never check the objects in the completion handler for nil (until I got a crash).

Unless you're Apple making Objective-C code in their massive libraries work correctly with Swift (and even here, Apple is only using ! as a placeholder and intends to replace these all with either non-optionals or regular optionals) OR you're writing something where a property can be nil in the init sometimes, then you don't need to be using the implicitly unwrapped optional!  It looks like you've got a handleful of implicitly unwrapped optionals that need to be removed.


Run arbitrary INSERT INTO SQL Query using parameters While DoCmd.RunSQL is all well and good for simple code, the number of times I've run into problems with unescaped apostrophes and the like was starting to nark a bit. With that in mind I crafted the below, a first attempt at a SQL wrapper for VBA which constructs and executes an arbitrary INSERT INTO query using parameters.

'Fields should be a comma-delimited string
Public Sub InsertInto(TableName As String, Fields As String, Values As Variant)
    Dim qdf As DAO.QueryDef
    Dim strParams As String
    Dim i As Long
    Dim strSQL As String

On Error GoTo Err_InsertInto
    'Assigns strParams value in format "[p1],[p2],[p3],...,[pN]"
    For i = 0 To UBound(Values)
        strParams = strParams & "[p" & i & "],"
    Next i
    strParams = Left(strParams, Len(strParams) - 1) 'Strip trailing comma

    'Construct SQL
    strSQL = "INSERT INTO " & TableName & "(" & Fields & ") " & _
             "VALUES (" & strParams & ");"
    Set qdf = CurrentDb.CreateQueryDef("", strSQL)

    'Give values to parameters
    For i = 0 To UBound(Values)
        qdf.Parameters("[p" & i & "]") = Values(i)
    Next i

    qdf.Execute

Exit_InsertInto:
    Set qdf = Nothing
    Exit Sub

Err_InsertInto:
    Select Case Err.Number
        Case 13   'Type mismatch i.e. Values is not an array
            Values = Array(Values)
            Resume
        Case Else 'Unhandled Error
            Set qdf = Nothing 'Release QueryDef object

            'Error is unhandled here, force it to 'bubble up' to calling code
            Dim iErrNum As Long, strErrDesc As String
            iErrNum = Err.Number
            strErrDesc = Err.Description
            On Error GoTo 0
            Err.Raise iErrNum, "SQLOps.InsertInto", strErrDesc
    End Select
End Sub

Likely to be called in a manner similar to (this example was used as a successful test case):

InsertInto "tblErrorLog", _
"ErrorNum,Description,User,When,Origin", _
Array(10,"Error's Description",Environ("USERNAME"),Now(),"A Ghost")

So far it seems to be working fine and does indeed alleviate the issues with unescaped apostrophes but I'm wondering whether this can be improved so I could use it as a sort of template for crafting similar functions for other SQL operations e.g. UPDATE SET / SELECT FROM.

Can this code be improved and are there any unforeseen bugs I should watch out for?

This code can't handle fields with spaces in them unless you specifically pass them pre-wrapped in square brackets. Things such as My Field would need to go into your parameter as "field1, [My odd field], field3". So, a definite area for improvement would be to handle that corner case.

I also don't really care for the comma delimited string parameter. I feel that it would be much cleaner and nicer to pass in a string array representing the field names. So, something along the lines of this psuedo code might be a bit cleaner.

Dim fields As Variant
fields = Array("field1","My odd field","field3")

InsertInto "Table1", fields, values)

' ...

Public Sub InsertInto(...)
    AddBracketsToStrings fields

    '...

End Sub

The next thing I notice is these comments:

'Assigns strParams value in format "[p1],[p2],[p3],...,[pN]"
...
'Construct SQL
...
'Give values to parameters
...

The comments are nice, and add a bit of clarity to the code, but these kinds of comments indicate an opportunity to extract subroutines/functions. Each of these comments could be replaced with a well named function.

ConvertValuesToParameters values
Set qdf = CurrentDb.CreateQueryDef("", ConstructSQL(TableName, Fields, strParams))
SetParameterValues qdf, values

Which brings me to the SQL Injection issue. It's never a good idea to concatenate SQL strings together. It's much better to use parameterized queries. This may require that you switch to using ADODB instead of DAO. I'm not sure. I don't know if DAO supports parameterized queries.

Case 13   'Type mismatch i.e. Values is not an array

That's a good comment, but a constant value would be better.

Const TypeMismatchError As Long = 13

Select Case Err.Number
    Case TypeMisMatchError
        ' ...


Checking if a number is a power of 2 without loops I made a short program which checks if a number is a power of 2 without using any loops.
The idea: A number which is a power of 2 must have only one bit "1" ( ex: 8= 1000, 4=100 and so on).
Suppose we have a power of 2:nr = 10...000 (in binary), if we subtract 1 we will get something like this:nr-1= 01...111. Now, if we do nr&(nr-1) we should always get 0 if the nr is a power of 2 and some random number if it isn't. What other solutions are there for this problem?

#include <stdio.h>
#include <stdlib.h>

int main()
{
    int nr;
    scanf("%d",&nr);
    if((nr&(nr-1))==0)
    {
        printf("\n%d is a power of 2",nr);
    }
    else
    {
         printf("\n%d is not a power of 2",nr);
    }
    return 0;
}

Use only necessary #includes. The <stdlib.h> is not needed here.
Give your operators some breathing space. ((nr&(nr-1))==0) is next to unreadable.
Separate logic from presentation:

int is_power_of_two(int nr)
{
    return nr & (nr - 1) == 0;
}

is much more reusable.
Care about corner cases. Your code claims that 0 is a power of two (which it is not).


3 textboxes with multiple value combinations to one output I would like if someone who has more experience in JavaScript reviewed this. I went through and tried to minimize this as much as possible, but I still feel this could be smaller and more compact.

I am really just looking to see if my way of making a low-level solution in JavaScript needs to be a bit more polished.

function sum() {
    var b1 = document.getElementById('Text1').value;
    var b11 = parseInt(b1);
    var b2 = document.getElementById('Text2').value;
    var b22 = parseInt(b2);
    var b3 = document.getElementById('Text3').value;
    var b33 = parseInt(b3);

    var a = b11 || b22 || b33;
    var b = (b11 + b22) || (b11 + b33) || (b22 + b11) || (b22 + b33) || (b33 + b11) || (b33 + b22);
    var c = b11 + b22 + b33;

    if (a > 0) {
        document.getElementById('result').innerHTML = a;
    }
        else if (a < 1) {
            document.getElementById('result').innerHTML = "";
        }
    if (b > 0) {
        document.getElementById('result').innerHTML = b;
    }
    if (c > 0) {
        document.getElementById('result').innerHTML = c;
    }
}

CodePen Demo

If I understand correctly, you want to add up all the fields, and print the result. If this is the case, you are complicating things quite a lot. If you think of the problem in terms transformations it will be easier to reason about. You have a list of numbers that you want to add up, that's the problem at hand; the boolean logic expressions are not necessary, because zero doesn't add anything (it's an identity element), and addition is commutative. To prevent non-numbers from being entered, you'd check for NaN, and return identity (zero) if it is not a number, effectively doing nothing:

function sum() {
  var result = document.getElementById('result')
  var ids = ['Text1','Text2','Text3']
  result.textContent = ids
    .map(function(id) {
      // + casts a number from a string
      var n = +document.getElementById(id).value
      return isNaN(n) ? 0 : n // identity
    })
    // add up all numbers
    .reduce(function(x, y){return x + y})
}

Ideally you'd also cache the elements to not query them each time the function runs, but I doubt it would make a big difference in this case.

Demo: http://codepen.io/anon/pen/JIwHx


Silex - Guzzle call to external API I'm at the point where I am happy with the functionality of my code but I dislike how its written. It seems like I can be more modular with it but I can't seem to figure out how to improve on it without changing to much. I understand that I can probably merge both getMed Methods but they are end points in a URL which I rather keep separate. Any assistance will be greatly appreciated

private function getSIS($url, $session, Application $app)
{
    $response = "You don't have permission to access this!";

    if($app['security']->isGranted('ROLE_SUPER_ADMIN'))
    {
        //['fomservices']['sis'] is hard coded in my config dev/prod file.
        $client = new Client(['base_uri' => $app['fomservices']['sis'],]);
        //for testing
        $token = 123456;

        try {
            if (!empty($session)) {
                $response = $client->request('GET', $url.'/'.$session, [
                    'headers' => [
                        'token' => $token
                    ]
                ]);
            } else {
                $response = $client->request('GET', $url, [
                    'headers' => [
                        'token' => $token
                    ]
                ]);
            }
            return $response;
        } catch (ClientException $e) {
            $response = $e->getResponse();
            $responseBodyAsString = $response->getBody()->getContents();
            return new Response($responseBodyAsString, $response->getStatusCode());
        } catch (ServerException $e) {
            $response = $e->getResponse();
            $responseBodyAsString = $response->getBody()->getContents();
            return new Response($responseBodyAsString, $response->getStatusCode());
        } catch (BadResponseException $e){
            $response = $e->getResponse();
            $responseBodyAsString = $response->getBody()->getContents();
            return new Response($responseBodyAsString, $response->getStatusCode());
        }
    }

    return $response;
}

public function getMedAction(Application $app)
{
    $response = $this->getSIS('med', '', $app);
    if($response instanceof \GuzzleHttp\Psr7\Response)
        return $response->getBody();
    return new Response ($response);
}

public function getMedSessionAction($session, Application $app)
{
    $response = $this->getSIS('med', $session, $app);
    if($response instanceof \GuzzleHttp\Psr7\Response)
        return $response->getBody();
    return new Response ($response);
}

I was able to improve on my previous code by making sure all the return statements were returning the same type of response (String / StatusCode). This allowed the 20 methods calling this one must smaller since I didn't have to check what response contain.

private function getSIS($url, $session, Application $app)
{
    if($app['security']->isGranted('ROLE_SUPER_ADMIN'))
    {
        $client = new Client(['base_uri' => $app['fomservices']['sis']]);
        $token = 234561;

        // Calling the external API which multiple errors could occur.
        try {
            if (!empty($session)) {
                $response = $client->request('GET', $url.'/'.$session, [
                    'headers' => [
                        'token' => $token
                    ]
                ]);
            } else {
                $response = $client->request('GET', $url, [
                    'headers' => [
                        'token' => $token
                    ]
                ]);
            }
            return new Response($response->getBody(), 200);
        } catch (ClientException $e) {
            $response = $e->getResponse();
            return new Response($response->getBody()->getContents(), $response->getStatusCode());
        } catch (ServerException $e) {
            $response = $e->getResponse();
            return new Response($response->getBody()->getContents(), $response->getStatusCode());
        } catch (BadResponseException $e){
            $response = $e->getResponse();
            return new Response($response->getBody()->getContents(), $response->getStatusCode());
        }
    }

    return new Response("You don't have permission to access this!", 401);
}

public function getMedAction(Application $app)
{
    return $this->getSIS('med', '', $app);
}

public function getMedSessionAction($session, Application $app)
{
    return $this->getSIS('med', $session, $app);
}


Sum Square Difference, which way is more Pythonic? I'm using python 3 and I am creating an algorithm to find the sum square difference for the first 100 (or 1 to x) natural numbers. This happens to be project euler problem 6 if anyone is wondering.

I've written it two ways and I am looking for criticism on my writing style to further guide me in how I code algorithms. I am aware there is probably a better "optimized" solution in terms of big(o) but my math skills just haven't reached there yet.

Algorithm 1

def sum_square_difference(max_range):
    #Finds the sum square difference for the first x(max range) natural numbers
    numbers = range(1,max_range+1)
    sum_squares = sum([x**2 for x in numbers])
    square_sum = sum(numbers) ** 2
    return square_sum - sum_squares

I find this algorithm to be the most readable, but something tells me it may be more verbose in terms of lines of code than necessary so I wrote the following algorithm.

Algorithm 2

def sum_square_difference2(max_range):
    numbers = range(1,max_range+1)
    return (sum(numbers) ** 2) - (sum([x**2 for x in numbers]))

This one seems much cleaner, but I find myself struggling more to understand and read what is going on, especially when considering the perspective of an outside observer.

I appreciate any insight.

I would do something like:

def sum_of_squares(n):
    return sum(i ** 2 for i in range(1, n+1))

def square_of_sum(n):
    return sum(range(1, n+1)) ** 2

def sum_square_difference(n):
    return sum_of_squares(n) - square_of_sum(n)

Notice the use of a generator expression in sum, where I have omitted the square brackets.


Getting the KML color code from colordialog box more efficiently Kml uses an 8 digit HEX color format. The traditional Hex format for red looks like #FF0000. In Kml, red would look like this FFFF0000. The first 2 digits are for opacity (alpha). Color format is in AABBGGRR. I was looking for a way to set the color as well as the opacity and return it in a string to be placed in an attribute of a KML.

string color
string polyColor;
int opacity;
decimal percentOpacity;
string opacityString;

//This allows the user to set the color with a colorDialog adding the chosen color to a string in HEX (without opacity (BBGGRR))
private void btnColor_Click(object sender, EventArgs e)
{
    if (colorDialog1.ShowDialog() == DialogResult.OK)
    {
        btnColor.BackColor = colorDialog1.Color;
        Color clr = colorDialog1.Color;
        color = String.Format("{0:X2}{1:X2}{2:X2}", clr.B, clr.G, clr.R); //Color returned in Hex format
    }
}

//This method takes the Opacity (0% - 100%) set by a textbox and gets the HEX value. Then adds Opacity to Color and adds it to a string.
private void PolyColor()
{
    percentOpacity = ((Convert.ToDecimal(txtOpacity.Text) / 100) * 255);
    percentOpacity = Math.Floor(percentOpacity);  //rounds down
    opacity = Convert.ToInt32(percentOpacity);
    opacityString = opacity.ToString("x");
    polyColor = opacityString + color;
}

In the System.Drawing namespace lives the ColorTranslator class which has an ToHtml() method which can be used here. A good way would be to create extension methods for this task. The first extension method will be named ToKMLColor and will expect a String parameter.

By using dec.TryParse() and Math.Min() we assure that the given parameter is a valid representation of a decimal/number with a maximum of 100. If the TryParse won't succeed, we assume that the opacity will be 0.
After we have got the computed (/100*255) opacity value we pass it

public static string ToKMLColor(this Color color, String percentageOpacity)
{
    decimal opacity = 0;
    decimal.TryParse(percentageOpacity, out opacity);

    opacity = Math.Min(opacity, 100);
    opacity = Math.Floor(opacity / 100 * 255);

    return color.ToKMLColor(opacity);
}

to the overloaded ToKMLColor() method, which takes a decimal parameter. After converting the decimal to an int we pass it

public static string ToKMLColor(this Color color, decimal opacity)
{
    return color.ToKMLColor(Convert.ToInt32(opacity));
}

to the overloaded ToKMLColor() method, which takes an integer parameter. Here we call the ToString() method with the X2 format to get the hex representation of the integer parameter and concat the return value of the ToHex() by using the Substring() method to remove the leading #.

public static string ToKMLColor(this Color color, int opacity)
{
    return opacity.ToString("X2") + color.ToHex().Substring(1);
}
public static String ToHex(this Color color)
{
    return System.Drawing.ColorTranslator.ToHtml(Color.FromArgb(0, color));
}

Now we can call

polyColor = colorDialog1.Color.ToKMLColor(txtOpacity.Text);