Display all Prime Factors from user input The following program is written in python. Is there a more efficient way to write this program? Suggestions and criticism are welcome. You can also comment about my variable naming practices and code readability.

# int ----> list
# returns factor list of a num
def factor(num):
    factors = []
    for i in range(num):
        if not (i == 0 or i == 1):
            if num % i == 0:
                factors.append(i)
    return factors

# int ----> bool
# returns true if num is prime
def prime(num):
    for i in range(num):
        if not(i == 0 or i == 1):
            if num % i == 0:
                return False
    return True

# main
# prints the factors of a number which are prime
def primeFactorization():
    num = int(input("Enter the number: "))
    factors = factor(num)
    prime_factors = []

    for item in factors:
        if prime(item):
            prime_factors.append(item)
    print(prime_factors)

primeFactorization()

For both of your functions prime and factor you have something like:

for i in range(num):
    if not (i == 0 or i == 1):
        if num % i == 0:
            factors.append(i)

when you could shorten by using range(start,stop):

for i in range(2,num):
    if num % i == 0:
        factors.append(i)


LeetCode 906: Super Palindromes I'm posting my code for a LeetCode problem. If you'd like to review, please do so. Thank you for your time!
Problem

Let's say a positive integer is a superpalindrome if it is a
palindrome, and it is also the square of a palindrome.
Now, given two positive integers L and R (represented as strings),
return the number of superpalindromes in the inclusive range [L, R].
Example 1:

Input: L = "4", R = "1000"
Output: 4
Explanation: 4, 9, 121, and 484 are superpalindromes.
Note that 676 is not a superpalindrome: 26 * 26 = 676, but 26 is not a palindrome.

Note:

\$1 <= len(L) <= 18\$
\$1 <= len(R) <= 18\$
L and R are strings representing integers in the range [1, 10^18).
int(L) <= int(R)

Inputs
"4"
"1000"

"10"
"99999199999"

"1"
"999999999999999999"

Outputs
4
23
70

Code
#include <cstdint>
#include <cmath>
#include <string>
#include <math.h>
#include <queue>
#include <utility>

struct Solution {
    static std::int_fast32_t superpalindromesInRange(const std::string L, const std::string R) {
        const long double lo_bound = sqrtl(stol(L));
        const long double hi_bound = sqrtl(stol(R));
        std::int_fast32_t superpalindromes = lo_bound <= 3 && 3 <= hi_bound;
        std::queue<std::pair<long, std::int_fast32_t>> queue;
        queue.push({1, 1});
        queue.push({2, 1});

        while (true) {
            const auto curr = queue.front();
            const long num = curr.first;
            const std::int_fast32_t length = curr.second;
            queue.pop();

            if (num > hi_bound) {
                break;
            }

            long W = powl(10, -~length / 2);

            if (num >= lo_bound) {
                superpalindromes += is_palindrome(num * num);
            }

            const long right = num % W;
            const long left = num - (length & 1 ? num % (W / 10) : right);

            if (length & 1) {
                queue.push({10 * left + right, -~length});

            } else {
                for (std::int_fast8_t d = 0; d < 3; ++d) {
                    queue.push({10 * left + d * W + right, -~length});
                }
            }
        }

        return superpalindromes;
    }

private:
    static bool is_palindrome(const long num) {
        if (!num) {
            return true;
        }

        if (!num % 10) {
            return false;
        }

        long left = num;
        long right = 0;

        while (left >= right) {
            if (left == right || left / 10 == right) {
                return true;
            }

            right = 10 * right + (left % 10);
            left /= 10;
        }

        return false;
    }

};

References

Problem

Discuss

Solution

The code wastes quite a few cycles rejecting palindromes less than lo_bound. It is not hard to find the smallest palindrome above lo_bound, and start from there.
If you are not comfortable constructing such palindrome, consider lifting the lead-in into the separate loop:
    long num = 1;
    while (num < lo_bound) {
        num = make_next_palindrome(queue);
    }

The entire business around queue is very non-obvious, and it takes a great mental effort to realize that it iterates palindromes. I recommend to factor this logic out into a class of its own, along the lines of
class palindrome_iterator {
    std::queue<...> queue;
    // length, W, etc as necessary
public:
    palindrome_iterator(long start_num);
    long next();
};

This way the main loop is streamlined into
    palindrome_iterator p_i(lo_bound);
    for (long num = p_i.next(); num < hi_bound; num = p_i.next()) {
        superpalindromes += is_palindrome(num * num);
    }

An additional (and possibly more important) benefit of such refactoring is that it enables unit testing of palindrome generation logic (which really sreams to be unit tested).

I strongly advise against -~length trick. length + 1 is much more clear, and for sure not slower.


Simple function to create Tokens from user input I am new to programming in Rust and as an exercise I was making a simple Lottery checker program.
It has a simple menu, the user enters a number for the corresponding option such as 'Add a line', 'Check if I am a winner' etc.

So all the user needs to do is enter either a number or a 'y' or 'n'.
So I created a simple Token to see if they have either entered a number or a char and return a token.

This works fine. But as I am on my own I was looking for feedback so that I can improve my skills.

use std::io::{self, Write};

#[derive(Debug, PartialEq)]
enum Token {
    Number(i32),
    Decision(char),
    // Error(String),
}

fn get_number(val: &str) -> Option<Token> {
    if let Some(x) = val.split_whitespace().next() {
        let y: i32 = match x.parse() {
            Ok(val) => val,
            Err(_error) => return None,
        };
        Some(Token::Number(y))
    } else {
        None
    }
}

fn get_char(val: &str) -> Option<Token> {
    if let Some(x) = val.split_whitespace().next() {
        let y: char = match x.parse() {
            Ok(val) => val,
            Err(_error) => return None,
        };
        Some(Token::Decision(y))
    } else {
        None
    }
}

fn get_input(msg: &str, input: &str) -> Option<Token> {
    print!("{}\n{}", msg, ">> ");
    io::stdout().flush().unwrap();

    let buffer = input;
    //let mut buffer = String::new();
    // while buffer.is_empty() {
    // io::stdin()
    // .read_line(&mut buffer)
    // .expect("GET_INPUT: error reading line.");

    //  buffer = buffer
    // .trim()
    // .parse()
    //  .expect("GET_INPUT: error parsing buffer");

    // if buffer.is_empty() {
    //     println!("You didn't enter anything.\n{}", &msg)
    // };
    // }

    if let Some(token) = get_number(&buffer) {
        if let Token::Number(val) = token {
            return Some(Token::Number(val));
        }
    } else if let Some(token) = get_char(&buffer) {
        if let Token::Decision(val) = token {
            return Some(Token::Decision(val));
        }
    }
    None
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn get_input_test() {
        let chars1 = "@  n 23";
        let chars2 = "34 n 23";
        let chars3 = "$ ££$^\"3234y n 23";
        let chars4 = "";
        let chars5 = "      ";

        assert_eq!(Some(Token::Decision('@')), get_input("", &chars1));
        assert_eq!(Some(Token::Number(34)), get_input("", &chars2));
        assert_eq!(Some(Token::Decision('$')), get_input("", &chars3));
        assert_eq!(None, get_input("", &chars4));
        assert_eq!(None, get_input("", &chars5));
    }

}

To get some more fine grain control I understand I will need to use an Iterator, is this correct?

It's not clear whether you want to support only y, n and numbers as your question states, or any characters besides y & n as your code implies :)

Basically you can divide the problem to two sub problems:

Split the input string to words I'm using the split methods from std but if you have more complex requirements, you can use the regex crate
Map the words to tokens. A simple equality check (==) or match is enough. There is no need to parse() everything.

As you've not specified what's allowed as a delimiter, I'll assume that it's whitespace.

#[derive(Debug)]
enum InputToken {
    Number(i64),
    Boolean(bool),
    Error(String),
}

fn main() {
    let input = "1 -2 +4 y n & abc";
    let tokens = input.split_whitespace();

    for x in tokens {
        println!("{:?}", parse_token(x));
    }
}

fn parse_token(input: &str) -> InputToken {
    match input {
        "y" => InputToken::Boolean(true),
        "n" => InputToken::Boolean(false),
        token => match token.parse::<i64>() {
            Ok(num) => InputToken::Number(num),
            Err(e) => InputToken::Error(format!("Invalid input token [{}]: {:?}", token, e))
    }
}

}


Reversing substrings in parentheses Problem:
The task is to reverse the strings contained in each pair of matching parentheses, starting from the innermost pair. The results string should not contain any parentheses.

Example:

>>> s = "a(bc)de"
>>> reverseParentheses(s)
"acbde"
>>> s = "The ((quick (brown) (fox) jumps over the lazy) dog)"
>>> reverseParentheses(s)
"The god quick nworb xof jumps over the lazy"

To achieve this task I have written the following piece of code:

def reverseParentheses(s):
 i = 0
 brackarray = []
 recursivearray = []
 a = []
 numberofbrack = -1
 alteredstring = s

 chars = "()"
 for char in chars:
  alteredstring = alteredstring.replace(char, "")

 while i < len(s):

  if s[i] == '(':
   j = i
   numberofbrack = numberofbrack + 1
   j = j - numberofbrack
   brackarray.append(j)

  if s[i] == ')':
   m = i
   numberofbrack = numberofbrack + 1
   m = m- numberofbrack
   n = brackarray.pop()
   recursivearray.append((n,m))

  i = i+1

 for item in recursivearray:
  lenas = len(alteredstring)
  z = recursive(item[0], item[1], alteredstring)
  alteredstring1 =  alteredstring[0:item[0]]
  alteredstring2 = alteredstring[item[1]:lenas]
  alteredstring =  alteredstring1+z+alteredstring2

 return alteredstring

def recursive(start, end, string):
  y = string[start:end]
  string = y[::-1]
  return string

Could somebody please review above code and provide suggestions and improvements?

Please follow the official PEP 8 style guide.  In particular, indentation matters a lot in Python!  One space per level of indentation is much too stingy for readability; four spaces is standard.  Also, using lowercase_with_underscores for function and variable names is the norm.

For this kind of function, I would strongly recommend writing a docstring with doctests.  You could easily test your code by running python -mdoctest reverse.py.

recursive is a poorly named function: it's not recursive, and the name tells you nothing about what it does.  I'd call it extract_and_reverse_substring.

You have too many variables, which makes the code confusing.  In particular, a is never used!  Furthermore, a lot of the complication arises from your decision to strip out all of the parentheses at the beginning, to form alteredstring.  If you keep the parentheses, and strip them out afterwards, then you wouldn't need alteredstring, numberofbrack, and recursivearray, because that information would still be preserved in the string itself.

Python strings are immutable.  When doing many repositioning operations on the characters in a string, consider using a list instead, for efficiency.  That would allow you to replace individual characters without allocating new strings.

Suggested solution

def reverse_parentheses(s):
    """
    Reverse the strings contained in each pair of matching parentheses,
    starting from the innermost pair. The results string should not contain
    any parentheses.

    >>> reverse_parentheses('a(bc)de')
    'acbde'

    >>> reverse_parentheses(
    ...     'The ((quick (brown) (fox) jumps over the lazy) dog)'
    ... )
    'The god quick nworb xof jumps over the lazy'
    """
    chars = list(s)
    open_bracket_indexes = []
    for i, c in enumerate(chars):
        if c == '(':
            open_bracket_indexes.append(i)
        elif c == ')':
            j = open_bracket_indexes.pop()
            chars[j:i] = chars[i:j:-1]
    if open_bracket_indexes:
        raise ArgumentError('Unclosed parenthesis')
    return ''.join(c for c in chars if c not in '()')


Clean up this Ruby code under DRY principle I've got the following code, which works, but seems repetitive. Under DRY principle, how can I clean this up?

puts "Blurt out a word"
word = gets.chomp
word_list = []
word_list.push word

until word == ""
  word = gets.chomp
  word_list.push word
end

puts word_list.sort

puts "Blurt out a word"
word_list = []
while word_list.last != ""
  word_list << gets.chomp
end
puts word_list.sort

If you prefer the until:

puts "Blurt out a word"
word_list = []
until word_list.last == ""
  word_list << gets.chomp
end
puts word_list.sort

You can also do it shorter:

puts "Blurt out a word"
word_list = []
word_list << gets.chomp until word_list.last == ""
puts word_list.sort

Remark: The last (empty) line is also returned. To skip the empty line you may use:

puts word_list[0..-2].sort


Sorting words by frequency I'm doing a simple task in Rust after reading the Rust book:

read a text file
split it at whitespace
sanitize words by removing non-letter characters
sort the words by their frequency of occurrence

Here's what I've got, which works, but I'd like some advice:

are there any unnecessary implicit copies that could be replaced by moves?
are there nice ways to avoid let mut?

use lazy_static::lazy_static;
use regex::Regex;
use std::collections::HashMap;

lazy_static! {
    static ref NON_LETTER_RE: Regex = Regex::new("[^a-zA-Z]").unwrap();
}

fn sanitize(s: &str) -> String {
    NON_LETTER_RE.replace_all(s, "").into_owned().to_lowercase()
}

fn main() {
    let words = std::fs::read_to_string("data.txt").unwrap();
    let words = words.split_whitespace().collect::<Vec<_>>();

    let mut freqs = HashMap::new();
    for word in words {
        *freqs.entry(sanitize(word)).or_insert(0) += 1;
    }

    let mut freqs = freqs.into_iter().collect::<Vec<_>>();
    freqs.sort_by(|(_, a), (_, b)| b.cmp(a));

    for (a, b) in freqs {
        println!("{}: {}", a, b);
    }
}

This is a very solid attempt!

Really, the only negative comment I have is that you collect words into a Vec and then immediately iterate over it. Instead, you can just use the iterator directly.

Regarding avoiding let mut, there's nothing specific to your code I can think of, but there are a few general methods to avoid it.

The first is to make a block and return it from that:

let v = {
    let mut v = vec![2, 3, 1];
    v.sort();
    v
}

The second is to rebind as immutable.

let mut v = vec![2, 3, 1];
v.sort();
let v = v;

The third is to use a crate like tap, which provides a trait implemented by everything which provides a tap method.

let v = vec![2, 3, 1].tap(|mut v| v.sort());

Next, I have some suggestions for alternatives and personal preferences.

Rather than splitting on whitespace then sanitizing words, you could use regex's match function and select all continuous sequences of letters. This has slightly different semantics, since it would separate e.g. "can't" into two words. However, you could instead also allow ' and - in a word. By instead scanning by regex with find_all, you can specify exactly what constitutes a word.

Instead of or_insert(0), you can use or_default(). Since the default for numbers is 0, This does the same exact thing, but is a bit cleaner.

When collecting, I prefer to annotate the type of the variable (let x: T) rather than using turbofish notation (::<T>), I believe it looks cleaner.

The way you're sorting is perfectly fine, but I prefer using sort_by_key when possible (and reasonable). In this case, you can just wrap the key in std::cmp::Reverse, a helpful struct that reverses the ord instance on it's contents.

In general, I don't see any unneeded clones/allocations. Since the hashmap is so temporary, I would typically suggest making it's key a &str, but you can't do this since you have to make your keys lowercase. However, there's a clever way to get around this. You can wrap your &strs in a helper struct that does case insensitive hash and eq. Luckily, there's a crate for this called unicase, with structs for unicode-case-insensitive and ascii-case-insensitive String/&str. By doing this, you eliminate the allocation of a String for each word. Note that this will make your words have the case of the first time they occur, so you may want to lowercase them before printing them.

All together, here's how I might write this program.

use std::cmp::Reverse;
use std::collections::HashMap;

use unicase::Ascii;
use lazy_static::lazy_static;
use regex::Regex;

lazy_static! {
    static ref WORD: Regex = Regex::new("[-'a-zA-Z]+").unwrap();
}

fn main() {
    let data = std::fs::read_to_string("data.txt").unwrap();
    let words = WORD.find_iter(&data);

    let mut freqs: HashMap<Ascii<&str>, usize> = HashMap::new();
    for word in words {
        *freqs.entry(Ascii::new(word.as_str())).or_default() += 1;
    }

    let mut freqs: Vec<_> = freqs.into_iter().collect();
    freqs.sort_by_key(|&(_, freq)| Reverse(freq));

    for (word, freq) in freqs {
        println!("{}: {}", word, freq);
    }
}


Refining/simplifying jquery for hiding and showing fieldsets I'm starting to figure out this jquery stuff, but as you can see there's a long ways to go. How would I approach refining and simplifying this snippet. Its purpose is to show a fieldset[data-content] with inputs inside when you click a "show" link, and to hide said fieldset and remove all "required" attributes when you click the "Hide" link.

I'm using data-* for all functionality and .class for styling. Hence not using classes/ID's for these items.

JQuery

$(function() {
  $('[data-content]').hide(); // hide content initially
  $('[data-show]').click(function() {
    $('[data-content]').fadeIn('slow'); //fade in/show content
    $(this).addClass('active'); //add .active to "show" link
    $('[data-hide]').removeClass('active'); // remove .active from "hide" link
    $("[data-content] :input").prop('required',true); // add required if :visible
  return false;
  });
  $('[data-hide]').click(function() {
    $('[data-content]').fadeOut('fast'); //fade out/hide content
    $(this).addClass('active'); //add active class to "hide" link
    $('[data-show]').removeClass('active'); //remove active class from "show" link
    $("[data-content] :input").prop('required',null); // remove "required" if :hidden
  return false;
  });
});

HTML example

Show
Hide

<fieldset data-content>
  <input type="text" placeholder="Yada">
</fieldset>
<fieldset>
  <input type="text" placeholder="yada2" required>
</fieldset>

Store selections in variables; don't call $(...) again and again for elements you've already selected
You start out by hiding the fieldset, but it'd be simpler to just hide it with CSS, so it starts out hidden by default. Conversely, if it's visible by default, then add the required attributes in the html, so it actually matches the behavior you want for a visible fieldset.
You're using data-* attributes, but not super effectively (see below).

The way you're using the data-* attributes is basically the same way you'd use a class or ID; as a key to help you find stuff. So you might as well use classes and IDs then, if you're not going to use the data-* attributes to store a value in addition to the key. But for cases like this, where you want a link to refer to an element, I suggest the following:

Foo

...

The link is completely valid syntactically and semantically: It references an element.

Without JS intervening, it'll just scroll the page to element, but with JS, we can intercept the click and do the showing/hiding. Of course, there's no way to distinguish between hide and show links, but I think that should be a toggle-style action anyway.

But to keep to your setup, here's a way to do that (and now data-* attributes come in handy):

Show
Hide

...

For the JS, you can then do something like this:

$(function() {
  var showLinks = $('a[data-shows]'),
      hideLinks = $('a[data-hides]');

  showLinks.on('click', function (event) {
    var target = $(this).addClass('active').data('shows');
    $(target).fadeIn().find(':input').prop('required', true);
    hideLinks.removeClass('active');
    return false;
  });

  hideLinks.on('click', function (event) {
    var target = $(this).addClass('active').data('hides');
    $(target).fadeOut().find(':input').prop('required', false);
    showLinks.removeClass('active');
    return false;
  });
});

There's a bit of repetition there, so you might do:

$(function() {
  var links = $('a[data-shows], a[data-hides]');

  links.on('click', function (event) {
    var link = $(this).addClass('active'),
        type = link.data('shows') ? 'shows' : 'hides',
        target = link.data(type),
        fadeFunc = type === 'shows' ? 'fadeIn' : "fadeOut";
    $(target)[fadeFunc]().find(':input').prop('required', type === 'shows');
    links.not(this).removeClass('active');
    return false;
  });
});

There are many ways of going about this


Sum of Absolute Differences algorithm implementation Those unfamiliar with that algorithm can have a deeper look here, in a few words, there are two arrays where you subtract each element ar1[0][0]-ar2[0][0] and add that result using the absolute values. My implementation is this:

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

#define LEN(arr) ((int) (sizeof (arr) / sizeof (arr)[0]))
int calSad(int templ[3][3], int image[3][3]);

int main()
{
    int tempImage[3][3] = {};

    int tem[3][3] = {
            {2, 5, 5},
            {4, 0, 7},
            {7, 5, 9}
    };

    int originalImage[3][5] = {
            {2, 7, 5, 8, 6},
            {1, 7, 4, 2, 7},
            {8, 4, 6, 8, 5}
    };

    int tempSad = 0, sad = 256, Xcoordinate = 0, Ycoordinate = 0;

    int originalImage_row_length = LEN(originalImage);
    int tempImage_row_length = LEN(tempImage);

    int originalImage_col_length = LEN(originalImage[0]);
    int tempImage_col_length = LEN(tempImage[0]);

    int tem_row_length = LEN(tem);
    int temp_col_length = LEN(tem[0]);

    int x, z, i, y, d = 0, q = 0;

    for (x = 0; x <= originalImage_row_length - tempImage_row_length; x++) {
        for (z = 0; z <= originalImage_col_length - tempImage_col_length; z++) {
            for (i = 0; i < tem_row_length; i++) {
                for (y = 0; y < temp_col_length; y++) {
                    tempImage[i][y] = originalImage[x+q][z+d];
                    d = d + 1;
                }
                q = q + 1;
                d = 0;
            }
            q = 0;
            tempSad = calSad(tem, tempImage);
            if (tempSad < sad) {
                sad = tempSad;
                Xcoordinate = x;
                Ycoordinate = z;
            }
        }
    }

    printf("SAD: %d\n", sad);
    printf("X: %d\n", Xcoordinate);
    printf("Y: %d\n", Ycoordinate);

    return 0;
}

int calSad(int templ[3][3], int image[3][3]) {
    int tempSad = 0;

    tempSad = (abs(templ[0][0] - image[0][0])+abs(templ[0][1] - image[0][1])+abs(templ[0][2] - image[0][2])+
               abs(templ[1][0] - image[1][0])+abs(templ[1][1] - image[1][1])+abs(templ[1][2] - image[1][2])+
               abs(templ[2][0] - image[2][0])+abs(templ[2][1] - image[2][1])+abs(templ[2][2] - image[2][2]));

    return tempSad;
}

The arrays are from the wikipedia link above, so I can duplicate the result and see if it works. It seems to work but I want your opinion if you see something wrong or if it doesn't work.

A more generic SAD function
Currently, your SAD function is hardcoded to work on a 3x3 area.  Also, because it takes 3x3 arrays as inputs, you need to use a temporary array tempImage in order to copy a 3x3 area of the original image so that you can pass it to your function.
If you modified your function a bit, you could pass the original image into the function without making a copy.  Here is how I would rewrite your function:
/**
 * Calculates sum of absolute differences between a pattern and an image.
 * The area of the image to be matched is at (imgX, imgY).
 */
int calSad(const int *pattern, int patWidth, int patHeight,
        const int *img, int imgX, int imgY, int imgWidth)
{
    int ret = 0;
    int x   = 0;
    int y   = 0;

    // Set img to point at &img[imgY][imgX].
    img = img + (imgWidth * imgY) + imgX;
    for (y = 0; y < patHeight; y++) {
        for (x = 0; x < patWidth; x++)
            ret += abs(pattern[x] - img[x]);
        pattern += patWidth;
        img     += imgWidth;
    }
    return ret;
}

Note that I used y to indicate row and x to indicate column, which is reverse of how you named your variables.
Your loop in main() would simplify to this:
for (x = 0; x <= originalImage_row_length - tempImage_row_length; x++) {
    for (z = 0; z <= originalImage_col_length - tempImage_col_length; z++) {
        tempSad = calSad(&tem[0][0], LEN(tem), LEN(tem[0]),
                        &originalImage[0][0], z, x, LEN(originalImage[0]));
        if (tempSad < sad) {
            sad = tempSad;
            Xcoordinate = x;
            Ycoordinate = z;
        }
    }
}


Sorting integer array Please check this program that sorts an integer array.  It doesn't look very efficient.  Please tell me how to correct this.

int temp;
for(int i=0;i<nums.length-1;i++)
{
    if(nums[i]>nums[i+1])// if this is true, swap the value
    {
        temp=nums[i];
        nums[i]=nums[i+1];
        nums[i+1]=temp;
        i=-1;//If there is swap happen, loop will start again from 0th element
    }
}

Here are some links to sorting techniques:
bubble sort ,
insertion sort, merge sortWhat you're doing in your code isn't efficient. In the worst case scenario where the array is totally unsorted it'll take quite sometime to sort it the way you're doing it.


User authentication system I'm new to OO PHP and I'm trying to create a simple properly-designed user authentication system.

What am I doing right and what not?
Is this right according to the MVC model?

Connection.class.php

<?php
class Connection {
    private $host = 'host';
    private $name = 'name';
    private $user = 'user';
    private $pass = 'pass';
    public $con;

    function __construct() {
        try {
            $this->con = new PDO("mysql:host=$this->host;dbname=$this->name;charset=utf8", $this->user, $this->pass);
            $this->con->setAttribute(PDO::ATTR_ERRMODE,PDO::ERRMODE_EXCEPTION);
        } catch(PDOException $ex) {
            die('Failed trying to connect to the database');
        }
    }
}

User.class.php

<?php
class User {
    private $id;
    private $username;
    private $email;
    private $password;

    function __construct($id, $username, $email, $password) {
        $this->id = $id;
        $this->username = $username;
        $this->email = $email;
        $this->password = $password;
    }

    function getId() {
        return $this->id;
    }

    function getUsername() {
        return $this->username;
    }

    function getEmail() {
        return $this->email;
    }

    function getPassword() {
        return $this->password;
    }
}

UserService.class.php

<?php
class UserService {

    protected $db;

    public function __construct($db) {
        $this->db = $db;
    }

    public function login($email, $password) {
        $stmt = $this->db->con->prepare('SELECT * FROM user WHERE email = ?');
        $stmt->execute(array($email));

        if ($stmt->rowCount() > 0) {
            //Check password here

            //Return user object and set session on success
            //$user = $stmt->fetchObject('User');
            $user = $stmt->fetch(PDO::FETCH_ASSOC);
            return new User($user['id'], $user['username'], $user['email'], $user['password']);
        }

            return false;
    }

    public function logout() { }

    public function register() { }

    public function getUser($userId) {
        $stmt = $this->db->con->prepare('SELECT * FROM user WHERE id = ?');
        $stmt->execute(array($userId));

        if ($stmt->rowCount() > 0) {
            return $stmt->fetch(PDO::FETCH_ASSOC);
        }

        return false;
    }
}

Example usage

//Include class files

$userService = new UserService($db);

if ($user = $userService->login('test@test.nl', 'testPass')) {
    echo 'Logged in as: ' . $user->getEmail();
} else {
    echo 'Invalid login';
}

Connection

Is just wrapping PDO in a class Connection.
Contains hard-coded credentials.

This class provides no significant benefit over using the PDO class directly. If somehow you need to change the database credentials, you'll have to change the class' properties directly, which is also a bad thing.

I suggest you get rid of your Connection class and just stick to using the PDO class directly. It already provides everything you need.

User

Just an anemic domain object with a bunch of getters and setters. I don't see anything wrong with that.

UserService

Violates the Single Responsibility Principle.

This class serves to:

Authenticate Users.
Register Users.
Write / read from the database.

I would transform it into 3 application service classes and give them all their own responsibility:

AuthenticationService:

This will serve to authenticate Users so it contains the following methods:

Find logged in User.
Log in.
Log out.

RegistrationService:

This class will take care of your User domain object registrations and contains the following method:

Register User.

And finally UserMapper:

This class will contain all the CRUD operations for your domain object User, and it will spit out User domain objects when reading from the database. It will contain the methods:

Find user by ID.
Find user by Email.

  Is this right according to the MVC model?

Hmm, not really. You're missing the dedicated Views and Controllers of the M V C pattern.

From what I see, you've got responsibilities of the View mixed with your code on the bootstrap level. The logic of which you are controlling the model classes with is supposed to be in a dedicated Controller. And the logic of what you output to the browser needs to be in it's corresponding View.

Btw, the 3 classes I've discussed above are supposed to reside in the Model layer in your MVC structure because they contain business and application logic ( = model).

Blueprint of a potential controller:

// declare namespace

// import statements

class AuthController
{
    // dependencies

    public function __construct(AuthService $authService, View $view)
    {
        // set the dependencies
    }

    public function index()
    {
        return $this->view->index()
    }

    public function submit()
    {
        if ($user = $this->authService->logInUser('foo@bar.nl', 'password')) {
            return $this->view->success([
                'user' => [
                    'email' => $user->getEmail()
                ]
            ]);
        }

        return $this->view->failed();
    }

    // log out action
}

Note: In the original MVC pattern the Controller is not supposed to call or instantiate any views, but solely control the Model layer, nothing else. However, over the years this has changed. Nowadays what you'll mostly see and hear is that a Controller's job is to control the model layer and call / return the appropriate View.

Wrapping up

Luckily your code is not in a super bad state. All it needs is some refactoring and restructuring to make it valid according to the MVC pattern and abide by the separation of concerns.

Try to adhere to the coding standards defined by the PHP Framework Interop Group. It gives better readability to your code, especially for other programmers who do it as well. Of course, you do not have to be perfect with it.

I hope my answer is clear enough and of some help to you. :)


Displaying some multiples Displaying Some Multiples
Write a program to calculate the multiples of a given number. Have the user enter a number, and then use a for loop to display all the multiples of that number from 1 to 12. It is not necessary to use a function.
You must use a for loop.
/*
 * Code by Clint <https://github.com/clieg>
 *
 *
 * Problem from: http://programmingbydoing.com/a/displaying-some-multiples.html
 */

import java.util.Scanner;

public class DisplayingSomeMultiples {
    public static void main(String[] args) {
        Scanner input = new Scanner(System.in);
        int number;

        System.out.print("Choose a number: ");
        number = input.nextInt();

        System.out.println();

        for (int i = 1; i <= 12; i++) {
            int mutiply = number * i;
            System.out.println(number + " x " + i + " = " + mutiply);
        }
    }
}

That looks quite good actually, minor stuff only.

for (int i = 1; i <= 12; i++) {

I'm a persistent advocate for using "real" names for loop variables, like index or count or factor in this case.
        for (int factor = 1; factor <= 12; factor++) {
            int result = number * factor;
            System.out.println(number + " x " + factor + " = " + result);
        }

        Scanner input = new Scanner(System.in);
        int number;

Do not declare all variables at the beginning of the method, declare them only when needed, that will limit the variable from creeping into scopes they don't belong in.

You could define the maximum factor as a constant.


General application log In my Node.js applications, I often find it helpful to have an application log so that should something go wrong in production, I can have more information than I get from a crash stack trace to track down problems.

To facilitate this, I use a global instance of Winston which is set up as one of the first things in my code:

var winston = require('winston');

global.applog = new (winston.Logger)({
  transports: [
    new (winston.transports.Console)({
      timestamp: true,
      level: 'debug'
    })
  ]
});

I then call applog.debug(), applog.info(), applog.warn(), and applog.error() as appropriate throughout my code.  Even if I am in a class included from the main application, I will often send debug messages.

For my purposes, this works great.  I get the information I need to keep things running smoothly, and handle unexpected behaviors.  However:

This reduces the reuse of any lib code as it cannot stand on its own without having a reference to applog.
It doesn't feel like a best practice to have global references used inside modular object-oriented code.
I don't think I would ever use a log utility that didn't have the standard methods (debug, info, warn, error), but if I did switch away from Winston it's possible that all my code would need to be updated.

Is there a more appropriate pattern I should be following for logging?

Having a log for a production application is an excellent idea. Some quick notes on logging web applications in general:

  Application logging should be always be included for security events.

  ...

  Each log entry needs to include sufficient information for the
  intended subsequent monitoring and analysis. It could be full content
  data, but is more likely to be an extract or just summary properties.
  The application logs must record "when, where, who and what" for each
  event.

OWASP Loggin Cheat Sheet

With that in mind, winston is generally considered a good choice for Node.js logging. It's dependencies are a little dated, and support is minimal. Another option is Scribe.js (lightweight, but also minimal support).

  This reduces the reuse of any lib code as it cannot stand on its own
  without having a reference to applog.

Not necessarily something bad. What you have is compact and easy to transport. You could consider pulling this snippet into it's own file and then require-ing that file in each JS script as needed.

I'm not aware of anything you can do to overcome this.

  It doesn't feel like a best practice to have global references used
  inside modular object-oriented code.

You're right, global variables/functions in general aren't the best. They can cause issues with naming and referencing in the global scope. There are a few solutions to this, w3 has a section devote to avoiding globals.

If your snippet can be placed inside a class (ES6 only) or an app object, I suggest that.

  if I did switch away from Winston it's possible that all my code would
  need to be updated.

Certainly something to think about. With a "find and replace all" command (using a regex perhaps in your IDE) though, much of the hassle could easily be taken care of.


Higher order functions in Python using reduce technique For the below query, show that both summation and product are
  instances of a more general function, called accumulate, with the
  following signature:

def accumulate(combiner, start, n, term):
    """Return the result of combining the first n terms in a sequence."""
    "*** YOUR CODE HERE ***"

  Accumulate takes as arguments the same arguments term and n as
  summation and product, together with a combiner function (of two
  arguments) that specifies how the current term is to be combined with
  the accumulation of the preceding terms and a start value that
  specifies what base value to use to start the accumulation. Implement
  accumulate and show how summation and product can both be defined as
  simple calls to accumulate:

def summation_using_accumulate(n, term):
    """An implementation of summation using accumulate.

    >>> summation_using_accumulate(4, square)
    30
    """
    "*** YOUR CODE HERE ***"

def product_using_accumulate(n, term):
    """An implementation of product using accumulate.

    >>> product_using_accumulate(4, square)
    576
    """
    "*** YOUR CODE HERE ***"

Below is the solution:

    from operator import mul, add

def accumulate(combiner, start, n, f):
    """Return the result of combining the first n terms in a sequence."""

    total = start           #Result of summation gets stored here
    i = 1                   #Initial value of sequence
    while i <= n:
        total = combiner(total, f(i))
        i = i + 1
    return total

def summation_using_accumulate(n, f):
    """An implementation of summation using accumulate.

    >>> summation_using_accumulate(4, square)
    30
    """
    return accumulate(add, 0, n, f)

def product_using_accumulate(n, f):
    """An implementation of product using accumulate.

    >>> product_using_accumulate(4, square)
    576
    """
    return accumulate(mul, 1, n, f)

def square(x):
    return mul(x, x)

print("product_using_accumulate: ",product_using_accumulate(4, square))
print("summation_using_accumulate: ",summation_using_accumulate(4, square))
print(accumulate(add, 0, 4, square))
print(accumulate(mul, 1, 4, square))

I have tested this code and looks good to me.

My questions:

Does the solution look incorrect in any aspect?
Any feedback on naming conventions?
Any feedback on coding style?

If a homework problem starts with the words "Show that", I would expect the answer to contain a proof (or at least an argument), not just an implementation.
You've misunderstood the problem statement. It says that start "specifies what value to use to start the accumulation", but in your implementation, start is the index of the first item in the sequence to contribute to the accumulation. Both of the test cases have start=1 and f(1)=1 so they are incapable of detecting your mistake.
Your implementations of summation_using_accumulate and product_using_accumulate do not actually use accumulate!


Session in Struts2 and Google App Engine Im using Struts2 running in google app engine.

User can use their google and facebook account to login to the site. When a user login, i have:

session.put("email", email);

When a user log-out, I have

session.remove("email");

In Struts2 Action, I have the following:

public class ChapterAction extends ActionSupport implements SessionAware{

    private static final long serialVersionUID = 1L;
    private Map<String, Object> session;

    public Map<String, Object> getSession() {
        return session;
    }
    public void setSession(Map<String, Object> session) {
        this.session = session;
    }

    public String add() throws Exception{
        String email = (String) session.get("email");
        if ( email == null)
            return LOGIN;

        //business logic here

        return SUCCESS;
    }
}

Please critic my code. Is the above usage of session OK and secure?

Thanks

Code looks good, But why not move this Part:

String email = (String) session.get("email");
if ( email == null)
    return LOGIN;

into an Interceptor? Or do you have only one relevant Action?


Design pattern to implement Database operations I am trying to implement design pattern in my project and have been reading material over net.

we have a sales application ( in C# ) where in we are storing entities like

Customer
Contacts
Sales Order
Lead
Opportunity

This is how i am planning to implement design pattern.

Entity would be the base class and it has common variables

namespace DesignModel.Model
{
    class Entity
    {
        protected IDBOperation dbOperation;
        public string id { get; set; }
        public int isSync { get; set; }
        public string timestamp { get; set; }

        public void insert()
        {
            dbOperation.insertData();

        }

        public void update()
        {
            dbOperation.updateData();
        }
        public void delete()
        {
            dbOperation.deleteData();

        }
    }
}

Here is the Contact class that is extending Entity class. Contact class would have its own properties too.

namespace DesignModel.Model
{
    class Contact : Entity
    {
        public Contact()
        {
             dbOperation = new DBOperations();

        }
        public string name { get; set; }
        public string company { get; set; }
        public string jobtitle { get; set; }

    }
}

Now what i have derived from the nature of this application is each entity has following operations for database interaction and to fetch data from server.

for database operation

insert
update
delete

to get and send data from server

getData
sendData

based on that i have created an interface named IDBOperation

namespace DesignModel.Database
{
    interface IDBOperation<T>
    {
        void insertData(T entity) ;
        void updateData(T entity);
        void deleteData(T entity);

        void displayData();

    }
}

and concrete class DBOperations that implements IDBOperation

namespace DesignModel.Database
{
    class DBOperations : IDBOperation<Entity>

    {

        public void insertData(Entity e) {

            Debug.WriteLine("insertData "+e.id );

        }

        public void deleteData(Entity e)
        {
            Debug.WriteLine("deleteData" + e.id);

        }

        public void updateData(Entity e)
        {
            Debug.WriteLine("updateData" + e.id);

        }
        public void displayData()
        {

        }
    }

and at last Contact class

namespace DesignModel.Model
{
    class Contact : Entity
    {
        public Contact()
        {
             dbOperation = new DBOperations();

        }
        public string name { get; set; }
        public string company { get; set; }
        public string jobtitle { get; set; }

    }
}

i am using contact as follows

    Contact contactData = new Contact();
    contactData.id = "1234";
    contactData.isSync = 1;
    contactData.jobtitle = "Sales Manger";
    contactData.insert(contactData);

I am little confused where i have been implementing it correctly or not.

the reason why i kept implementation in DBOperation because if new interface method comes i have to implement it at one place oppose to if i am implementing method in customer, contact and sales order classes.

one thing that worries me is Entity parameter in insertData inside DBOperation not sure whether i will be able to add entire contact into db or not.

so if anyone help me in providing pointers to correctly implement the design pattern in this use case then it will me more helpful

I think you might be looking for an ORM, which provides you with an intermediate layer between your application and the database for the sake of clarity.

Since you already mentioned that EntityFramework is not an option, you should consider developing your own library. It's going to take a while, but you surely won't have any headaches while hunting down the bugs of a library you didn't write or don't understand.

In your code, you are calling the delete, insert and update methods on the instances themselves, which is a bad idea per se, since these methods are executed on the database and not on that particular instance. Instead, you could solve this by refactoring your code.

I suggest you recreate the structure of your database server in your application too: you have one database server, within a server there are multiple databases (eg. abstract class for DatabaseEntity) and within a database there can be multiple tables (eg. abstract class for TableEntity). If you execute actions on a table, you might want to do this the following way:

public class MyServer
{
    // ....
}

public class MyDatabase : DatabaseEntity
{
    // ....
}

public class Account : TableEntity
{
    public int Identifier { get; set; }
}

MyServer Server = new MyServer();

Account MyAccount = Server.Database<MyDatabase>.Select<Account>(x => x.Identifier == 123);

MyAccount.Identifier = 456;
Server.Database<MyDatabase>.Update<Account>(MyAccount);

Server.Database<MyDatabase>.Insert<Account>(new Account());

Within that particular Insert<> function, you can scan through the properties and convert their names and values into an INSERT INTO query and execute it. You can also create an attribute for the primary key, which will be used as global identifier when eg. updating an instance. Most likely the hardest part is going to be the Select<> method. You literally have to examine the expression tree in the parameters, so that this expression:

Server.Database<MyDatabase>.Select<Account>(x => x.Identifier == 123);

can become pure SQL (or whatever db you're using):

SELECT * FROM `MyDatabase.Account` WHERE `Identifier` = 123;

With your own solution, you can shape the structure of your library to perfectly suit all your needs and to keep your code clean and well understandable.

Also, I currently have a weekend ORM project on Github based on the previously mentioned structure, you should check out the source code. Feel free to adapt it to your needs or use useful snippets from it.


Allocate Resource Effectively I have module called printer allocator (PrinterAllocator) which will allocate the next available printer to the requester.

import java.util.ArrayList;
import java.util.Arrays;
import java.util.BitSet;
import java.util.List;

public class PrinterAllocator {

    private static final int TOTAL_AVAILABLE_PRINTERS = 25;
    private BitSet bitSet = new BitSet( TOTAL_AVAILABLE_PRINTERS );

    public PrinterAllocator() {
        List<Integer> unassignedPriters = new ArrayList<>( Arrays.asList( 1, 2, 3, 24 ) ); // but this array will be fetched from database
        unassignedPriters.forEach( printerId -> bitSet.set( printerId - 1 ) );
    }

    public int getNextPrinter() {
        int index = bitSet.nextClearBit( 0 ) + 1;
        bitSet.set( index );
        return index + 1;
    }

    public boolean hasEnoughPrinters(int printersNeeded){
        /* point 1*/
        return bitSet.cardinality()+printersNeeded<=TOTAL_AVAILABLE_PRINTERS;
    }

}

Point 1: I am unable to replace 'point 1' with following code

 return bitSet.cardinality()+printersNeeded<=bitSet.size()

    (or)

return bitSet.cardinality()+printersNeeded<=bitSet.length();

Because bitSet.size() and bitSet.length() has a different meaning that is size() != length() != TOTAL_AVAILABLE_PRINTERS

Any better Idea to implement PrinterAllocator with/without bitset

Why do you use BitSet? You're not saving kilobytes of memory by representing your printers as a set of bits. Go for code clarity and represent each printer with a dedicated instance of Printer class that holds the allocation status of the printer in question. Store them in a List or other data structure that suits your needs. You could even have two LinkedLists for allocated and free printers and just pop the first from the free list and push it to the end of the allocated list. You need to synchronize anyway. No need to count sizes to see if there are available printers. Just check isEmpty().

bitSet is a bad name for printer reservartion status. Java is a strongly typed language so you don't need to repeat the type in the field name. Use printerAllocationStatus if you really need to use a BitSet (see above).

You're stacking on responsibilities by implementing hasEnoughPrinters in your allocator. Instead implement getNumberOfFreePrinters and let the caller decide what to do with the information. The method is of limited use anyway, since printer allocation status might change right after the caller has checked for availability making the data the caller has completely useless.

getNextPrinter does not communicate the side effect of changing printer allocation status in it's name. Use allocatePrinter instead.

Use consistent terminology. In a PritnerAllocator a printer is allocated or free, not assigned.


Container with most water The task
is taken from leetcode

  Given n non-negative integers a1, a2, ..., an , where each represents
  a point at coordinate (i, ai). n vertical lines are drawn such that
  the two endpoints of line i is at (i, ai) and (i, 0). Find two lines,
  which together with x-axis forms a container, such that the container
  contains the most water.

  Note: You may not slant the container and n is at least 2.

My first solution:

/**
 * @param {number[]} height
 * @return {number}
 */
var maxArea = height => {
  let max = 0;
  height.forEach((hL, i) => {
    height.slice(i + 1).forEach((hR, j) => {
      max = Math.max(max, Math.min(hL, hR) * (1 + j));
    });
  });
  return max;
};

My second solution:

/**
 * @param {number[]} height
 * @return {number}
 */
var maxArea2 = height => {
  let l = max = 0;
  let r = height.length - 1;
  while (l < r) {
    const area = Math.min(height[l], height[r]) * (r - l);
    if (height[l] > height[r]) {
      r--;
    } else {
      l++;
    }
    max = Math.max(max, area);
  }
  return max;
};

Is it possible to solve it mathematically and make it faster?

Is it possible to solve it mathematically and make it faster?

Not faster than the second solution,
which is \$O(n)\$, making a single pass over the input.
It's easy to see that it's not possible to find the correct answer without inspecting every element. For example, if two values are not inspected, then one could pick arbitrarily high values for them to make them produce the maximum area.

I have some style comments about the second solution:

I dislike the variable name l because in some fonts it can be confused with | or 1.
I would rename l to left and r to right for natural clarity. Still simple enough.
For symmetry with the while (l < r), I would rewrite the if in the loop body to be if (height[l] < height[r]), as I feel it would facilitate understanding of the logic.


Do I need to use inheritance and classes for my OOP webscraper? I am currently writing python code that scrapes information from the web.
I have to scrape several sites, but there a two types of procedures:

Directly scrape from the website
Download pdf and scrape it with regexes

I consider the following 3 options, which one would be recommended?
Option 1
Use inheritance:
import requests
import PyPDF2
from bs4 import BeautifulSoup
import re

class Scraper:
    def __init__(self, name):
        self.name = name
        self.url = None

    def get_text_from_pdf(self, page_number):
        self.download_pdf(self.url, './data/{}.pdf'.format(self.name))
        mypdf = open('./data/{}.pdf'.format(self.name), 'rb')
        fileReader = PyPDF2.PdfFileReader(mypdf)
        page = fileReader.getPage(page_number)
        text = page.extractText()
        return text

    def download_pdf(self, self.url, path):
        response = requests.get(self.url)
        with open(path, 'wb') as f:
            f.write(response.content)

    def get_soup_from_url(self):
        response = requests.get(self.url)
        result = response.text
        soup = BeautifulSoup(result)
        return soup

class Website1(Scraper):
    def __init__(self):
        super().__init__('website1')
        self.url = 'https://website1.com'

    def get_info(self, soup):
        '''
        Parse the html code through Beautifullsoup
        '''

class Website2(Scraper):
    def __init__(self):
        super().__init__('website2')
        self.url = 'https://website2.com/some_pdf.pdf'

    def get_info(self, text):
        '''
        Parse the pdf text through regexes
        '''

if __name__ == "__main__":
    Website1_Scraper = Website1()
    raw_info = Website1_Scraper.get_soup_from_url()
    Website1_Scraper.get_info(raw_info)

    Website2_Scraper = Website2()
    raw_info = Website2_Scraper.get_text_from_pdf(page_number=0)
    Website2_Scraper.get_info(raw_info)

    #Website3_Scraper, 4, 5 ... 10

Option 2
Only use the sub classes Website1 and Website2and convert the methods of the Scraper class to regular functions
Option 3
Delete all classes and only use functions, such as: get_info_from_website1() and get_info_from_website2()

I see no benefit to any of these approaches. You're just making yourself write more code. All you need is one class that analyzes a website entered from a user. If you have a predefined list of websites that you need to scrape, then iterate over each website instead of using a class for each one.
Since you say there are two ways of accomplishing this task, you can have the user pass in what mode they want to use to get the information. Something like
website = Scraper("https://www.google.com", "DIRECT")
website = Scraper("https://www.website_here.com/articles/article.pdf", "PDF")

Then you build your class around it. You want your code to be easily accessible and usable by a user. As a first time user, I would have no idea if I should use Website1 or Website2. What are the differences? Why should I use one over the other? You should always look at your code through the lens of a user.


Setting every element in an array to a given value Given the length of an array and a char, I would like to replace every element in a char array with that given char.

Using a for loop, I'm changing each element to that char. Is there a better way to do it, or a specific method for that?

using System;
using System.Collections.Generic;

namespace TestProject
{
    class Solution
    {
        static void TestMethod()
        {
            //lines of code
            int lengthOfArray = int.Parse(Console.ReadKey());
            char charReplace = Console.ReadKey().KeyChar;

            char[] arrayChar = new char[lengthOfArray];
            for (int i = 0; i < lengthOfArray; i++)
            {
                arrayChar[i] = charReplace;
            }
        }
    }
}

You can also use the static Array.Fill to quickly initialize an array to a given value:

bool[] isPrime = new bool[500];
Array.Fill(isPrime, true);

This will set all items in the array to true.

As mentioned in the comments by Pieter Witvoet, this feature is available in .NET Core only.


Multiple explicit cast operations This sample code works fine, but it looks awful.  How would you improve this?

data.Add(((Adress)(((OwnerIDList)owner.Adresses.Value)[0].Adress.Value)).FirstName.Value.ToString());
data.Add(((Adress)(((OwnerIDList)owner.Adresses.Value)[0].Adress.Value)).LastName.Value.ToString());

Why do we use .Value in FirstName.Value.ToString()?

FirstName is a DTString object (implements a basic interface for all data types to be stored in data base).

at least I would extract a variable:

var address = (Adress)((OwnerIDList)owner.Adresses.Value)[0].Adress.Value;
data.Add(address.FirstName.Value.ToString());
data.Add(address.LastName.Value.ToString());

All these cast operations make me believe that your code is not that strongly typed. If it so then there is not that much you can do with readability in this code.

P.S.: Address in English has two d.


Getting Record entries I have a class Record which keeps entries of strings(implemented using vector of strings). I want to keep the data member private to the class. To get entries, I have defined a member function get_entry which accepts a reference to vector of strings to store results.

Below is the code.

#include <iostream>
#include <vector>
#include <string>

class Record {
public:
        void add_entry(const std::string& entry)
        {
                static int seq = 1;
                entries.push_back(std::to_string(seq++) + " "+ entry);
        }
        void get_entry(std::vector<std::string>& res) const
        {
                for (auto &v: entries)
                        res.push_back(v);
        }

private:
        std::vector<std::string> entries;
};

int main()
{
        Record name;
        name.add_entry("abc");
        name.add_entry("def");
        std::vector<std::string> res;
        name.get_entry(res);
        for (auto &v : res)
                std::cout << v << "\n";
        return 0;
}

Are there other better ways to get entries from such a class? I don't want to make entries data member public and access it directly.

The obvious downside of your current approach is that you do a lot potentially unnecessary copying. One common solution is to return (const) iterators to the entries, i.e.,:

class Record
{
    public:
        // ...

       auto cbegin() const { return entries.cbegin(); }
       auto cend() const { return entries.cend(); }

       // Perhaps similar methods for getting non-const iterators ...
};

This solution allows you to use e.g., standard algorithms on the entries, but you can't manipulate the underlying range from the outside (due to const-ness).

To support your earlier use case, you can still do std::vector<std::string> res(r.cbegin(), r.cend()); if you wish, where r is an object of type Record.


Circular Queue in C for an embedded project I wrote a simple circular queue. (in order to store characters because of the fact, that I am using UART through the DMA and sometimes my printf's override each other)
I am looking for some review, though my testfile was successful I have the feeling I've overseen something and the queue might be buggy at some point.
I appreciate any kind of critics, ideas or improvements

Header File:

#ifndef __QUEUEH
#define __QUEUEH

typedef struct queue {

    unsigned int head;
    unsigned int tail;
    unsigned char* buf;
    unsigned int bufSize;
    unsigned int freeSpace;
    unsigned int occuSpace;
    unsigned char initialized;

} queue_t;

extern int queue_init(queue_t* q, unsigned char* buf, unsigned int bufSize);
extern int enqueue(queue_t* q, unsigned char* element, unsigned int size);
extern int dequeue(queue_t* q, unsigned char* str, int strMaxSize);
// change to static
extern int printQueue(queue_t* q);
#endif

C-File:

#include "queue.h"
#if !defined bool
typedef unsigned char bool;
#endif

extern int queue_init(queue_t* q, unsigned char* buf, unsigned int bufSize) {
    q->head = -1;
    q->tail = 0;
    q->buf = buf;
    q->bufSize = bufSize;
    q->freeSpace = bufSize;
    q->occuSpace = 0;
    q->initialized = 1;
    return 0;
}

static inline bool moveTail(queue_t* q) {
    q->occuSpace = q->occuSpace - 2 - (q->buf)[q->tail];
    q->freeSpace = q->freeSpace + 2 + (q->buf)[q->tail];
    q->tail = (q->tail + (q->buf)[q->tail]+2) % (q->bufSize);
    return 0;
}

static inline bool enoughSpaceAvailable(queue_t* q, unsigned int size) {
    if(q->freeSpace < size+2)
        return 0;
    return 1;
}

extern int enqueue(queue_t* q, unsigned char* element, unsigned int size) {

    if(size==0)
        return -5;
    else if(size>q->bufSize-2)
        return -8;

    while(!enoughSpaceAvailable(q,size))
            moveTail(q);

    q->occuSpace = q->occuSpace + 2 + size;
    q->freeSpace = q->freeSpace - 2 - size;
    q->head = (q->head+1)%(q->bufSize);
    (q->buf)[q->head] = size;
    for(int i = 0; i<size; i++) {
        q->head = (q->head+1)%(q->bufSize);
        (q->buf)[q->head] = element[i];
    }
    q->head = (q->head+1)%(q->bufSize);
    (q->buf)[q->head] = size;
    return 0;
}

extern int dequeue(queue_t* q, unsigned char* str, int strMaxSize) {

    int len = q->buf[q->tail];
    if(q->freeSpace == q->bufSize) return -6; else if(strMaxSize<len) return -7;

    for(int j=0; j<len; j++) {
        str[j] = q->buf[(q->tail+1+j)%(q->bufSize)];
    }
    moveTail(q);
    return 0;
}

Functionality unclear

Post discusses "... circular queue. (in order to store characters ...".

After digging deeply into code I now realize the goal is not to enqueue a character and then a character and so on.  Instead the goal is to enqueue a group of characters, then another group and so on.

This is not clear from the the post and importantly, it is not clear from the .h file.

Documentation

The .h file, as a public interface deserves  as least a little documentation/comment per function.

Nonuniform name-space

__QUEUEH, queue, queue_init, enqueue, dequeue, printQueue would benefit with a more  common naming convention to clearly show that they belong together.

Consider QUEUE_H, queue, queue_init, queue_set, queue_get, queue_print

queue_t

Information hiding: The members of queue_t do not need to be in the .h file.  The below is sufficient.  If external code needs to read any of these members, I recommend a function to do so.

typedef struct queue queue_t;

Put the full struct queue definition in the .c file.

.initialized is a good candidate for bool.

// unsigned char initialized;
bool initialized;

Unnecessary item in .h

As a static function, printQueue does not belong in the .h file.  Remove it.

Avoid standard library collisions

The below conflicts with bool from stdbool.h.  Do not re-invent the standard boolean type.

// #if !defined bool
// typedef unsigned char bool;
// #endif
#include <stdbool.h>

queue_init()

Why set .head, an unsigned type, to a negative value?  Set to a unsigned value.

// q->head = -1;
q->head = -1u;

moveTail()

The role of magic number 2 is unclear here and in many of the functions.  Explain the 2.

Unexpected strMaxSize

How does reading the data in the queue relate to the length of anything?

int len = q->buf[q->tail];

The role of strMaxSize in dequeue() is unclear.

Hmmmmm

After more review it looks like the first character in a group of characters is the group's length.

enoughSpaceAvailable()

Avoid overflow: size+2 can overflow - this code has no control over the value of size.  Consider a more robust test.

// if(q->freeSpace < size+2) return 0;
//return 1;

return q->freeSpace > size && q->freeSpace - size >= 2;

const

Many functions would benefit with a const queue_t *

// enoughSpaceAvailable(queue_t* q, unsigned int size)
enoughSpaceAvailable(const queue_t* q, unsigned int size)

3 to do the job of 2

Unclear why code uses .bufSize, .freeSpace, .occuSpace when only 2 are needed.  I'd expect .bufSize, .occuSpace are sufficient here.

Overall

Code lacks documentation to indicate the overall coding goals.  Without that, there is too much to discover.

I still do not know how to use best these calls.  Will enqueue(q, "", 0); cause havoc?   How to test is the queue is empty? What are all the error codes?  Why are errors negative?  What does a positive response mean?

To me, it is  the un-posted testfile contains too much information there and not enough here.

This code deserves an update and then a following review.

Sample alternate aqueue.h.
I avoided "queue" as too generic and used "aqueue".

/*
 * aqueue: queue of groups of binary data
 */

#ifndef AQUEUE_H
#define AQUEUE_H

typedef struct aqueue aqueue_t;

// All functions that return `int` return 0 on success.

/*
 * Initialize the queue with the buffer to use and its size.
 * Size [4..UINT_MAX]
 * Return TBD with invalid parameter values.
 */
int aqueue_init(aqueue_t* q, unsigned char* buf, unsigned bufSize);

/*
 * Insert a copy of what `data` points to.
 * Return TBD with insufficient space
 */
int queue_set(aqueue_t* q, const unsigned char* data, unsigned dataSize);

/*
 * Extract entry from the queue.
 * Return TBD when attempted with empty queue.
 * Return TBD when dataSize is too small.
 */
int aqueue_get(aqueue_t* q, unsigned char *data, unsigned dataSize);

/*
 * Queue empty?
 */
_Bool aqueue_empty(const aqueue_t* q);

/*
 * Get data without removing from the queue  (Peek)
 * Return TBD when attempted with empty queue.
 * Return TBD when dataSize is too small.
 */
int aqueue_top(const aqueue_t* q, unsigned char* data, unsigned dataSize);

#endif


Printing octo-thorpes Given a number, for example, \$6\$, this Python function should print hashes in the following format:

     #
    ##
   ###
  ####
 #####
######

This is the function used to accomplish this:

def hash_print(number):
    for i in range(number):
        hashes='#' *(i+1)
        spaces=' '*(number -(i+1))
        print("%s%s") %(spaces,hashes)

Is there any way that this can be improved?

Python's str has justifying functions. The relevant one here would be rjust(). So we could just use that directly:

def hash_print(number):
    for i in range(number):
        hashes = '#' * (i+1)
        print hashes.rjust(number)

This is also \$O(n^2)\$ to build up hashes over time, so on the off chance that performance becomes a consideration, we could build it up as we go:

def hash_print(number):
    hashes = [' '] * number
    for i in range(number):
        hashes[number - i - 1] = '#'
        print ''.join(hashes)


Code Review for Adding two numbers without using the + operator Here's my code. Please review it for correctness, readability and efficiency. If you think it can be improved also let me know. This code works fine for positive numbers but doesn't for negative. How can I change it ? Any comments for improvement will be appreciated.

int sum(int a, int b)
    {
       int sum = 0;
       int carry = 0;
       int mask = 1;
       sum = doAdd(a,b,mask,carry, 0);
       return sum;
    }

    int doAdd(int a,int b, int mask, int carry, int sum)
    {
       int aBit = a&mask;
       int bBit = b&mask;

       if(mask<a||mask<b||carry!=0)
       {
          sum = sum | (carry^aBit^bBit);
          carry = (carry==0) ? (aBit&bBit) : (carry&a|carry&b);
          mask = mask << 1;
          if(carry!=0)
          carry = mask;
          return (doAdd(a,b,mask,carry, sum));
       }

       return sum;
    }

Your algorithm looks good, but the best algorithm (as far as I know) is following:

Add 2 binary numbers without considering carry (an xor operation)
Construct the difference with the actual solution, i.e., construct a number in the carry positions (a and operation then a left shift)
Add the result of step 1 with step 2 (by recursion), until step2 is 0.

The code will look like this:

int add(int a, int b) {
    if (b == 0) return a;
    return add(a ^ b, (a & b) << 1);
}


Adding an item to database + JSON Is this code good? Or is it the noobiest PHP you've ever seen?

<?php
//Initialization Section
require "helper.php";
$dest = "../forms/addhandout.php";

//Check for $_POST Data
if(!$_POST) {
    send(array("msg" => "ERROR: No Data Recieved"));
} else if(!isset($_POST["name"]) || $_POST["name"] == "") {
    send(array("msg" => "ERROR: Name field cannot be left blank"));
} else if(!isset($_POST["class"])) {
    send(array("msg" => "ERROR: Class field cannot be left blank"));
}

//Set $_POST Data
$name = $_POST["name"];
$cid = $_POST["class"];

//Check if PHP is connected to database
if(!$sql) {
    send(array("msg" => "ERROR: Cannot Connect to Database"));
}

//Check for Existing Handout
$query = "SELECT id FROM Handouts WHERE name='$name' AND cid=$cid";
$result = mysqli_query($sql, $query);
if(!$result) {
    send(array("msg" => "ERROR: Cannot Connect to Database"));
} else if(mysqli_num_rows($result) > 0) {
    send(array("msg" => "ERROR: Handout already exists"));
}

//Add Handout to DB
$query = "INSERT INTO Handouts (name, cid) VALUES ('$name', $cid)";
$result = mysqli_query($sql, $query);
if(!$result) {
    send(array("msg" => "ERROR: Cannot Connect to Database"));
}

//Get Handout ID
$query = "SELECT id FROM Handouts WHERE name='$name' AND cid=$cid";
$result = mysqli_query($sql, $query);
if(!$result) {
    send(array("msg" => "ERROR: Cannot Connect to Database"));
}
$hid = mysqli_fetch_assoc($result)["id"];

//Get JSON from file
$json = jread("../data.json");

//Add handout to JSON
$json[$hid] = array();

//Write JSON to file
jwrite("../data.json", $json);

//Close Connection to DB
mysqli_close($sql);

//Return to Web Form
send(array("msg" => "SUCCESS: Your Handout has been Created!"));
?>

<?php
define("HOST", "localhost");
define("USR", "web");
define("PSWD", "thundercats...thundercats...HO!!!!!");
define("DB", "Inventory");

$sql = mysqli_connect(HOST, USR, PSWD, DB);
$dest = "";

function jread($file) {
    $handle = fopen($file, "r");
    if(!($data = fread($handle, filesize($file)))) {
        return FALSE;
    }
    $json = json_decode($data, TRUE);
    return $json;
}

function jwrite($file, $data) {
    $handle = fopen($file, "w");
    if(fwrite($handle, json_encode($data))) {
        return TRUE;
    } else {
        return FALSE;
    }
}

function redir() {
    global $dest;
    header("Location: " . $dest);
    exit();
}

function send($params) {
    global $dest;
    $get = http_build_query($params);
    header("Location: $dest?$get");
    exit();
}
?>

You have a major security vulnerability.

As in, serious enough that if I knew your web address, I could delete your entire database.

Near the top of your code, you set $cid to the raw value of $_POST["cid"]. That's bad. You need to escape this data before you put it anywhere near a database.

As of rit now, if I send a POST request to this page with the cid parameter set to ; DROP DATABASE Inventory; --, your entire database is gone. That string will be put into the $cid variable, and then when you execute these lines:

$query = "SELECT id FROM Handouts WHERE name='$name' AND cid=$cid";
$result = mysqli_query($sql, $query);

the following query will be executed on your database:

SELECT id FROM Handouts WHERE name='' AND cid=; DROP DATABASE Inventory; --

The first query will throw a syntax error, the second will delete your database, and any subsequent queries are commented out.

You need to fix this before you do anything else.


Oracle SQL Code - Multiple Joins used I'm currently working on some SQL code within Oracle Fusion BIP but it's not very optimised as it's using multiple LEFT,RIGHT and FULL OUTER joins to link tables together.

Unfortunately this is making the query timeout as there's roughly 80k lines and this will be the norm over a rolling 12 month period give or take a few thousand.

Is there any tips on how to optimise the below code to run quicker when ran.

SELECT DISTINCT
AID.INVOICE_ID,
LINES.AMOUNT,
AID.PERIOD_NAME,
GCC.SEGMENT1 AS Organisation,
GCC.SEGMENT2 AS ServiceArea,
GCC.SEGMENT3 AS AccountCode,
GCC.SEGMENT4 AS SubAccount,
INV.INVOICE_NUM,
INV.CREATION_DATE,
PO.SEGMENT1 as PO_Number,
SUP.VENDOR_NAME AS Supplier_Name,
--AID.LINE_TYPE_LOOKUP_CODE,
LINES.LINE_NUMBER,
LINES.DESCRIPTION,
SUP.SEGMENT1 as Supplier_Number,
FVV1.DESCRIPTION As OrganisationDesc,
FVV2.DESCRIPTION As ServiceAreaDesc,
FVV3.DESCRIPTION As AccountCodeDesc,
FVV4.DESCRIPTION As SubAccountDesc
FROM
AP_INVOICES_All INV
INNER JOIN
  AP_INVOICE_LINES_ALL LINES
  ON INV.INVOICE_ID = LINES.INVOICE_ID
INNER JOIN
  AP_INVOICE_DISTRIBUTIONS_ALL AID
  ON INV.INVOICE_ID = AID.INVOICE_ID
  --LEFT JOIN AP_INVOICE_LINES_ALL LINES
  --ON PO_HEADERS_ALL.PO_HEADER_ID = LINES.PO_HEADER_ID
FULL OUTER JOIN
  PO_HEADERS_ALL PO
  ON LINES.PO_HEADER_ID = PO.PO_HEADER_ID
LEFT JOIN
  PO_DISTRIBUTIONS_ALL PDA
  ON PO.PO_HEADER_ID = PDA.PO_HEADER_ID
INNER JOIN
  GL_CODE_COMBINATIONS GCC
  ON GCC.CODE_COMBINATION_ID = nvl(PDA.CODE_COMBINATION_ID, AID.DIST_CODE_COMBINATION_ID)
INNER JOIN
  POZ_SUPPLIERS_V SUP
  ON INV.VENDOR_ID = SUP.VENDOR_ID
LEFT JOIN
  FND_VS_VALUES_VL FVV1
  ON GCC.SEGMENT1 = FVV1.VALUE
LEFT JOIN
  FND_VS_VALUES_VL FVV2
  ON GCC.SEGMENT2 = FVV2.VALUE
LEFT JOIN
  FND_VS_VALUES_VL FVV3
  ON GCC.SEGMENT3 = FVV3.VALUE
LEFT JOIN
  FND_VS_VALUES_VL FVV4
  ON GCC.SEGMENT4 = FVV4.VALUE
WHERE
AID.LINE_TYPE_LOOKUP_CODE NOT IN
(
  'REC_TAX',
  'NONREC_TAX'
)
AND LINES.LINE_TYPE_LOOKUP_CODE NOT IN
(
  'TAX'
)
AND trim(TRANSLATE(FVV3.DESCRIPTION, '0123456789-,.', ' ')) is not null
ORDER BY
AID.INVOICE_ID,
LINES.LINE_NUMBER

You open with

  SELECT DISTINCT

. Is this really necessary? This is a performance killer. Perhaps duplicates can only occur between certain tables. If that were the case, put the distinct as deep as you can in query, even if this means adding additional nested queries.


Implementation of a linear congruential generator Here's a simple LCG that I've made to learn a bit more about pseudo random number generation.

Is this implementation correct, and if so, how can I improve it further?

The following code is self contained and it should run without problems.

#include <chrono>
#include <iostream>
#include <map>

namespace Random {
    class LCG {
        static constexpr uint64_t const A = 0x5851F42D4C957F2D;
        static constexpr uint64_t const C = 0x14057B7EF767814F;
        static constexpr uint64_t const M = 0xFFFFFFFFFFFFFFFF;

        uint64_t this_seed;

        auto now() noexcept {
            using namespace std::chrono;
            auto const output = high_resolution_clock::now();
            return output.time_since_epoch().count();
        }

    public:
        LCG() noexcept
            : this_seed(now()) {
        }

        LCG(uint64_t const value) noexcept
            : this_seed(value) {
        }

        void seed() noexcept {
            this_seed = now();
        }

        void seed(uint64_t const value) noexcept {
            this_seed = value;
        }

        // [0, 2 ^ 64 - 1)
        auto next() noexcept {
            this_seed = (this_seed * A + C) & M;
            return this_seed;
        }

        void discard(uint64_t const amount) noexcept {
            for (uint64_t i = 0; i != amount; ++i) {
                next();
            }
        }

        // [0, 1)
        double get() noexcept {
            return static_cast<double>(next()) / M;
        }

        // x - 0 == (-1 | 0 | 1) ? 0 : x > 0 ? [0, x) : (x, 0]
        int64_t get(int64_t const x) noexcept {
            return static_cast<int64_t>(get() * x);
        }

        // b - a == (-1 | 0 | 1) ? 0 : b > a ? [a, b) : (b, a]
        int64_t get(int64_t const a, int64_t const b) noexcept {
            return a + static_cast<int64_t>(get() * (b - a));
        }
    };
}

int main() {

    Random::LCG lcg(0);
    std::map<int64_t, uint64_t> buckets;
    for (uint64_t i = 0; i != 1000000; ++i) {
        ++buckets[lcg.get(10)];
    }
    for (auto const [a, b] : buckets) {
        std::cout << a << '\t' << b << '\n';
    }

    return 0;
}

Your mask value M is gaining you nothing, since the types for this_seed and M are the same and you have every bit in M set to 1.  Your comment for next is wrong; it can return 264-1 (i.e., all bits set).  The correct range can be stated as either [0, 2 ^ 64 - 1] or [0, 2 ^ 64).  This in turn can cause your get functions to return a value larger than expected (1.0, x, or b).

Where did you get the values for the A and B constants?  I've not looked to see if they are appropriate.


Update UNIX-like system user profile picture with its GitHub account's one Seeking to learning and have fun, I wrote out this script (what I expected to be) self explanatory and would appreciate some comments on that.
I published it as a gist and tried to do my best on what I understand about object-orientation, documentation, code readability and python best practices, so to speak. The idea was practice and learn!
Any review in these or other concepts that could turn this result in a better solution is very welcome!
#!/usr/bin/python3
"""Update UNIX-like system user profile picture with its GitHub account's one

This script assumes same system and platform username, but a different one can be declared.
It downloads target GitHub account's picture and set it up to current system user
"""

import os
import requests
import configparser
import click

# Get system's logged username
USER = os.getlogin()

def save_github_picture(username, path):
    """Gets GitHub user account's profile picture and saves it into target path

    :param username: GitHub account username
    :param path: Picture's path destination
    """

    with open(path, "wb") as picture:
        gh_profile_picture = requests.get(f"https://github.com/{username}.png").content
        picture.write(gh_profile_picture)

def set_user_profile_picture(path):
    """Edits logged user AccountsService file to setup its profile picture path

    :param path: New profile picture path
    """

    # UNIX-like system user settings file
    accounts_service_path = f"/var/lib/AccountsService/users/{USER}"

    cfg = configparser.ConfigParser()
    cfg.read(accounts_service_path)
    cfg['User']['Icon'] = path

    with open(accounts_service_path, 'w') as accounts_service:
        cfg.write(accounts_service)

@click.command()
@click.argument('github_username', default=USER)
@click.option('--path', default=f"/home/{USER}/.face", help='define picture target path.')
def run(github_username, path):
    """Command line interface to define script's arguments

    :param github_username: GitHub account username (default: system user)
    :param path: New profile picture path (default: ~/.face)
    """
    try:
        save_github_picture(github_username, path)
        set_user_profile_picture(path)
    except Exception:
        raise
    else:
        print(f"User profile picture updated with {github_username}'s GitHub one")

if __name__ == '__main__':
    if os.geteuid() != 0:
        raise PermissionError("This action requires root privileges")
    else:
        run()

try:
    ...
except Exception:
    raise
else:
    ...

You're not doing anything with the captured exception, so this accomplish nothing more than the code in .... You can drop the try, the code in the else will never execute if there is an exception in the previous two functions anyway.
Depending on the size of the downloaded images, you may also consider streaming the http response to a file directly instead of using the memory as a buffer.


Converting a number to binary and back as a test in Java This is one of my first takes on Java, coming mainly from Matlab I found I had many things to learn.
The code will try to convert a number into an array of ones and zeros and then do the opposite returning you the original number (or at least something very close to it, as it is hard to return the exact number). It is not tested very well, so I can only tell that it is symmetric and not that it is correct.

It would be nice to know any errors I have made, or things that I could improve. An idea I had was using BigInt, I have read about it but not yet used it.

If you could focus on the language specific issues and kindly point them out I would be grateful.

package baseconversion;

import java.lang.Math;
import java.util.Arrays;

public class FloatToBin{

    public static double pow2(double a, double b) {
        return a * (java.lang.Math.pow(2, b));
    }

    public static double rem(double a, double b) {
        return a % b;
    }

    public static double fix(double val) {
        if (val < 0) {
            return Math.ceil(val);
        }
        return Math.floor(val);
    }

    public static double vConv(int[] a, double[] b) {
        int aRows = a.length;
        int bRows = b.length;
        double c = 0;
        for (int i = 0; i < aRows; i++) {
                c += a[i] * b[i];
        }
        return c;
    }

    public static void main(String []args){
        double a = 340.3562;
        int n = 20;
        int m = 20;

        int count = 0;

        int[] out = new int[n + m];

        for (int i = -(n-1); i <= m; i++) {
             out[count] = (int)rem(a * pow2(1, (double)i), 2);
             count++;
        }

        double[] temp = new double[n + m];
        double origVal;
        count--;
        int revCount = 0;

        for (int ii = n - 1; ii >= -m; ii--) {
             temp[revCount] = pow2(1, (double)ii);
             count--;
             revCount++;
        }

        origVal = vConv(out, temp);

        System.out.println("Original number is:");
        System.out.println(a);
        System.out.println("Binary representation is:");
        System.out.println(Arrays.toString(out));
        System.out.println("Inverting we get:");
        System.out.println(origVal);
    }
}

Cryptic names

int n = 10;
int m = 20;

Please give longer and more descriptive names to n and m

Overworked main

I would define outside of main

public static double FloatToBin(float n) {

}

public static double BinToFloat(bin n) {

}

So that main can contain the IO only.


Python xml schema parsing for simpleContent and simpleTypes I am writing a few python functions to parse through an xml schema for reuse later to check and create xml docs in this same pattern. Below are two functions I wrote to parse out data from simpleContent and simpleType objects. After writing this, it looked pretty messy to me and I'm sure there is a much better (more pythonic) way to write these functions and am looking for any assistance. I am use lxml for assess to the etree library.

def get_simple_type(element):
    simple_type = {}
    ename = element.get("name")
    simple_type[ename] = {}
    simple_type[ename]["restriction"] = element.getchildren()[0].attrib
    elements = element.getchildren()[0].getchildren()
    simple_type[ename]["elements"] = []
    for elem in elements:
        simple_type[ename]["elements"].append(elem.get("value"))
    return simple_type

def get_simple_content(element):
    simple_content = {}
    simple_content["simpleContent"] = {}
    simple_content["simpleContent"]["extension"] = element.getchildren()[0].attrib
    simple_content["attributes"] = []
    attributes = element.getchildren()[0].getchildren()
    for attribute in attributes:
        simple_content["attributes"].append(attribute.attrib)
    return simple_content

Examples in the schema of simpleContent and simpleTypes (they will be consistently formatted so no need to make the code more extensible for the variety of ways these elements could be represented in a schema):

<xs:simpleContent>
    <xs:extension base="xs:integer">
        <xs:attribute name="sort_order" type="xs:integer" />
    </xs:extension>
</xs:simpleContent>

<xs:simpleType name="yesNoOption">
    <xs:restriction base="xs:string">
      <xs:enumeration value="yes"/>
      <xs:enumeration value="no"/>
      <xs:enumeration value="Yes"/>
      <xs:enumeration value="No"/>
    </xs:restriction>
</xs:simpleType>

The code currently creates dictionaries like those show below, and I would like to keep that consistent:

{'attributes': [{'type': 'xs:integer', 'name': 'sort_order'}], 'simpleContent': {'extension': {'base': 'xs:integer'}}}

{'yesNoOption': {'restriction': {'base': 'xs:string'}, 'elements': ['yes', 'no', 'Yes', 'No']}}

Do more with literals:

def get_simple_type(element):
    return {
        element.get("name"): {
            "restriction": element.getchildren()[0].attrib,
            "elements": [ e.get("value") for e in element.getchildren()[0].getchildren() ]
        }
    }

def get_simple_content(element):
    return {
        "simpleContent": {
            "extension": element.getchildren()[0].attrib,
            "attributes": [ a.attrib for a in element.getchildren()[0].getchildren() ]
         }
    }


Computing the relative magnitude of the values in one double array to another An interview question I received:

  You are given two unsorted lists of positive doubles

  x contains the results from experiment 1
  y which contains the results from experiment 2.

  All results from the second experiment y are improvements over x
  by i percent, where i is an integer. That is to say that given a
  result from x, the corresponding result from y will be an i
  percent improvement.

  However, the lists are not parallel; that  is to say x[0] does not
  necessarily correspond to y[0]. Find the value of i.

  Note: must be written in Java 7

  Some examples

  Inputs:

y = [1.0]
x = [1.0]

  Output:

0

  Inputs:

y = [2.2999999999999998, 15.0, 102.40000000000001, 3486.8000000000002]
x = [23.0, 150.0, 1024.0, 34868.0]

  Output:

90

  Inputs:

y = [23, 11.1, 50.4]
x = [22.2, 46, 100.8]

  Output:

50

  Note in the last case where 46 -> 23 and 22.2 -> 11.1 but the
  index of 46 is not the index of 23, etc.

My Solution

import java.util.Arrays;

//Time complexity: O(n)
//Space complexity: O(1)
public class Solution{
    public static int solution(double[] secondRun, double[] firstRun) {

        Double firstRunMax = findMax(firstRun),
               secondRunMax = findMax(secondRun);

        return (int)((1.0-secondRunMax/firstRunMax)*100);
    }

    private static double findMax(double[] array){
        double max = -Double.MAX_VALUE;

        for(double d : array){
            if(d > max){
                max = d;
            }
        }

        return max;
    }
}

Strengths: Very simple, as efficient as possible (I think?), ignores most of the data

Weakness: the return line is a little convoluted. Perhaps there is a better way to simplify it also. Also I thought I would be able to find a simple Arrays.Max method or something in Java, but I didnt see any during a quick glance.

You are auto-boxing the results of your function call, only to un-box them later. If everything else is using double (a primitive), there is no reason to declare the local variables in solution() as Double (an object). This doesn't effect the big-O of your solution, but it is extra operations for the JVM to execute.

Give your class and public method better names. If you actually needed this in production code, there would be no way to know what this code did. The names tell you nothing and there is no documentation.

There is a max(), but it works on Collections, not arrays. However, writing your own max method is not that hard, so it is up to you if you want to replace it.

Note: Until seeing you question, I didn't know that Double.MIN_VALUE and Integer.MIN_VALUE do not represent the same concept for the respective type. This isn't your fault, but it is unintuitive. Explination


Calculating exponential moving averages I'm wanting to calculate exponential moving averages of a variable, distance. Is the logic (and math) correct in the below code?

time and lastTime are millisecond-precise timestamps in seconds – the former is the current time, the latter is the time of the last calculations.

lastA etc are the exponential moving averages from the last calculations.

a etc will be left with the calculated exponential moving averages.

var distance = ...;
var a = Math.pow(1.16, -(time-lastTime)),
    b = Math.pow(1.19, -(time-lastTime)),
    c = Math.pow(1.22, -(time-lastTime)),
    d = Math.pow(1.26, -(time-lastTime)),
    e = Math.pow(1.30, -(time-lastTime)),
    f = Math.pow(1.35, -(time-lastTime)),
    g = Math.pow(1.40, -(time-lastTime));
a = a*lastA + (1-a)*distance;
b = b*lastB + (1-b)*distance;
c = c*lastC + (1-c)*distance;
d = d*lastD + (1-d)*distance;
e = e*lastE + (1-e)*distance;
f = f*lastF + (1-f)*distance;
g = g*lastG + (1-g)*distance;

This is border line a bad question, as not enough code is given to properly review it.

The variables a -> g look terrible, I would create an array with the numbers you need:

var dataPoints = [1.16,1.19,1.22,1.26,1.30,1.35,1.40];

Then I would would loop over those points and create an averages object

var averages = {},
    value, x;
for(var i = 0, length = dataPoints.length ; i < length ; i++ ){
  value = dataPoints[i];
  x = Math.pow(value, -(time-lastTime));
  averages[value] = x * lastAverages[value] + (1-x) * distance;
}

I cannot tell whether the math is correct, if it is not correct, then this question does not belong here :)


Are there any problems with this memcpy() implementation? I received this question at a C++ programming interview:

void memcpy(char* dst, const char* src, int numBytes)
{
    int* wordDist = (int*)dst;
    int* wordSrc = (int*)src;
    int numWords = numBytes >> 2;
    for (int i = 0; i < numWords; i++)
    {
        *wordDist++ = *wordSrc++;
    }
    int whatisleft = numBytes >> 2 - (numWords );
    dst = (char*)wordDist;
    src = (char*)wordSrc;
    for (int i = 0 ; i <= whatisleft; i++);
    {
        *dst++ = *src++;
    }
}

What are portability issues?
At least 1 syntax error?
Algorithm considerations?

Now I found that this line:

int whatisleft = numBytes >> 2 - (numWords );

is wrong as it doesn't calculates the correct number of bytes left to copy.

The correct one should be:

int numBytesInWord = (numWords << 2 );
int whatisleft = numBytes  - numBytesInWord;

At the end, the copied char array is several characters shorter than the source.

There's a number of issues with this code:

int* wordDist = (int*)dst;
int* wordSrc = (int*)src;

Even if this works on a given platform, it is definitely not portable. int* can often have more strict alignment requirements than char* - this is technically undefined behaviour.

Further, (int*)src; is casting away constness - always dangerous if you then modify the value somewhere in the rest of the function.

int numWords = numBytes >> 2;

This assumes that an int is 32 bit, which is again non-portable, and will of course be wrong on any platform where this isn't the case. This could be fixed by using:

int numWords = numBytes / sizeof(int);

However, the alignment issues mentioned previously still remain.

This code:

int whatisleft = numBytes >> 2 - (numWords);

is completely wrong, as thanks to operator precedence, it is equivalent to numBytes >> (2 - numWords) which is likely to be negative, causing undefined behaviour. Also, one has to be careful regardless, and keep in mind the differences between an arithmetic right shift and a logical right shift. Again, technically, right shifts for signed values are implementation defined, and could potentially be either.


stability of an exponential moving average I used the following code:

#include<vector>
class UltrasonicRecorder
{
public:
    UltrasonicRecorder(int referenceValue, Input referenceInput) :
        _referenceValue(referenceValue),
        _referenceInput(referenceInput)
    {}

    std::vector<int> GetData();

    void RecordData(Input);
    int CompensateTemperatur(int);

private:
    double _compensation;
    int _referenceValue;
    Input _referenceInput;
    std::vector<int> _data;
};

std::vector<int> UltrasonicRecorder::GetData()
{
    std::vector<int> ret;
    std::swap(ret, _data);

    return ret;
}

void UltrasonicRecorder::RecordData(Input input)
{
    int value = (ANALOG_VALUE_RANGE - 1) - GetAnalogValue(input);
    value = CompensateTemperature(value);
    _data.push_back(value);
}

int UltrasonicRecorder::CompensateTemperature(int value)
{
    int measuredValue = GetAnalogValue(_referenceInput);
    if(measuredValue > 0)
    {
        static const double delta = 1.0e-6;
        _compensation = (1.0 - delta) * _compensation +
            delta * _referenceValue / (double)measuredValue;
    }
    return static_cast<int>(value * _compensation);
}

int GetAnalogValue(Input) returns a 12 bit value from a ultrasonic sensor measuring distance (0-4095, ANALOG_VALUE_RANGE = 4096). double _compensation; is a member of the encapsulating class. _referenceValue is the initial reference value from _referenceInput (normally it has a low fluctuation, with a value somewhere in the middle of the 12 bit range, but it changes with temperature).

RecordData() is called every 2 milliseconds (more or less, running on Windows). So a high update rate for the reference value, but I wanted a gradual compensation over time. Therefore I used an exponential moving average, the delta is empirically determined and may need to be changed later.

GetData() is called approximately every second to flush _data and use the record for further evaluation.

We started testing this code on customer facilities, but got the feedback that values are drifting away after a few days. I think it's a numerical unstable algorithm that accumulates rounding errors. Can someone with more experience with the subtleties of floating-point arithmetic answer me:

Can this exponential moving average algorithm be improved to achieve better stability?

Notes for additional improvements of the code are welcomed as well.

How about initializing the compensation factor so that it doesn't have to do the convergence from an undefined state?

UltrasonicRecorder(int referenceValue, Input referenceInput) :
    _referenceValue(referenceValue),
    _referenceInput(referenceInput)
{
    _compensation = 1.0; // This!
}

Edit: Your EWMA is correct. Your woes of compounding errors are unfounded. As you always "move towards" the measurement value, any compounding errors will be limited. In fact the compound error will at any point in time be less than:

(sum{i=0,infty} (1-delta)^i)*ULP(ANALOG_VALUE_RANGE) ~ 2^-(52-12) / delta ~ 10^-6


Get a "train or test" slicing elegantly after splitting I am learning numpy, pandas, and sklearn for machine learning now. My teacher gives me code like below, but I guess it is ugly. This code is for splitting the data into training set and testing set, and later we could manipulate the entire data or just training/testing set by the [True, False, True, False ... ] idx_train/idx_test list later.

import numpy as np
import pandas as pd
from sklearn.model_selection import train_test_split

m = 10
x1 = np.arange(m)
x2 = x1 * 2
x3 = x1 * 3
x4 = x1 * 4
x = np.c_[x1, x2, x3, x4]
x = pd.DataFrame(x)

x_train, x_test = train_test_split(x, train_size=0.7, random_state=666)
TRAIN, TEST = 'train', 'test'
TRAIN_TEST = TRAIN + TEST
pd.options.mode.chained_assignment = None
x_train[TRAIN_TEST] = TRAIN
x_test[TRAIN_TEST] = TEST
pd.options.mode.chained_assignment = 'warn'
x = pd.concat([x_train, x_test])
idx_train = x[TRAIN_TEST] == TRAIN
idx_test = np.invert(idx_train)
x_train = x[idx_train]  # later
x_test = x[idx_test]

And I get the below code by myself. I feel it is only a little better.
offset_orig = np.arange(m)
rate = 0.7
offset_train, offset_test = train_test_split(offset_orig, train_size=rate, random_state=666)
if rate < 0.5:
    offset_set_train = set(offset_train)
    idx_train = [i in offset_set_train for i in offset_orig]  # This may be slow. Is numpy broadcast or implicit cycling available here?
    idx_test = np.invert(idx_train)
else:
    offset_set_test = set(offset_test)
    idx_test = [i in offset_set_test for i in offset_orig]  # This may be slow. Is numpy broadcast or implicit cycling available here?
    idx_train = np.invert(idx_test)

x_train = x[idx_train]  # later
x_test = x[idx_test]

I want to get the [True, False, True, False ... ] list idx_train and idx_test for slicing usage later.
I believe that in the world of Python, there should be and will be only one appropriate way to achieve an objective. Could someone give me that very approach to get a "train or test" slicing? (Usage of the lib function train_test_split is required.)

Inspired by Gulzar at Stackoverflow.
I do not realize it works until after a night's sleep.
So, below is enough:
offset_orig = range(m)
offset_train, offset_test = train_test_split(offset_orig, train_size=0.7, random_state=666)

x_train = x.iloc[offset_train]  # later
x_test = x.iloc[offset_test]

The offset_train list ([0, 2, ...]) has the same feature in later programming against the [True, False, True, False ...] list for slicing I pursued at the beginning.
For example, I can use offset_train later as below:

import matplotlib.pyplot as plt
import seaborn as sns
fig, axes = plt.subplots(nrows=2, ncols=2)
fig.set_size_inches(12, 10)
sns.boxplot(data=df.iloc[offset_train], y='count', ax=axes[0][0])
sns.boxplot(data=df.iloc[offset_train], y='count', x='season', ax=axes[0][1])
sns.boxplot(data=df.iloc[offset_train], y='count', x='hour', ax=axes[1][0])
sns.boxplot(data=df.iloc[offset_train], y='count', x='workingday', ax=axes[1][1])

And even as below:

# get noise index
mu_count = df.loc[offset_train, 'count'].mean(axis=0)
sigma_count = df.loc[offset_train, 'count'].std(axis=0)
idx_bad_np = abs(df.loc[offset_train, 'count'] - mu_count) > 3 * sigma_count
idx_good_np = np.invert(idx_bad_np)
# remove noise index
print(f'Before removal: sum(idx good) = {sum(idx_good_np)}, sum(idx bad) = {sum(idx_bad_np)}, len(df[train]) = {len(df.iloc[offset_train])}')
offset_train = list(set(offset_train) - set(np.array(offset_train)[idx_bad_np]))
print(f'After removal: sum(idx good) = {sum(idx_good_np)}, sum(idx bad) = {sum(idx_bad_np)}, len(df[train]) = {len(df.iloc[offset_train])}')
# check again
idx_bad_np = abs(df.loc[offset_train, 'count'] - mu_count) > 3 * sigma_count
print(f'Check sum(idx bad) after removal: {sum(idx_bad_np)}')

Thanks to the community. I feel I have so far to go on the path of Python.