Scrape data from website into dataframe(s) using Split function Outline:

This code uses the Split function to extract specific information from the following website: https://www.webscraper.io/test-sites/tables.

The required information are the four tables visible on the page with headers "#", "First Name","Last Name","Username".  I am extracting the information within these into 4 dataframes.

Example table:

Description:

I use the requests library to make the GET request, and split the response text on "table table-bordered" to generate my individual table chunks.

There is a fair amount of annoying fiddly indexing to get just the info I want, but the tutorial I am following requires the use of the Split function, and not something far more logical, to my mind, like Beautiful Soup, where I could just apply CSS selectors, for example, and grab what I want. The latter method would be less fragile as well.

I have written a function, GetTable, to parse the required information from each chunk and return a dataframe. There is a difference between the Split delimiter for table 1 versus 2-4.

There isn't an awful lot of code but I would appreciate any pointers on improving the code I have written.

I am running this from Spyder 3.2.8 with Python 3.6.

Code:

def GetTable(tableChunk):
    split1 = tableChunk.split('tbody')[1]
    split2 = split1.split('<table')[0]
    values = []

    aList = split2.split('>\n\t\t\t\t<')
    if len(aList) !=1:
        for item in aList[1:]:
                values.append(item.split('</')[0].split('d>'[1])[1])
    else:
        aList = split2.split('</td')
        for item in aList[:-1]:
           values.append(item.split('td>')[1])

    headers =  ["#", "First Name", "Last Name", "User Name"]
    numberOfColumns = len(headers)
    numberOfRows = int((len(values) / numberOfColumns))

    df = pd.DataFrame(np.array(values).reshape( numberOfRows, numberOfColumns ) , columns = headers)
    return df

import requests as req
import pandas as pd
import numpy as np

url = "http://webscraper.io/test-sites/tables"
response = req.get(url)
htmlText = response.text
tableChunks = htmlText.split('table table-bordered')

for tableChunk in tableChunks[1:]:
   print(GetTable(tableChunk))
   print('\n')

Don't parse HTML manually, you should use the BeautifulSoup module!
import should be at the top of the file
Use a if __name__ == '__main__' guard
Functions and variable should be snake_case

First you can rewrite the getTable() alot using the BeautifulSoup module

import requests
from bs4 import BeautifulSoup

url = "http://webscraper.io/test-sites/tables"
soup = BeautifulSoup(requests.get(url).text, 'html.parser')
for table in soup.select('.table'):
    new_table = [[c.text for c in row.find_all('td')] for row in table.find_all('tr')]

The only problem is that it will also give back None values in the table, so we'd need to catch the None values and only yield when the list is not filled with None

Revised Code

import requests
import pandas as pd
from bs4 import BeautifulSoup

def parse_table(table):
    for row in table.find_all('tr'):
        col = [c.text for c in row.find_all('td')]
        if not all(c is None for c in col):
            yield col

def scrape_tables():
    url = "http://webscraper.io/test-sites/tables"
    soup = BeautifulSoup(requests.get(url).text, 'html.parser')
    for table in soup.select('.table'):
        parsed_table = [col for col in parse_table(table)]
        df = pd.DataFrame(parsed_table, columns=["#", "First Name", "Last Name", "User Name"])
        print()
        print(df)

if __name__ == '__main__':
    scrape_tables()


Project Euler 18/67: Maximum Path Sum Solution I wrote my solution to Project Euler #18 and #67 in C++ (which I'm relatively new to), and was just wondering what everyone else thinks of it. It executes in 3-4ms and works flawlessly (even for triangles not given by the website). But I'm not a professional programmer or anything, so I wouldn't know if it's necessarily good or not. Anyone have any potential improvements?

The website's instructions:

  By starting at the top of the triangle below and moving to adjacent numbers on the row below, the maximum total from top to bottom is 23.

   3
  7 4
 2 4 6
8 5 9 3

  That is, 3 + 7 + 4 + 9 = 23.

  Find the maximum total from top to bottom in triangle.txt (right click and 'Save Link/Target As...'), a 15K text file containing a triangle with one   hundred rows.

My code:

#include <iostream>
#include <fstream>
#include <string>
#include <ctime>
#include <sstream>
#include <vector>

int main()
{
    using namespace std;

    clock_t starttim = clock();
    //prog
    vector<vector<int>> lines;

    ifstream istrm("triangle.txt");
    string line;
    while (getline(istrm, line))
    {
        int num;
        stringstream ss(line);
        vector<int> curvec;
        while (ss >> num)
        {
            curvec.push_back(num);
        }
        lines.push_back(curvec);
    }

    for (int currow = lines.size() - 1; currow >= 0; currow--)
    {
        if (currow != 0)
        {
            for (int curidx = 0; curidx <= lines[currow].size() - 2; curidx++)
            {
                int num1 = lines[currow][curidx];
                int num2 = lines[currow][curidx + 1];
                int greater = (num1 > num2 ? num1 : num2);
                lines[currow - 1][curidx] += greater;
            }
        }
        else cout << lines[0][0] << endl;
    }

    //endprog
    cout << "Execution time - " << (((clock() - starttim) / (double)CLOCKS_PER_SEC) * 1000) << "ms." << endl;
    system("PAUSE");
    return 0;
}

This is pretty good code for a C++ beginner.  I believe you got the right algorithm.  Its running time is linear in the number of entries.  You visit every element once; I don't believe it is possible to do better than that.  Its memory usage is also linear in the number of entries.  The memory usage, however, can be improved.  Instead of processing the rows bottom-up, if you do it top-down, you can process the rows as you read them, keeping just two rows' worth of data at any given time.

It would be slightly more elegant to implement a simple abstraction layer for parsing the input.  I would also avoid hard-coding the filename.

Writing using namespace std; is considered bad practice.

The if-else inside the for-loop is pointless.  It just means that currow >= 0 was a poor termination condition.

I find the "cur…" prefixes on your variable names annoying.  What's "curve c"?  What's "currow" — does it rhyme with "furrow"?  As for "curidx", how about just i?

The timing printout should go to standard error to avoid contaminating the output.  You can avoid the (double) cast by writing 1000.0 to trigger floating-point arithmetic.

#include <ctime>

#include <algorithm>
#include <fstream>
#include <iostream>
#include <memory>
#include <sstream>
#include <string>
#include <vector>

template<typename T>
class Input {
  public:
    Input<T>(std::istream &in) : in(in) {}

    bool next_row(std::vector<T> &entries) {
        entries.clear();
        std::string line;
        if (!std::getline(this->in, line)) {
            return false;
        }
        std::stringstream ss(line);
        for (T n; ss >> n; ) {
            entries.push_back(n);
        }
        return true;
    }

  private:
    std::istream &in;
};

int main(int argc, char *argv[]) {
    std::clock_t start_time = clock();

    // Input from either a file or STDIN
    // http://stackoverflow.com/a/2159469/1157100
    std::shared_ptr<std::istream> input(&std::cin, [](...) -> void {});
    if (argc >= 2 && std::string("-") != argv[1]) {
        input.reset(new std::ifstream(argv[1]));
    }
    Input<int> triangle(*input);

    // For each entry in each row "b", add the greater of the two entries in
    // row "a" above.  Row "b" then contains the maximum path sum from the
    // top up to that point.
    std::vector<int> a, b;
    for (triangle.next_row(a); triangle.next_row(b); a = b) {
        b[0] += a[0];
        for (std::vector<int>::size_type i = 1; i < a.size(); i++) {
            b[i] += std::max(a[i - 1], a[i]);
        }
        b[b.size() - 1] += a[a.size() - 1];
    }
    std::cout << *std::max_element(a.begin(), a.end()) << std::endl;

    std::cerr << "Execution time - "
              << ((std::clock() - start_time) * 1000.0 / CLOCKS_PER_SEC)
              << "ms." << std::endl;
}


Flatten 2d-array at compiletime In my code, I need to flatten a two-dimensional std::array into a one-dimensional one at compile time. While writing the code I realized, that I am unsure about several aspects of my implementation (as far as readability is concerned).
Therefore I would really appreciate your input!
I am especially unsure, if

There is a more obvious / simpler version in which I can achieve my goal (I previously thought of expanding multiple parameter packs with one ... or the like)
if using a nested lambda is a good idea (I do this in order to obtain access to the parameter pack created by the integer_sequence and not expose this API)
If using the requires expression is really more readable than using a std::enable_if_t in this use case
and, even if I kind of like my solution to this problem, whether there is a cleaner way of

having automatic template parameter deduction for the "exposed" API
having access to sizes and type of the nested array

If there are other things I can improve with my code

#include <array>
#include <iostream>

/// Helper struct, which is std::false_type if the type is not a nested array, and otherwise holds
/// inner and outer array sizes.
template<typename Arr> struct NestedArray : std::false_type{};
template <std::size_t sizeOuter, std::size_t sizeInner, typename T>
struct NestedArray<std::array<std::array<T, sizeInner>, sizeOuter>> : std::true_type{
  static constexpr auto outer = sizeOuter;
  static constexpr auto inner = sizeInner;
  using type = T;
};

/// Return #nestedArray.flatten()[#index].
template <auto nestedArray, std::size_t index,
          typename = std::enable_if_t<NestedArray<std::decay_t<decltype(nestedArray)>>::value>>
consteval auto getValueByIndex() noexcept {
  using NestedArrayHelper = NestedArray<std::decay_t<decltype(nestedArray)>>;
  constexpr std::size_t innerIndex = index % NestedArrayHelper::inner;
  constexpr std::size_t outerIndex = index / NestedArrayHelper::inner;
  static_assert(NestedArrayHelper::inner > innerIndex &&
                    NestedArrayHelper::outer > outerIndex, "Index out of bounds.");
  return std::get<innerIndex>(std::get<outerIndex>(nestedArray));
}

/// Return 1-dimensional array #nestedArray.flatten() from 2-dimensional array #nestedArray.
/// @param nestedArray:   two-dimensional std::array (std::array<std::array<T, I>, O>)
/// @return               flat representation of #nestedArray (std::array<T, I * O>) with
///                       nestedArray[o][i] = result[i + I * o]
template <auto nestedArray>
requires requires() { requires NestedArray<std::decay_t<decltype(nestedArray)>>::value; }
consteval auto flattenNestedArray() noexcept {
  using NestedArrayHelper = NestedArray<std::decay_t<decltype(nestedArray)>>;
  using ContainedType = typename NestedArrayHelper::type;

  constexpr std::size_t flatSize = NestedArrayHelper::outer * NestedArrayHelper::inner;
  return []<std::size_t... ix>(std::index_sequence<ix...>) consteval noexcept {
    return std::array<ContainedType, flatSize> {getValueByIndex<nestedArray, ix>()...};
  }(std::make_index_sequence<flatSize>());
}

// Test, only for demonstrating the purpose of the code.

/// 2d array
constexpr std::array<std::array<int, 2>, 3> arrayOfPairs{{{1, 2}, {3, 4}, {15, 16}}};

/// Resulting 1d array
constexpr auto flatArray = flattenNestedArray<arrayOfPairs>();

// Test: Print out the resulting 1d array
int main () {
  for (std::size_t i = 0; i < 6; ++i) { std::cout << flatArray[i] << " "; } std::cout << std::endl;
}

I hope that this is appropriate for a Code Review, I linked the code in compilerExplorer. Note that it can only be compiled with gcc trunk, as clang trunk does not allow std::array as non-type template argument.
Thanks a lot :)

The biggest readability concern, in my opinion, is squashing everything on one line.  Compare:

int main () {
  for (std::size_t i = 0; i < 6; ++i) { std::cout << flatArray[i] << " "; } std::cout << std::endl;
}

to

int main()
{
    for (std::size_t i = 0; i < 6; ++i) {
        std::cout << flatArray[i] << " ";
    }
    std::cout << std::endl;
}

which can then be improved to:

int main()
{
    for (auto v : flatArray) {
        std::cout << v << ' ';
    }
    std::cout << '\n';
}

We can use some shorthand to make nested arrays easier to understand:

namespace detail {
    template <typename T, std::size_t... Dims>
    struct multi_array;

    template <typename T>
    struct multi_array<T> {
        using type = T;
    };

    template <typename T, std::size_t Dim, std::size_t... Dims>
    struct multi_array<T, Dim, Dims...> {
        using type = std::array<
            typename multi_array<T, Dims...>::type, Dim
        >;
    };
}

template <typename T, std::size_t... Dims>
using multi_array = typename detail::multi_array<T, Dims...>::type;

Now, it is more idiomatic to pass the array as an argument instead of a template argument, so that invocation uses more natural syntax and works for both runtime values and compile time values:

template <std::copy_constructible T, std::size_t N, std::size_t M>
constexpr auto flatten(const multi_array<T, N, M>& array)
    noexcept(std::is_nothrow_copy_constructible_v<T>)
{
    return /* magic */;
}

We can also easily generalize into multiple dimensions from here:

#include <array>
#include <concepts>
#include <type_traits>

namespace detail {
    template <typename T, std::size_t... Dims>
    struct multi_array;

    template <typename T>
    struct multi_array<T> {
        using type = T;
    };

    template <typename T, std::size_t Dim, std::size_t... Dims>
    struct multi_array<T, Dim, Dims...> {
        using type = std::array<
            typename multi_array<T, Dims...>::type, Dim
        >;
    };

    template <typename T>
    struct multi_array_traits {
        static constexpr std::size_t size{1};
        using type = T;
    };

    template <typename T, std::size_t N>
    struct multi_array_traits<std::array<T, N>> {
        static constexpr std::size_t size{N * multi_array_traits<T>::size};
        using type = typename multi_array_traits<T>::type;
    };

    template <std::size_t I>
    constexpr const auto& get(const auto& scalar) noexcept
    {
        static_assert(I == 0);
        return scalar;
    }

    template <std::size_t I, typename T, std::size_t N>
    constexpr const auto& get(const std::array<T, N>& array) noexcept
    {
        constexpr auto InnerSize = multi_array_traits<T>::size;

        constexpr auto Outer = I / InnerSize;
        constexpr auto Inner = I % InnerSize;

        return detail::get<Inner>(array[Outer]);
    }

    template <typename T, std::size_t... Indices>
    constexpr auto flatten(const T& array, std::index_sequence<Indices...>)
    {
        constexpr auto Size = multi_array_traits<T>::size;
        using Elem = typename multi_array_traits<T>::type;

        return std::array<Elem, Size>{Elem(detail::get<Indices>(array))...};
    }
}

template <typename T, std::size_t... Dims>
using multi_array = typename detail::multi_array<T, Dims...>::type;

template <std::copy_constructible T>
constexpr auto flatten(const T& array)
{
    constexpr auto Size = detail::multi_array_traits<T>::size;
    return detail::flatten(array, std::make_index_sequence<Size>{});
}

(live demo)


Codewars challenge - pair of shoes Yesterday you found some shoes in your room. Each shoe is described by two values:

type indicates if it's a left or a right shoe;
size is the size of the shoe.

Your task is to check whether it is possible to pair the shoes you found in such a way that each pair consists of a right and a left shoe of an equal size

For:

shoes = [[0, 21],
       [1, 23],
       [1, 21],
       [0, 23]]

the output should be true;

For:

shoes = [[0, 21],
       [1, 23],
       [1, 21],
       [1, 23]]

the output should be false.

So I wrote the code below,added some comments

 shoes = [[0, 21],
           [1, 23],
           [1, 21],
           [0, 23]];

  function pairOfShoes(shoes) {
        //replace 0 with -1 so I can reduce them to 0 later if they are a pair
        for (var i = 0; i < shoes.length; i++) {
            if (shoes[i][0] == 0) {
                shoes[i][0] = -1
            }
        }
        //reduce the array to object with shoe size as key
        function objectify(array) {
            return array.reduce(function(p, c) {
                p[c[1]] = (p[c[1]] != undefined ? p[c[1]] : 0) + c[0];
                return p;
            }, {});
        }

        var p = objectify(shoes);
        //check for each shoe size if the value is 0
        for (var x in p) {
            if (p[x] != 0) {
                return false;
            }
        }
        return true;
    }

 console.log(pairOfShoes(shoes));

Seems convoluted to me can this code be improved?

Convert your test values where you need them

Your initial for loop only sets each "right / left" value to -1 if it is 0. This can be done in the reduce function itself. Also, your conditional p[c[1]] != undefined is unnecessary, as undefined evaluates to false, and there is no concern here about other "falsy" values such as zero.

Use Javascript's array methods

.some() can eliminate unnecessary for loops with conditionals in Javascript.
.values() lets you concisely iterate through values of an object.

Combined with the arrow operator =>, these can result in concise code that reads a little more like a natural sentence.

Applying both of the above points to your code:

shoes = [[0, 21],
         [1, 23],
         [1, 21],
         [0, 23]];

function pairOfShoes(shoes) {
    var p = shoes.reduce(function(p, c) {
        p[c[1]] = (p[c[1]] || 0) + (c[0]==1 ? 1 : -1);
        return p;
    }, {});
    if (Object.values(p).some(x => x != 0)) { return false }
    else { return true };
}

console.log(pairOfShoes(shoes));

[Edit: It might also be clearer to replace your reduce function with a `for' loop.]


Video Editor Clean up I am making a video editor with tkinter, and seriously need to clean it up and make it nicer to read / more efficient. Can anybody help me clean up my code (I was sent here from stackoverflow)
Here it is: https://github.com/MiniMinnoww/moviepy-video-editor
The parts I kinda need to figure out are:

Buttons and popup windows. I need 2 functions to run 1 video editor function. For example, if I want to add text, I have to call a function to make a popup window for the details of the text, then call another function when the button is pressed to actually edit the video clip.

Here is what I tried to do (in simple terms):
def addText(self, x=0):
    if x == 0:
        button = Button(root, command=functools.partial(self.addText, 1))
    else:
        # run other stuff for function

Here you can see I am re-executing the function using an argument. Although this is effective, it is messy and I was wondering whether there's a better way to do this?

Deferring command execution with a simple lambda
It is very similar to what you did, but no need to bring in heavier artillery like functools.partial, especially because you have no idea who is going to read your code and they may not understand it :(
I'd go with
def addText(self, x=0):
    if x == 0:
        button = Button(root, command=lambda: self.addText(1))
    else:
        # run other stuff for function

If this still annoys you because of the recursive call, then consider:
def auxiliaryStuff(self, x):
    # run other stuff for function

def addText(self, x=0):
    if x == 0:
        button = Button(root, command=lambda: self.auxiliaryStuff(1))
    else:
        self.auxiliaryStuff(x)

I'm assuming x can take values other than 1 or 0 from other places in your code. If they are always either 1 or 0 and they just represent a flag, consider using True and False instead.


Checking login from associative array vulnerabilities For a very simple databaseless website, I'm using an associative array to store logins like this:

$logins = array(
'user' => 'password',
'user2' => 'password2',
);

I check login attemps using this code:

$user = strtolower($_POST['username']);
$pass = $_POST['password'];
if(!isset($logins[$user]) or $logins[$user] != $pass){
    // failed login
}else{
    $_SESSION['username'] = $user;
}

What would the security vulnerabilities in this be?

In this response, I assume that you are making a little script for yourself and some friends, i.e. not a large or critical system (e.g. that could delete other files). So you want some security, but not to extreme measures.
Passwords in Plain Text
As pacifist mentioned, storing passwords as clear text is always a bad idea. If you're just making a little page for yourself and some friends, at least store the passwords hashed with a salt (a random string per user). You could then store your login data the following way:
$logins = array(
 'user' => array('hash' => 'ae092...', 'salt' => 'a7#(ad~I$...'),
);

Checking for a valid login would then be used the following way (using PHP's crypt() to make SHA-256 hashes):
// Added trim() so something like "user " becomes "user"
$user = strtolower(trim($_POST['user']));
$pass = $_POST['password'];

if(isset($logins[$user]) && crypt($logins[$user]['salt'].$pass, 'SHA-256') == $logins[$user]['hash']) {
   // valid login!
}

This is not much more difficult and already provides you a little more safety. This is still not sufficiently safe for critical applications – you can read all about password hashing over at Security StackExchange. The linked answer covers a lot of aspects and is a great starting point.
Again, the above code is a simple way of handling logins for a small, non-critical page.
At the very worst, if you can't be bothered to give an individual salt to every user, add a global prefix/suffix to each password – a so-called pepper. You'll still be better off (preventing online rainbow table look-ups), because it doesn't take any knowledge at all to search the SHA-256 hash of "admin" and look up the results. However, searching the hash of admin!OTdf-u23;)(#$@_!#Rlf (where the text after admin would be your pepper) will most likely not return any results.
Brute Force
OK, but now let's assume your hashes/passwords don't ever leak. So, if I would want access, how would I proceed? I would just send probable passwords to the login form. You have no protection against abusive login attempts, so I can try as many passwords as I please. After the plain text storage of passwords, this is your weakest point, IMHO.
If it's a small, trivial script, it's probably not worth implementing anything to block that as no one will bother, though if there could be some substantial damage, it might be best to block invalid login attempts after a while. You know how things are now, but maybe you'll pick a fight with the wrong guy later on, or an automatic script will pick up on your login form.
A quick & dirty blocking mechanism is to keep an array of the number of invalid login attempts per IP address ($logins = array('127.0.0.1' => 3)). Once it reaches a certain number, you either block it permanently (if it's a small script and your friend is affected, he'll contact you) or you log the time and block it for a day or so.
Issues with Warnings
I recommend you output all warnings at least during development; this could help you find some issues. This can be done by adding error_reporting(E_ALL) to your login page.
A few issues that will arise is the direct reference to $_POST without checking first; use isset($_POST['user']) to see if the key you're asking for exists in the first place.
If you want to take it a step further, you could also do a check with is_scalar(), as sending an array as $_POST['pass'] will make crypt() issue a warning. This is something a lot of websites tend not to cover, which allows an attacker to find out the full path of your server and possibly the inner workings of your scripts (say, if the error comes from /hidden_include_folder/validator.php, this file will be shown in the error).
Working with Files
You need to account for concurrent file modifications. Again, if this is just a little fun script, it's a risk you can take, but anything of substantial size and/or importance should really be handled with a database like MySQL, which accounts for concurrent modifications and handles them for you automatically.
Note that keeping data in a file does not scale well; if you think the amount of data you're storing could grow a lot, consider setting up a database. I've seen it happen more than a few times that PHP reads a large file, then runs out of memory while writing to it, and the file ends up being empty – not a fun situation.


Expanding the region of interest of an image I am solving a problem of increasing the ROI of a retinal image using the below algorithm-

  First, the set of pixels of the exterior border of the ROI is determined, i.e., pixels that are outside the ROI and are neighbors (using four-neighbourhood) to pixels inside it. Then, each pixel value of this set is replaced with the mean value of its neighbors (this time using eight-neighbourhood) inside the ROI. Finally, the ROI is expanded by the inclusion of this altered set of pixels. This process is repeated and can be seen as artificially increasing the ROI.

My image is of following type-

and the mask on which I am applying the algorithm is below-

Pseudocode for the above approach which I got from here is below-

while there are pixels not in the ROI:
    border_pixels = []

    # find the border pixels
    for each pixel p=(i, j) in image
        if p is not in ROI and ((i+1, j) in ROI or (i-1, j) in ROI or (i, j+1) in ROI or (i, j-1) in ROI)):
            add p to border_pixels

    # calculate the averages
    for each pixel p in border_pixels:
        color_sum = (0, 0, 0)
        count = 0
        for each pixel n in 8-neighborhood of p:
            if n in ROI:
                color_sum += color(n)
                count += 1
        color(p) = color_sum / count

    # update the ROI
    for each pixel p=(i, j) in border_pixels:
        set p to be in ROI

and my implementation for the above pseudocode is below-

img = Image.open(path_dir)
pixelMap = img.load()
@jit
def roifun(img,pixelMap):
    roi = []
    for i in range(img.size[0]):
        for j in range(img.size[1]):
            if pixelMap[i,j] == 255:
                roi.append([i,j])

    return roi

roi= roifun(img,pixelMap)
notroi = img.size[0]*img.size[1] - len(roi)

@jit
def border_enhance(img,pixelMap,roi,notroi):
    while(notroi):
        border_pixels = []
        for i in range(img.size[0]):
            for j in range(img.size[1]):
                if [i,j] not in roi and ([i+1, j] in roi or [i-1, j] in roi or [i, j+1] in roi or [i, j-1] in roi):
                    border_pixels.append([i,j])

        for (each_i,each_j) in border_pixels:
            color_sum = 0
            count = 1
            eight_neighbourhood = [[each_i-1,each_j],[each_i+1,each_j],[each_i,each_j-1],[each_i,each_j+1],[each_i-1,each_j-1],[each_i-1,each_j+1],[each_i+1,each_j-1],[each_i+1,each_j+1]]
            for pix_i,pix_j in eight_neighbourhood:
                if (pix_i,pix_j) in roi:
                    color_sum+=pixelMap[pix_i,pix_j]
                    count+=1
            pixelMap[each_i,each_j]=(color_sum//count)

        for (each_i,each_j) in border_pixels:
            roi.append([each_i,each_j])
            border_pixels.remove([each_i,each_j])
            notroi = notroi-1
            print(notroi)

border_enhance(img,pixelMap,roi,notroi)

I run this code for image of dimension 50×50 and it is running correctly but for an image of larger size like 512×512, it is taking too long a time.
I also tried modifying it using numba but it gave me plenty of warnings and then taking the same time as before.

The secret to performance is to choose the appropriate data structures which lay out the memory as raw arrays of bytes (or uint8_t values). This is done by creating cython typed memoryviews from numpy arrays or images loaded through PIL. This applies to the border list as well; that is, I create an array of (x, y) coordinates rather than a list of python tuples (which is slow, not contiguous in memory, and requires conversions to access the data from python to C or vice versa. Below is the preliminary code that I have so far, hope this helps.

from libc.stdint cimport *
from libc.string cimport *
cimport libc.math as c_math
import numpy as np
from PIL import Image

cpdef uint8_t[:, :, :] load_image(str image_path):
    cdef uint8_t[:, :, :] image_data
    image = Image.open(image_path).convert("RGBA")
    image_np = np.array(image)
    image_np.setflags(write=1)
    image_data = image_np
    return image_data

cpdef save_image(uint8_t[:, :, :] image_data, str image_path):
    image = Image.fromarray(np.array(image_data)).convert("RGBA")
    image.save(image_path)

cpdef border_enhance(uint8_t[:, :, :] image, uint8_t[:, :, :] mask):
    cdef:
        uint8_t[:, :, :] out
        uint8_t[:] color
        uint8_t[:, :] roi
        uint32_t[:, :] border
        size_t i, j, k
        size_t w = image.shape[0]
        size_t h = image.shape[1]
        size_t num_border
        size_t num_not_roi = 0
        float avg[4]
        size_t avg_count
        size_t x, y
        size_t sx, sy
        int a, b

    roi = np.zeros((w, h), dtype=np.uint8)
    border = np.zeros((w * h, 2), dtype=np.uint32)#assumes no product overflow
    out = image[:, :, :]

    #define ROI from mask; only white pixels are part of the mask?
    for i in range(w):
        for j in range(h):
            color = mask[i, j]
            if color[0] == 255 and color[1] == 255 and color[2] == 255 and color[3] == 255:
                roi[i, j] = True
            else:
                roi[i, j] = False
                num_not_roi += 1

    while num_not_roi:

        #Create border
        k = 0
        for i in range(w):
            for j in range(h):
                if not roi[i, j]:
                    #assumes edges wrap over
                    if roi[i-1, j] or roi[i+1, j] or roi[i, j-1] or roi[i, j+1]:
                        border[k, 0] = i
                        border[k, 1] = j
                        k += 1

        for i in range(k):
            x = border[i, 0]
            y = border[i, 1]
            avg_count = 0
            avg = [0, 0, 0, 0]
            for a in range(-1, 2):
                for b in range(-1, 2):
                    sx = x+a
                    sy = y+b
                    if roi[sx, sy]:
                        #print(np.array(image[sx, sy]))
                        avg[0] += image[sx, sy, 0]
                        avg[1] += image[sx, sy, 1]
                        avg[2] += image[sx, sy, 2]
                        avg[3] += image[sx, sy, 3]
                        avg_count += 1
            for a in range(4):
                avg[a] /= avg_count
                out[x, y, a] = <uint8_t>c_math.round(avg[a])

        for i in range(k):
            x = border[i, 0]
            y = border[i, 1]
            roi[x, y] = True
            num_not_roi -= 1
            #dprint(num_not_roi)

    save_image(out, "./images/out.png")

cdef:
    uint8_t[:, :, :] image
    uint8_t[:, :, :] mask
    size_t count

image = load_image("./images/image.png")
mask = load_image("./images/mask.png")
border_enhance(image, mask)

That being said, the results do not seem particularly correct. The intermediate border step generates the correct pixel results, but the final result is wrong. Here is my output for the out.png after the call to the border_enhance function:


Arabic to Roman Number in Ruby on Rails I am making a class method in ruby to convert from roman number to Arabic and vice versa. However the loop is run to match each character and again to match with roman mapper. Can I optimize the same?

class String
      private

      def roman_mapping
        {
          1000 => "M",
          900 => "CM",
          500 => "D",
          400 => "CD",
          100 => "C",
          90 => "XC",
          50 => "L",
          40 => "XL",
          10 => "X",
          9 => "IX",
          5 => "V",
          4 => "IV",
          1 => "I"
        }
      end
    end

def to_arabic(str = self, result = 0)
  return result if str.empty?
  roman_mapping.values.each do |roman|
    if str.start_with?(roman)
      result += roman_mapping.invert[roman]
      str = str.slice(roman.length, str.length)
      return to_arabic(str, result)
    end
  end
end

The organization of your code seems strange to me: should to_arabic be class or instance method of the String class or maybe neither? The method lies outside of the String class bracket, yet references private hash roman_mapping and self. My JRuby 1.7.18 throws a runtime error while trying to execute it.

Then the code doesn’t properly handle invalid (non-roman) characters, it stops recursion because for the first invalid character the mapping will fail, in this case not the result of the canversion up to that point but the array of the roman_mapping’s values will be returned, which is supposedly not what you want to achieve.

After putting to_arabic within the class frame and adding public before it for the code to execute:

p "MCMPIII".to_arabic2

  ["M", "CM", "D", "CD", "C", "XC", "L", "XL", "X", "IX", "V", "IV", "I"]

Next you have a hash map but do not use it as such, just iterating over values. Even then you could use:

roman_mapping.each_pair do |value, roman|

to spare on the invert call.

At the end I’d suggest reverting the keys and values of the mapping hash (because you start from roman characters), using symbols instead of strings as keys and replacing recursion with a loop: like this:

class String
  private
  ROMAN_MAPPING =
    { :M => 1000, :CM => 900, :D => 500, :CD => 400,
      :C => 100, :XC => 90, :L => 50, :XL => 40, :X => 10,
      :IX => 9, :V => 5, :IV => 4, :I => 1 }.freeze

  public

  def to_arabic()
    return 0 if empty?

    str = self
    result = 0
    while !str.empty?
      roman = str[0..1]
      value = ROMAN_MAPPING[roman.to_sym]
      if !value.nil?
        result += value
        str = str[roman.length, str.length]
        next
      elsif roman.length == 2
        roman = str[0]
        value = ROMAN_MAPPING[roman.to_sym]
        if !value.nil?
          result += value
          str = str[roman.length..-1]
          next
        end
      end
      raise "Invalid roman character %s in #{self}" % roman
    end
    result
  end

end

Here exemplary outputs:

p "MCMIII".to_arabic

  1903

p "MCMPIII".to_arabic

  RuntimeError: Invalid roman character P in MCMPIII

I admit it is longer, but behaves better raising error on erroneous input and is faster – on my machine benchmarked against your code was regularely almost 3 times faster.

UPDATE

@200_success Thanks for asking - besides the performance gain there are two more reasons for using symbols instead of strings as keys.

Never use mutable objects as keys in hash tables

This is reason enough - mutable objects as keys are evil, if they change unpredictable things can happen, since values are placed in buckets of a hash table based on hashcode of the keys, now you can imagine that changing the keys of pair after it has been placed into a hash map is never a good idea. And strings in Ruby are mutable!

I admit though, that this will rather not happen in our case, yet it would be a bad practice anyway and should be avoided just to keep the code clean. And it seems that in fact Ruby copies and freezes the strings if used as keys in hash maps in case we forget about this rule :-). Nevertheless we have our performance gain by using symbols.

roman_mapping is in fact an immutable constant

Neither keys nor values would ever change. So it is logical to use an immutable class for the keys - like symbols. Now numbers are immutable, too, but the hash maps are not. So while we are talking this, we should make roman_mapping also immutable using freeze, and while doing this we can also replace the method roman_mapping with a frozen constant.

To make roman_mapping immutable all these steps are necessary - simple freezing the hash map wouldn't be enough. If a reference to its keys leaks into the outside word it still could be changed.

I've just updated the code above. A new benchmark on my machine shows now even more performance gain: the new code is more than 10 times faster than the original:

                user     system      total        real
new to_arabic   0.700000   0.010000   0.710000 (  0.717587)
old to_arabic   9.200000   0.050000   9.250000 (  9.427725)


Sharing $scopes across Controllers with ScopeFactory Previously, I use to always navigate through the $parent $scopes in order to send or change data from one Controller into/in another. I then created my own ScopeFactory as a way to storing each Controllers $scope and then getting those scopes from other Controllers.

Having not experienced any problems with using this method, I'm just curious if this is a good way to do it.

.factory('ScopeFactory', [function() {

    var notifications  = { that: this };
    var tagsPanel      = { that: this };
    var activityHeader = { that: this };

    var saveScope = function(type, vs) {
        switch(type) {
            case 'notifications':
                notifications.that = vs;
                break;
            case 'tagsPanel':
                tagsPanel.that = vs;
                break;
            case 'activityHeader':
                activityHeader.that = vs;
                break;
        }
    };

    var getScope = function(type) {
        switch(type) {
            case 'notifications':
                return notifications.that;
                break;
            case 'tagsPanel':
                return tagsPanel.that;
                break;
            case 'activityHeader':
                return activityHeader.that;
                break;
        }
    };

    return {
        saveScope : saveScope,
        getScope  : getScope
    };
}]);

Saving a scope:

// inside activityHeader Controller:
ScopeFactory.saveScope('activityHeader', vs);

Getting a scope from another Controller:

var activityHeader = {};

activityHeader = ScopeFactory.getScope('activityHeader');
activityHeader.chosenTicker   = initTicker;
activityHeader.companyName    = compName;
activityHeader.currentPrice   = price;
activityHeader.currentPercent = percent;
activityHeader.initialTickerLoaded(ticker);

I've finally refactored out the anti-pattern ScopeFactory out of my apps.
Instead I'm using $broadcast and $emit to accomplish what I'm trying to do, which is the more Angular way of doing this.

Example
Controller 1 (I $emit out the event and send a variable):
$rootScope.$emit("add.ticker.typing", word);
Controller 2 (I listen for the event and pick it up, including variables like objects or strings):
$rootScope.$on(
    "add.ticker.typing",
    function handleSearchTyping(event, word, option) {
        if (option != 'empty') {
            typingSearch(word);
        } else {
            $scope.tickersList       = [];
            $scope.tickersListLoaded = false;
        }
    }
);

Now with this, now only is it a true Angular pattern, but I'm not sending giant Controller $scope objects all around my app.


Build a flat List from a Tree structure in Java I have a Tree structure represented by a List, which I want to get it flattened.

I wrote the function below:

List<Entry> flatTree(List<Entry> toFlat, List<Entry> destination) {
    for (Entry entry : toFlat) {
        if (CollectionUtils.isNotEmpty(entry.getEntries())) {
            this.flatTree(entry.getEntries(), destination);
        } else {
            destination.add(entry);
        }
    }
    return destination;
}

used in this way:

List<Entry> flattenedEntries = this.flatTree(entries, new ArrayList<>());

with this kind of input (entries):

[
    {
        key: "level1.1",
        entries: []
    },
    {
        key: "level1.2",
        entries: [
            {
                key: "level2.1",
                entries: []
            }
        ]
    },
    {
        key: "level1.3",
        entries: [
            {
                key: "level2.2",
                entries: []
            },
            {
                key: "level2.3",
                entries: [
                    {
                        key: "level3.1",
                        entries: []
                    }
                ]
            }
        ]
    }
]

What I'm going to achieve is to have a list containing only objects with keys: level1.1, level2.1, level2.2, level3.1 or rather, all the elements that has no sub elements entries.

Each object has just 2 properties inside: key and entries.

It works, but I would like to avoid the side effect on the List<Entry> destination. Is there a better way to write it?

To answer the question from the comment on RoToRa's answer:

  Thanks! I wonder if there is any way to do this by using, for example, just the Java 8 Stream API or something similar, to have it in just one line of code.

Using the Java Stream API

Assuming the following Entry class:

public class Entry {
   final String key;
   final List<Entry> entries = new ArrayList<>();

   Entry(String _key, Entry ...children) {
      key = _key;
      entries.addAll(Arrays.asList(children));
   }

   public List<Entry> getEntries() {
      return Collections.unmodifiableList(entries);
   }

   public String toString() {
      return key;
   }
}

We can build up the example input with:

List<Entry> top = new ArrayList<>();
top.add(new Entry("level1.1"));
top.add(new Entry("level1.2", new Entry("level2.1")));
top.add(new Entry("level1.3", new Entry("level2.2"), new Entry("level2.3", new Entry("level3.1"))));

From @RoToRa's answer, we'll borrow these method signatures:

public static List<Entry> flatTree(List<Entry> toFlat) {
   return flatTree(toFlat, ArrayList::new);
}

public static List<Entry> flatTree(List<Entry> toFlat, Supplier<List<Entry>> listSupplier) {

Now, to flatten with the Stream API, (the requested "one line of code"), we'd want to write something like:

   return toFlat.stream().flatMap(Entry::leaves)
                         .collect(Collectors.toCollection(listSupplier));
}

So, we'll need to define Entry::leaves to extract only the leaves from our tree:

public static Stream<Entry> leaves(Entry entry) {
   if (entry.getEntries().size() > 0)
      return entry.getEntries().stream().flatMap(Entry::leaves);
   else
      return Stream.of(entry);
}

And on the sample data:

jshell> System.out.println(Entry.flatTree(top));
[level1.1, level2.1, level2.2, level3.1]

jshell>


A Sieve of Eratosthenes I have written this Sieve of Eratosthenes. This is a bit of software that will find all of the prime numbers between 2 and n, where n is a user-defined value.
The code does seem quite bloated and any help at making it more elegant would be great. I know that I am not very good at list comprehensions and if it is possible to utilise them more in this situation I would love to know how.
def sieve_of_eratosthenes(n):
    """
    Create a Sieve of Eratosthenes to find all of the prime
    numbers between 2 and n
    """
    n += 1

    num_list = []
    [num_list.append(i) for i in range(2, n)]

    test = 0
    while test < len(num_list):
        for i, x in enumerate(num_list):
            if num_list[i] % num_list[test] == 0:
                if num_list[i] != num_list[test]:
                    del num_list[i]
        test += 1
    return num_list

Agreed, this is not the Sieve of Eratosthenes. This is trial division by primes. (As long as you're doing trial division, you can do it slightly better, and only test up to the square root of the number.)
Replace the while loop, making it clearer and more pythonic: for test in num_list:
It is inefficient to delete from the middle of a list in python, which takes O(n) time.
It is generally unsafe, and will give wrong results to delete from an collection WHILE iterating through it. In this case it seems to work, but be careful. I would instead keep a binary flag for each number, with whether we've found a factor already or not (array of True/False).


Output string length and classical Fibonacci sequence in a web page I wrote a JavaScript and XHTML webpage. I think the format of the output is not better or the validation process is not as it is supposed to be. How can I improve the code? How can I make it more efficient?

There are two text boxes. First, it takes a sentence and returns the length. Second, it takes an integer and returns that number of classical Fibonacci numbers.

<!DOCTYPE html>

<titile></title>

<button id="lenClick" onclick="getStrLen()">String Length</button>
Enter a sentence or multiple ones: <input type="text" id="textbox1"></input>

Number of characters in this sentence is:

<input type="text" id="fibText"></input>
<button id="fibClick" onclick="outFib()">Generate Fibonacci Sequence</button>

Getting the length of a string

You split the string into characters and then you count the array's length. While this works, there's an even simpler way of doing it, using the length property of a String:

const length = msgDiv.value.length;

Changing getStrLen

You can cache the DOM elements. Then you don't need to re-query them, each time the function is called:

const strlenInput = document.getElementById('textbox1');
const strlenDisplay = document.getElementById('msg');

const getStrLen = () => {
    strlenDisplay.textContent = strlenInput.value.length;
}

Now this alone won't work in the head-element of your document, as both elements are not available yet. You can either:

wait until the DOM is ready to run that script, like document.addEventListener('DOMContentLoaded', function(event) { […] }, or
place the Javascript at the end of your page instead

This is explained in detail here: $(document).ready(function(){}); vs script at the bottom of page.

Creating the Fibonacci sequence

genFibonacci() takes one parameter, so you pass an unused parameter, when calling the function recursively:

genFibonacci(times - 1, false);

There's a flaw in your logic: You don't reset the values each time the button is clicked. If the user's input is 1 and they hit the generate button 5 times, the result is not 0 but:

 0 1 1 2 3

With this in mind, let's try to change your algorithm, that it won't rely on external variables. We also don't necessarily need recursion here:

const getFibonacciValues = (n) => {
    const values = [];

    if (n >= 1) {
        values.push(0);
    }

    if (n >= 2) {
        values.push(1);
    }

    for (let i = 2; i < n; ++i) {
        values.push(values[i - 2] + values[i -1]);
    }

    return values;
}

Now we have a function, which purpose is solely to create the values. It doesn't have a clue, how you're going to output them. outFib() is doing this for us now:

const outFib = () => {
    const n = fibInput.value;
    const values = getFibonacciValues(n);
    fibDisplay.textContent = values.join(', ');
}

Now, this still has flaws, so we add a little bit of validation at least:

use parseInt to parse the value
check whether the input is at least 1
check whether the input is a number

The final result could look like this:

const fibButton = document.getElementById('fibClick');
const fibInput = document.getElementById('fibText');
const fibDisplay = document.getElementById('fibseq');

const getFibonacciValues = (n) => {
    const values = [];

    if (n >= 1) {
        values.push(0);
    }

    if (n >= 2) {
        values.push(1);
    }

    for (let i = 2; i < n; ++i) {
        values.push(values[i - 2] + values[i -1]);
    }

    return values;
};

const outFib = () => {
    const n = parseInt(fibInput.value);

    if (n < 1 || isNaN(n)) {
        fibDisplay.textContent = 'Please insert a number greater than 0';
        return;
    }

    const values = getFibonacciValues(n);
    fibDisplay.textContent = values.join(', ');
};

fibClick.addEventListener('click', outFib);
<input type="text" id="fibText">
<button id="fibClick">Generate Fibonacci Sequence</button>

Further improvements

This will fail for large inputs – try 2000 for example. You'll get a lot of Infinity.

You can take a look into bigNumber.js or you can set an upper limit, like if (n > 1000) { fibDisplay.textContent = 'Too large'; return; } or you could store all values for your maximum n.

Calculating all numbers over and over again is some overhead you could avoid. You could cache already calculated values, and only add new values if n > values.length. Alternatively you can store all numbers beforehand, which is an increase in memory usage, but your runtime becomes incredibly fast, as all you have left is:

const values = [0, 1, 1, 2, 3, …];
const output = values.splice(0, n);


Bash script to rsync website I'm new to bash scripting. I have pieced together a bash script to rsync my website files and databases.

Any guidance on ways I could improve or make my script more secure would be greatly appreciated.

#!/bin/bash

set -e

site_host=(
    "test1@grid.co.uk"
    "test2@grid.co.uk"
)

backup_dest=(
    "/Users/computername/Desktop/rsync/site1.co.uk"
    "/Users/computername/Desktop/rsync/site2.co.uk"
)

now=`date "+%d/%m/%Y %H:%M:%S"`
today=`date +"%d-%m-%Y"`
yesterday=`date -v -1d +"%d-%m-%Y"`
backup_old=`date -v -2d +"%d-%m-%Y"`

log="/Users/computername/Desktop/rsync/rsync.log"

site_count=${#site_host[@]}

db_creds=()
db_creds+=("")
db_creds+=("localhost,user,pass,dbname")

for (( i = 0; i < site_count; i++ )); do

    # Source and destination
    site_source="${site_host[$i]}:~/public_html"
    site_dest="${backup_dest[$i]}/$today/"

    # DB creds
    for val in ${db_creds[$i]}; do
        subset=($(echo $val | tr ',' ' '))
        db_host=${subset[0]}
        db_user=${subset[1]}
        db_pass=${subset[2]}
        db_name=${subset[3]}
    done

    if [ "$db_host" ]; then

        # Set DB seperate directory
        mkdir -p ${backup_dest[$i]}/$today/db

        # Database backup
        ssh -p22 ${site_host[$i]} "mysqldump \
                --host=$db_host \
                --user=$db_user \
                --password=$db_pass \
                --databases $db_name \
                --lock-tables \
                | bzip2" > ${backup_dest[$i]}/$today/db/$today.sql.bz2

        echo "$now - SUCCESS - DB Backup - ${backup_dest[$i]}/$today/db/$today.sql.bz2" >> $log

    fi

    # Remove old backups
    if [ -d "${backup_dest[$i]}/$backup_old" ]; then

        rm -Rf ${backup_dest[$i]}/$backup_old;

        echo "$now - DELETED - Full Backup - ${backup_dest[$i]}/$backup_old" >> $log

    fi

    if  rsync -zavx -e 'ssh -p22' \
            --numeric-ids \
            --delete -r \
            --link-dest=../$yesterday $site_source $site_dest;
    then

        echo "$now - SUCCESS - File Backup - ${backup_dest[$i]}/$today" >> $log

    else

        echo "$now - FAILED - File Backup - ${backup_dest[$i]}/$today" >> $log

    fi

done

good

yay for specifying bash in your #! line.
nice indentation of loops and conditionals
good variables names
Thanks for splitting long commands onto multiple lines, which are also nicely indented.  (Another way to to do this is to built up the command options in a variable and then pass that as the command argument.)

suggestions

I'm a big fan of https://www.shellcheck.net/ which is basically a linter for shell scripts.  Don't worry about fixing all of its suggestions, but most of them are worth considering.  My next few suggestions are based on its output.
Using backticks (`) for command substitution is not the current best practice.  Using $() for command substitution is nicer since it matches the () used for subshells and it stands out a bit more.  Lots of folks were confused by quotes being used for non-quoting things.  SC2006 mentions a few other reasons.
When you do mkdir -p ${backup_dest[$i]}/$today/db on line 46, if one of the backup_dest had spaces in it you'd end up making two directories instead of one.  It is best when doing variable substitutions in the shell to enclose them in double quotes.  mkdir -p "${backup_dest[$i]}/$today/db" is safer.  SC2086 explains more.
Are you expecting more db_creds in the future?  The for loop at line 35 seems superfluous.  Even if you had 99 entries in db_creds it is only going to set the db_* variables for the last one.  If you want to do something for each one you'd have to put more of the work inside the loop.  Or am I missing some intent here?
checking to make sure you got a db_host in line 43 is a good idea, but should there be an else do this that warns about it being missing?
does it matter if any of the stuff before the rsync fails?  Do you want to exit with FAILED for anything before the rsync?
putting a bit more documentation in the code would be better, but the comments that are there are good.

Overall, an excellent job for someone new to shell scripting.


Ruby Tic-Tac-Toe using classes This is Tic-Tac-Toe using negamax for AI. Help me make it better!

Some sore points are:

def get_mark
    good_mark = false
    until good_mark
      mark = gets.chomp
      if mark =~ /x/i
        chose_x
        good_mark = true
      elsif mark =~ /o/i
        chose_y
        good_mark = true
      else
        puts <<-EOS.gsub(/^ */, '')

        What is this strange mark? please choose 'X' or 'O'!
        EOS
      end
    end
  end

and

def game_won?
  @solutions.clear
  make_solutions
  won = false
  @solutions.each do |solution|
    if solution[0] != ' ' &&
      solution[0] == solution[1] && solution[1] == solution[2]
    won = true
    end
  end
  return won
end

The rest of this is here.

Some suggestions:

Avoid flags. Oftentimes there are a hint that you can structure your code better
Learn the available language constructs and methods (e.g. case/when or #any?)
Avoid methods with side effects when you can better use a functional style. (E.g. a implement solutions method returning the solutions instead of a make_solutions method which modifies an instance variable with even has to be initialized before the call.) Among other things this improves reusability. You can still cache results in instance variables if needed.

Considering this you can write your provided code snippets a little neater:

def get_mark
  while (mark = gets.chomp)
    case mark
    when /x/i then chose_x
    when /o/i then chose_y
    else
      puts <<-EOS.gsub(/^ */, '')

      What is this strange mark? please choose 'X' or 'O'!
      EOS
      continue
    end
    break
  end
end

def game_won?
  solutions.any? do |solution|
    solution[0] != ' ' && solution[0] == solution[1] && solution[1] == solution[2]
  end
end

I didn't dig through your git repo though. If you have more specific questions you should ask them.


New svn repository script This is my very first Bash script that does something real. I'm here to learn, so please tell me if and how this can be done better.

#!/bin/sh

# Daniele Brugnara
# October - 2013

if [ -k $1 ]; then
        echo "Error! You must pass a repository name!"
        exit 1
fi

SVN_PATH="/var/lib/svn"
currPath=$(pwd)

cd $SVN_PATH
svnadmin create $1
chown -R www-data:www-data $1
cd $currPath

echo "Done! Remember to set valid user permission in authz file."

If the first argument to your script contains a space, then you get an error message:

$ ./cr32076.sh "my repository"
./cr32076.sh: line 6: [: my: binary operator expected

To avoid this, put quotes around the variable $1, like this:

if [ -k "$1" ]; then

The man page for [ says:

-k file       True if file exists and its sticky bit is set.

is this really what you mean? It seems more likely that you want the -z option:

-z string     True if the length of string is zero.

You remember the current directory in a variable:

currPath=$(pwd)

and then later change to it:

cd $currPath

but this will go wrong if the current directory has a space in its name:

$ pwd
/Users/gdr/some directory
$ currPath=$(pwd)
$ cd $currPath
bash: cd: /Users/gdr/some: No such file or directory

To avoid this, put quotes around the variable, like this:

cd "$currPath"

If you want to change directory temporarily and change back again, then instead of writing

currPath=$(pwd)   # remember old directory
cd "$SVN_PATH"    # change to new directory
# do some stuff
cd "$currPath"    # go back to old directory

you can use a subshell:

(
    cd "$SVN_PATH"    # change to new directory
    # do some stuff
) # old directory is automatically restored when subshell exits

But in this script, there's no need to change directory back again, because after you go back all you do is echo "Done! ..." and it doesn't matter what directory you do that in.
In fact, there's no need to change directory at all, because instead of

SVN_PATH="/var/lib/svn"
currPath=$(pwd)

cd "$SVN_PATH"
svnadmin create "$1"
chown -R www-data:www-data "$1"
cd "$currPath"

you could write

SVN_PATH="/var/lib/svn/$1"
svnadmin create "$SVN_PATH"
chown -R www-data:www-data "$SVN_PATH"


Injections and query I made a class that connects to my DB and inserts some values. Is it secure or how can I protect this further from injections? The object declaration will come from variables with POST from a form, after being validated, ofc. Just want to know if this is secure.

That, and also, should I make a method for every query I need? Or is there a better and secure way?

<?php
include "db/db_info.php";
/*$DBServer
$DBUser
$DBPass
$DBName*/
class WorkDB {
    private $server;
    private $user;
    private $pass;
    private $name;
    private $conn;

    public function __construct( $server, $user, $pass, $name ) {
        $this->server=$server;
        $this->user=$user;
        $this->pass=$pass;
        $this->name=$name;
    }

    public function tryconn() {
        $this->conn = new mysqli(  $this->server, $this->user, $this->pass, $this->name );

    if ( mysqli_connect_error()) {
        die( '*************Connection Error (' . mysqli_connect_errno()  . '):'
                .mysqli_connect_error() );
    }

    else echo 'ok';

}

public function query_register( $user, $pass, $email ) {
    $stmt = $this->conn->prepare( "INSERT INTO `users` (`username`, `password`, `email`) VALUES (?, ?,?)" );
    $stmt->bind_param( "sss", $user, $pass, $email );
    $stmt->execute();
    $stmt->close();
}

}//end of class

$a=new WorkDB( $DBServer, $DBUser, $DBPass, $DBName );
$a->tryconn();
$a->query_register( 'a', 'b', 'c' );

?>

What you did well

Storing sensitive information, such as the database connection parameters, separately from the source code is a good idea.
You used parameterized queries with placeholders, which are not vulnerable to SQL injection.

Things to work on

Your indentation is inconsistent.
tryconn() should not echo 'ok' on success, as that pollutes the output.
$a is a horrible name for your database handle.  It's conventionally called $db or something like that.


Including references in onResume Lifecycle What's a more concise way to do this? It's way too verbose for my taste.

    @Override
    protected void onResume() {
    super.onResume();
    Thread.currentThread().setPriority(Thread.MAX_PRIORITY - 1);
    TinyDB tinydb = new TinyDB(this);
    Log.v(TAG + "BaseStartup.onResume", "onResume");
    IntentFilter intentFilter = new IntentFilter();
    intentFilter.addAction("tv.apps.v4.ve");
    intentFilter.addAction("tv.apps.v4.ev");
    intentFilter.addAction("tv.apps.v4.we");
    intentFilter.addAction("tv.apps.v4.wef");
    intentFilter.addAction("tv.apps.v4.wefew");
    intentFilter.addAction("tv.apps.v4.gwe");
    intentFilter.addAction("tv.apps.v4.ewew");
    intentFilter.addAction("SMS_ewe");
    intentFilter.addAction("INTERNET_BACK_UPweeew");
    intentFilter.addAction("tv.apps.v4.weewwee");
    intentFilter.addAction("tv.apps.weewe");
    intentFilter.addAction("tv.apps.weet");
    intentFilter.addAction("tv.apps.weewewewees");
   // Get source of open for app re-engagement
    mobileAppTracker.setReferralSources(this);
    // MAT will not function unless the measureSession call is included
    mobileAppTracker.measureSession();
    //sceneTransport("resumeSession", "null", "null" );

}

You could use a loop to reduce the duplication:

List<String> extensions = Arrays.asList(
    "ve",
    "ev",
    "we",
    "wef"
    // ...
);
for (String ext : extensions) {
    intentFilter.addAction("tv.apps.v4." + ext);
}

But actually, it would be better to move hardcoded strings outside of java code.
You could create a res/values/arrays.xml file like this:

<?xml version="1.0" encoding="utf-8"?>
<resources>
    <string-array name="actions">
        <item>tv.apps.v4.ve</item>
        <item>tv.apps.v4.ev</item>
        <item>tv.apps.v4.we</item>
        <!--
        <item>...</item>
        -->
    </string-array>
</resources>

And use it in the code like this:

for (String action : getResources().getStringArray(R.array.actions)) {
    intentFilter.addAction(action);
}

Does this work as intended?

Thread.currentThread().setPriority(Thread.MAX_PRIORITY - 1);

I don't know about this particular usage,
but I do know that low level thread operations are not recommended in Android development in general.

Unless this usage is explicitly mentioned somewhere in the official guides,
I'm not sure if this will work reliably at all.


Function to get current value or default from a string of two values I have SQL scripts that I want to be executable directly in the editor (like DataGrip or SSMS) and also in automated integration tests. However, in each use-case they require different environment and version parameters. I cannot use string_split because we use an ancient SQL Server 2012.

In order to differentiate between them, I created the GetCurrentOrDefault function that can either use the first or the second value.

I start by finding the index of the | that I use as a separator. Then I get the string before and after this character. Finally, I check whether the string has the format '{%}|%' and if yes, then I use the default version, otherwise the current one. (All magic-numbers are arbitrary primes.)

if object_id('dbo.GetCurrentOrDefault') is not null drop function GetCurrentOrDefault

go

create function GetCurrentOrDefault(@valueOrDefault nvarchar(59)) returns nvarchar(17)
begin
    declare @current_value as nvarchar(17);
    declare @default_value as nvarchar(17);
    declare @value_separator_index as int

    select @value_separator_index = charindex('|', @valueOrDefault)

    if @value_separator_index = 0 return null

    select @current_value = substring(@valueOrDefault, 0, @value_separator_index)
    select @default_value = substring(@valueOrDefault, @value_separator_index + 1, len(@valueOrDefault) - @value_separator_index + 1)
    return iif(@valueOrDefault like '{%}|%', @default_value, @current_value)
end

go

Usage example:

declare @env as nvarchar(51) = N'{Environment}|production'
declare @ver as nvarchar(59) = N'{Version}|3.9.0'

select @env = dbo.GetCurrentOrDefault(@env)
select @ver = dbo.GetCurrentOrDefault(@ver)

if @env is null raiserror ('Invalid environment: ' + @env, 16, 1)
if @ver is null raiserror ('Invalid version: ' + @ver, 16, 1)

-- many many inserts with settings ...

Tests replace the placeholders with their custom values like:

.GetSql().Format(new { Environment = "test", Version = "4.0.0" }) // C#

What do you think? Is there a more clever solution?

Potential Issues

a naive string replacement could change more than just the placeholders; for instance, when another part of the script uses the same string as a literal
the replacement can introduce a value with the same format {%}; your sql has no way of knowing that this is a replacement value, and since the format matches that of a place holder, it will be ignored and the default value will be taken
make sure the sql user of your unit test project has limited rights, because you don't know the exact sql that is going to run at runtime

If you insist on using a string replacement, try to manage escape characters and try to avoid false positives/negatives, both from sql and C# injection.


ROT13 implementation I'm doing some coding challenges to get better practiced at my coding.  I'm trying to do one that requires us to do a ROT13.  I've got the implementation correct, and I just want to know a couple of things.

The space character is showing up as a crazy character in my terminal screen, which I'm assuming is because of character encoding, although I could be wrong.  Also, how does my code look?

#include <iostream>
#include <string>
using namespace std;

const int LOWER_A = 97;
const int LOWER_M = 109;
const int LOWER_N = 110;
const int LOWER_Z = 122;

const int UPPER_A = 65;
const int UPPER_M = 77;
const int UPPER_N = 78;
const int UPPER_Z = 90;

string rot(string input) {
    int inputSize = input.size();
    int index = 0;

    while(index != inputSize) {
        if(input[index] >= LOWER_A && input[index] <= LOWER_M)
            input[index] = input[index] + 13;
        else if(input[index] >= LOWER_N && input[index] <= LOWER_Z)
            input[index] = input[index] - 13;
        else if(input[index] >= UPPER_A && input[index] <= UPPER_M)
            input[index] = input[index] + 13;
        else if(input[index] <= UPPER_N && input[index] <= UPPER_Z)
            input[index] = input[index] - 13;

        index++;
    }
    return input;
}

int main() {
    string plaintext;
    string cypher;

    cout << "input: ";
    getline(cin,plaintext);
    cypher = rot(plaintext);
    cout << cypher << endl;

    return 0;
}

A few fairly minor things:

Constants for the characters

Instead of using the constants, you can just use a character inline:

if(input[index] >= 'a' && input[index] <= 'm')

It's worth noting though that a literal character will be a char rather than an int like your constants are (won't affect anything in this program though).

using namespace std;

It's fine in short programs like this, but as you hinted that you're fairly new to C++, I wanted to mention that this can be a bad habit to get into.  In particular, in large applications with many different segments of code, this can cause naming clashes.

This question on SO addresses the problems it can cause.

Naming

I would probably call rot, rot13 since there are other kinds of rotations.  This is mainly me just being overly picky though :).

Strange Character For Space

  The space character is showing up as a crazy character in my terminal screen, I'm assuming it's because of character encoding, although I could be wrong.

rot13 traditionally only operates on alpha characters (a-z and A-Z), so your space characters should not be changing.

I believe your program has a bug in it:

else if(input[index] <= UPPER_N && input[index] <= UPPER_Z)

If this is not a bug, you should know that  this will be always be true if input[index] <= UPPER_N, so really there's no point in the second conditional.

Anyway, I think what's happening is that the space character (and anything under 'N') is falling into this block when it shouldn't be.

Space is ASCII 32, so it's getting changed to 19, which is not a printable character.

while vs for

Your loop seems to fit the construct of a for loop very well, so I might consider using that:

for (int inputSize = input.size(), index = 0; index != inputSize; ++index) {

}

This comes down to personal preference though.

You should usually try to match the return type of a method

std::string::size() doesn't return an int; it returns a std::string::size_type which is always (in every implementation I've ever seen anyway), a size_t (which is in turn an unsigned 32 or 64 bit integer depending on the platform and compiler).

Anyway, when working with standard containers (or really APIs in general), unless you have a reason to use a different type, I would try to stick with the return type specified.  For example:

for (std::string::size_type len = input.size(), idx = 0; idx != len; ++idx) {
    //input[idx] = ...;
}

A potential implementation

I suspect that the if-else tree can be simplified a bit using isalpha and family, but this is what I might implement your algorithm like;

std::string rot13(std::string input) {

    for (std::string::size_type len = input.length(), idx = 0; idx != len; ++idx) {
        if (input[idx] >= 'a' && input[idx] <= 'm') {
            input[idx] = input[index] + 13;
        } else if (input[idx] >= 'n' && input[idx] <= 'z') {
            input[idx] = input[idx] - 13;
        } else if(input[idx] >= 'A' && input[idx] <= 'M') {
            input[index] = input[index] + 13;
        } else if(input[idx] >= 'N' && input[idx] <= 'Z') {
            input[index] = input[index] - 13;
        }
    }

    return input;

}

To get a little carried away for a moment, you could also implement it using iterators.  Using iterators would allow you to apply it to any container that implements iterators (all of the standard containers and plain pointers).

template <typename Iter>
void rot13(Iter begin, const Iter& end) {

    while (begin != end) {

        //Doesn't need to be here, but I'm lazy and don't like
        //typing *begin over and over again.
        char& c = *begin;

        if (c >= 'a' && c <= 'm') {
            c += 13;
        } else if (c >= 'n' && c <= 'z') {
            c -= 13;
        } else if (c >= 'A' && c <= 'M') {
            c += 13;
        } else if (c >= 'N' && c <= 'Z') {
            c -= 13;
        }

        ++begin;

    }

}

Note that this modifies a container in place rather than creating a copy:

char str[] = "Hello World";
rot13(str, str + strlen(str));
std::cout << str << std::endl;
rot13(str, str + strlen(str));
std::cout << str << std::endl;

Would output:

Uryyb Jbeyq
Hello World

You could of course modify it to operate on a copy instead though.


Retrieving values from Entity Framework DbContext to render in a ListBox I need to pass a ViewModel to a strongly typed View which will display a multiple-select box using Html.ListBoxFor. My ViewModel has a List<int> for the selected values and an IEnumerable<SelectListItem> for the options.

Is there a better way I could be enumerating the list of options?

private MyDbContext db = new MyDbContext();

public List<int> SelectedPhysicians { get; set; } = new List<int>(1) { 1 };

public IEnumerable<SelectListItem> AllPhysicians
{
    get
    {
        List<SelectListItem> temp = new List<SelectListItem>();
        foreach (PhysicianModel p in db.Physicians)
        {
            temp.Add(new SelectListItem { Value = p.Id.ToString(), Text = p.Name });
        }
        return temp;
    }
}

Also, can you see any long-term reasons I would even need a List<int> rather than an int[]? I tend to go with List because I almost always end up needing more than basic array operations. In this case, however, I'm just retrieving and storing sets of values in the database.
It's occurred to me that all of my models are using Lists for no tangible reason, but I want to be sure before refactoring.

You might as well just use LINQ to project the collection to SelectListItem

return db.Physicians.Select(new SelectListItem { Value = p.Id.ToString(), Text = p.Name });

As far as lists and arrays are concerned there is actually only one main difference betwen them: arrays cannot change its size. Use what is more conveninet. If you have fixed collection of items the I'd probably use an array an in all other cases I guess I'd chose a list.

Alternatively you can use ICollection<T> as a return type and since most collections implement this interface it doesn't matter what the underlying type is.


Alphabet Rangoli Challenge The question is about printing alphabets in a rangoli pattern of size/dimension input by the user.
Example:
size 3

----c----
--c-b-c--
c-b-a-b-c
--c-b-c--
----c----

The picture contains details about the challenge
OR
https://www.hackerrank.com/challenges/alphabet-rangoli/problem
Sample Code:
n = int(input('Enter a size: '))

alpha = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']

for j in range(0, n-1):
    ls_1 = str(''.join(alpha[n-1:abs(n-(j+2)):-1]))
    ls_1 = ls_1 + ls_1[-2::-1]
    ls = str('-'.join(ls_1))
    print('-' * (((n - j) * 2) - 2) + ls + '-' * (((n - j) * 2) - 2))

ls_1 = str(''.join(alpha[n-1::-1]))
ls_1 = ls_1 + ls_1[-2::-1]
ls = str('-'.join(ls_1))
print(ls)

for j in range(n-2, -1, -1):
    ls_2 = str(''.join(alpha[n-1:abs(n-(j+2)):-1]))
    ls_2 = ls_2 + ls_2[-2::-1]
    ls_s = str('-'.join(ls_2))
    print('-' * (((n - j) * 2) - 2) + ls_s + '-' * (((n - j) * 2) - 2))

Sample Output:
Enter a size: 5
--------e--------
------e-d-e------
----e-d-c-d-e----
--e-d-c-b-c-d-e--
e-d-c-b-a-b-c-d-e
--e-d-c-b-c-d-e--
----e-d-c-d-e----
------e-d-e------
--------e--------

Is the above code that I have done...is it a good code to say?
Also, I couldn't find any other way to solve the problem.
(I am a beginner in Python)

Use built-ins
alpha = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']

This is a very verbose and error-prone way of getting all of the ASCII lowercase letters.
from string import ascii_lowercase as alpha

will give approximately the same result.  It is a string, instead of a list, but Python strings are effectively just lists of characters.  In particular, alpha[0] will be 'a' and alpha[25] will be 'z'.
Unnecessary str()
The result of a ''.join(...) call will be a string.  Wrapping this in a str(...) call is pointless.
Unnecessary abs()
for j in range(0, n-1) means that j will always be less than n-1.  In other words:
$$ j \le n - 2 $$
Consider, abs(n-(j+2)):
$$ j \le n - 2$$
$$ j + 2 \le n$$
$$ n - (j+2) \ge 0$$
In other words, the abs(...) is unnecessary, and just adds confusion.
General Comments
Follow PEP-8 guidelines (spaces around operators, etc).  Use functions.  Use a main-guard.
Reworked & Simplified Code
from string import ascii_lowercase

def print_rangoli(n: int) -> None:

    alpha = ascii_lowercase[:n]

    for row in range(- n + 1, n):
        row = abs(row)
        dashes = "-" * (2 * row)
        print(dashes + "-".join(alpha[:row:-1] + alpha[row:]) + dashes)

if __name__ == '__main__':
    n = int(input("Enter a size: "))
    print_rangoli(n)

Format Specification Mini-Language
Python's format statement (and f-strings) use a format specification mini-language.  This allow you substitute values into a larger string in fixed-width fields, with your choice of alignment and fill characters.  Here, you'd want centre alignment, with '-' for the fill character.
For example, f"{'Hello':-^11}" is a f-string, which places the string 'Hello' into a field 11 characters wide, centre justified (^), with '-' used as a fill character.  Producing '---Hello---'.  Instead of a hard-coded width (11 in the above example), we can use a computed {width} argument.
Using this, we can further "simplify" (for some definition of simplify) the print_rangoli function:
def print_rangoli(n: int) -> None:

    width = n * 4 - 3
    alpha = ascii_lowercase[:n]

    for row in range(- n + 1, n):
        row = abs(row)
        print(f"{'-'.join(alpha[:row:-1] + alpha[row:]):-^{width}}")


RAII wrapper for SQLite transactions I'm using this nice C++ wrapper to work with SQLite3 in my project. I wanted to handle errors gracefully and automatically rollback if piece of code fails, so wrote this simple helper class:

class SqlTransaction { // RAII class to rollback on error
    public:
        SqlTransaction(std::shared_ptr<sqlite::database> db_): db(db_) { *db << "begin;"; };
        ~SqlTransaction() { if (!finished) rollback(); };
        void commit() { if (!finished) { *db << "commit;"; finished = true; } };
        void rollback() { if (!finished) { *db << "rollback;"; finished = true; } };

        // Disable both copying and moving
        SqlTransaction(const SqlTransaction&) = delete;
        SqlTransaction& operator=(const SqlTransaction&) = delete;
        SqlTransaction(SqlTransaction&&) = delete;

    private:
        std::shared_ptr<sqlite::database> db;
        bool finished = false;
    };

It's to be used like this:

SqlTransaction trans(db);
// work with db here, may throw if something goes wrong
trans.commit();

There's not a lot of code here to be reviewed, but I'll have a go.

A small efficiency gain is possible, by moving the db_ argument in the initializer list, rather than copying it:

SqlTransaction(std::shared_ptr<sqlite::database> db_)
  : db{std::move(db_)}
{ *db << "begin;"; };

It's not necessary to delete the move constructor, as explicitly deleting the copy constructor prevents the move constructor being implicitly provided.  However, if you feel that doing so improves clarity, you should probably delete move assignment, too.
We should document that the class isn't thread-safe.  It's quite reasonable that we should use it from only one thread, but we need to be clear to our users about that.  Alternatively, we could make it thread-safe by making finished an std::atomic<bool> and using its test-and-set method, exchange().
Since rollback() tests finished, there's no need to duplicate that in the destructor - just call rollback() unconditionally.  It may be worth taking steps to avoid it throwing when called there - destructors that throw need to be handled with extreme care.

~SqlTransaction() { try { rollback(); } catch (...) { /* ignore */ } };

I have nothing specific to SQLite, as I've not used that library myself (I'm surprised it has to parse string commands, rather than having methods for those operations, though).


ReactJS component to classify a list of books In my project, I am getting an array of objects from api, and I want to divide the array into 3 three arrays based on the value of the shelf property of each
array element which is an object. Now for doing that I am using formatBooks function inside the same component.

Later on I am passing this formatBooks function as prop to other component which calls this function.

I was hoping if there is any better by making another component and place the formatBooks inside that. I wanted to know a better and elegant way of doing things like this.

class BooksApp extends React.Component {
  state = {
    books: [],
    searchResults: [],
    newBooksFromSearch: [],
  }
  //format the books array
  formatBooks = () => {
    const currentlyReading = []
    const read = []
    const wantToRead = []
    this.state.books.forEach( item => {
      if(item.shelf === "currentlyReading" || item.shelf === "Currently Reading"){
          item.setShelf = "Currently Reading"
          currentlyReading.push(item)
      }
      if(item.shelf === "read" || item.shelf === "Read" ){
          item.setShelf = "Read"
          read.push(item)
      }
      if(item.shelf === "wantToRead" || item.shelf === "Want to Read"){
          item.setShelf = "Want to Read"
          wantToRead.push(item)
      }
    })
    return(
      {
        currentlyReading: currentlyReading,
        read: read,
        wantToRead: wantToRead
      }
    )
  }

You may start to make your code slightly less verbose (and with all the required semicolons):

const result = {
    currentlyReading: [],
    read: [],
    wantToRead: []
};

this.state.books.forEach(item => {
    if (...) {
        item.setShelf = "Currently Reading";
        result.currentlyReading.push(item);
    }
});

return result;

Then you might simplify comparison using regular expressions:

if (item.shelf.match(/^currently\s?reading$/i)) { ... }

Now you can see that you are MUTATING THE STATE without using setState(). That's an Incredibly Bad Thing™. You do not give enough context to provide a specific suggestion then I'll mention few:

1) Do not mutate existing objects, create new ones:

if (item.shelf.match(/^currently\s?reading$/i) {
    result.currentlyReading.push({ ...item, setShelf: "Currently Reading" });
}

2) Perform this translation in componentDidUpdate() when you populate the books state property.

3) If you're using Redux then perform this translation in your Reducer (or Selector, if you're using them).

Re-think your design. Why do you need to duplicate an information you already have? What do you need these lists (with slightly changed values) for if they're already there in state.books? Let's imagine an hypothetical render() method:

render() {
    const { books } = this.state;

    return (
        <React.Fragment>
            <Shelf title="Currently reading" books={books} filter="^currently\\s?reading$" />
            <Shelf title="Read" books={books} filter="^read$" />
            <Shelf title="Want to read" books={books} filter="^want\\s?to\\s?read$" />
        </React.Fragment>
    );
}

Your separate lists are now obsolete. If you do not care about localization then regex for filter attribute might even be calculated directly (inside Shelf component!) with something like:

const filter = new RegExp(`^${this.props.title.replace(" ", "\\s?")}$`, "i");

Even if I am not doing this in my example please note that regular expression text MUST be escaped (before calling .replace()): Escape string for use in Javascript regex.

Now that you have a separate component...you may consider to move selection again outside and use memoization, simplified example (pseudo-code for memoization, pick the package you prefer):

render() {
    const { books } = this.state;

    const read = memoize((list, filter) => list.filter(book => book.shelf.match(filter)));

    return (
        <React.Fragment>
            <Shelf title="Currently reading" books={read(books, /^read$/i)} />
        </React.Fragment>
    );
}

Note that I'm NOT using the setShelf property any more in favor of Shelf.title property.


Code for movement controller of the player in Unity I am writing code for games in Unity. I should write the part very often and would like to know the flaws of the piece of code. It seems like everything is fine, but nevertheless I feel that some parts may be wrong. I am interested whether my style of coding is good or bad and would like to hear from someone who see my code for the first time whether he or she understands it or not.

Here is the code:

using System;
using System.Collections;
using System.Collections.Generic;
using UnityEngine;

public class MovementController : MonoBehaviour
{
    private enum Turned
    {left, right};
    private Turned turned;

    public float walkingSpeed, runningSpeed, currentSpeed;
    public Transform playerTransform;
    public PlayerAnimationController playerAnimationController;
    public SpriteRenderer playerSpriteRenderer;

    void Start ()
    {
        initializeNeededForMovementVariables();
    }

    private void initializeNeededForMovementVariables()
    {
        turned = Turned.left;
    }

    void Update ()
    {
        moveHorizontallyAndAnimateMovementIfNecessary();

    }

    private void moveHorizontallyAndAnimateMovementIfNecessary()
    {
        float moveHorizontallyValue = Input.GetAxis("Horizontal");
        if (moveHorizontallyValue == 0)
        {
            playAnimationOfBeingIdle();
            return;
        }

        if (Input.GetKey(KeyCode.LeftShift) || Input.GetKey(KeyCode.RightShift))
        { currentSpeed = runningSpeed; }
        else
        { currentSpeed = walkingSpeed; }

        conductHorizontalTranslationOfThePlayer(moveHorizontallyValue);
        flipFacingIfNecessary(moveHorizontallyValue);
        playAnimationOfHorizontalMovement();
    }

    private void playAnimationOfBeingIdle()
    {
        playerAnimationController.beingOnlyIdling();
    }

    private void playAnimationOfHorizontalMovement()
    {
        if (currentSpeed == walkingSpeed)
        { playerAnimationController.beingOnlyWalking(); }
        else if(currentSpeed == runningSpeed)
        { playerAnimationController.beingOnlyRunning(); }
    }

    private void flipFacingIfNecessary(float moveHorizontallyValue)
    {
        if (moveHorizontallyValue > 0 && turned == Turned.left)
        {
            toggleFlipXOnPlayerRenderer();
            turned = Turned.right;
        }
        else if(moveHorizontallyValue < 0 && turned == Turned.right)
        {
            toggleFlipXOnPlayerRenderer();
            turned = Turned.left;
        }
    }

    private void toggleFlipXOnPlayerRenderer()
    {
        playerSpriteRenderer.flipX = !playerSpriteRenderer.flipX;
    }

    private void conductHorizontalTranslationOfThePlayer(float moveHorizontallyValue)
    {
        transform.Translate(moveHorizontallyValue * currentSpeed, 0, 0);
    }
}

Will appreciate any attention and comments. If this question is not appropriate for the site and should be in game overflow, please, move it there.

First of all, I didn't like the long method names you used in the script.
like:

void Start ()
{
    initializeNeededForMovementVariables();
}

I mean, why did you need to call it like that? In my opinion this makes the code very messy once you have a lot of code in a script.
I would had call it something like: "InitVariables" or simply "Init"

Again,

float moveHorizontallyValue = Input.GetAxis("Horizontal");

The same point applies here too. You could have just written:

float h = Input.GetAxis("Horizontal");

Another thing: Why are you doing the following block?

private void playAnimationOfHorizontalMovement()
{
    if (currentSpeed == walkingSpeed)
    { playerAnimationController.beingOnlyWalking(); }
    else if(currentSpeed == runningSpeed)
    { playerAnimationController.beingOnlyRunning(); }
}

You have all this code to play animations. I suppose you are using other scripts too, and again, this is my opinion, it makes everything not totally understandable.

Then, Why aren't using an animator?

The code would be A LOT less and also A LOT more understandable, plus WAY EASIER to code!

Now, don't worry if your code isn't perfect now, you'll get better at it. It requires time. I really hope this helps you, Let me know!

Cheers


Copy Excel formula in a relative way I use NPOI to copy Excel formula, but can't find an option to do it in a relative way. Therefore, I've written a function to implement it. The task is "easy". When copying a formula A1+B1 of C1 to C2, the result will be A2+B2. But the formula can do more than that. For example:

Left(A1, 3) of B1 to B2 => Left(A2, 3)
(AB1 - AB$1) * $AB1 of AC1 to AD2 => (AC2 - AC$1) * $AB2
A1 & "-B1-C1" of B1 to B2 => A2 & "-B1-C1"

The difficult part is identifying the cell and handling the $.

public string GetCellForumulaRelative(string formula, int columnOffset, int rowOffset)
{
    var cells = formula.Split("+-*/(),:&^>=< ".ToCharArray(), StringSplitOptions.RemoveEmptyEntries)
        // start w/ A-Z
        .Where(i => (Convert.ToChar(i.Substring(0, 1).ToUpperInvariant()) >= 'A' &&
                     Convert.ToChar(i.Substring(0, 1).ToUpperInvariant()) <= 'Z') ||
                     Convert.ToChar(i.Substring(0, 1).ToUpperInvariant()) == '$')
        // end w/ 0-9
        .Where(i => Convert.ToChar(i.Substring(i.Length - 1, 1)) >= '0' &&
                    Convert.ToChar(i.Substring(i.Length - 1, 1)) <= '9');

    int startIdx = 0;
    foreach (var cell in cells)
    {
        int sepIdx = cell.IndexOfAny("0123456789".ToCharArray());
        if (cell.Substring(sepIdx - 1, 1).Equals("$"))
            sepIdx--;

        string col = cell.Substring(0, sepIdx);
        if (col.StartsWith("$") == false)
            col = GetExcelColumnName(ExcelColumnNameToNumber(col) + columnOffset);

        string row = cell.Substring(sepIdx);
        if (row.StartsWith("$") == false)
            row = (Convert.ToInt32(row) + rowOffset).ToString();

        startIdx = formula.IndexOf(cell, startIdx);
        formula = formula.Substring(0, startIdx) +
                  col + row +
                  formula.Substring(startIdx + cell.Length);
        startIdx += cell.Length;
    }

    return formula;
}

Any suggestions will be appreciated: performance, improvements, one-liners, or bugs.

I haven't used NPOI and I would believe you have valid reasons for using it (I'll take a wild guess at saying the thing is running on a server that doesn't have Excel installed), but for the record, if my memory isn't failing me if you used Microsoft VSTO / Excel Interop, you could use a plain & simple Copy+Paste and let Excel do the hard part.

Your code shows how much of a pain this could be otherwise.

That said @ANeves' comment about using Regular Expressions, along with @Zonko's comment about R1C1 references, could make your code much simpler:

Here's the magic regex: (\$?[A-Z]+\$?\d)(?=([^"]*"[^"]*")*[^"]*$)

...and the proof (using Expresso):

So the above regex pattern will spoonfeed you all A1 cell references that you need to analyze - those that aren't surrounded by quotes.

I don't know if NPOI facilitates this in any way (is there a Range object to play with?), but I would highly recommend getting the equivalent R1C1 cell references, so instead of AB$1 you get R1C[28]; then you can easily run a [much, much simpler] regex on these addresses to get the rows and columns and add your offsets, rebuild the addresses and rebuild the formulas.


XML attribute analysis To begin, I have an XML file at /Users/gaze/lab/lab/gpib_lib/out.xml that contains lines like...

<MemberCall method_name="setFastCoupleMode" callee_name="GPIB_7280" fname="/Users/gaze/lab/lab/gpib_lib/../apps/capbridge/capbridge.C" lineno="394" colno="9" />

<MemberCall method_name="setInputLineFilter" callee_name="GPIB_7280" fname="/Users/gaze/lab/lab/gpib_lib/../apps/capbridge/capbridge.C" lineno="395" colno="9" />

<MemberCall method_name="setFrequency" callee_name="capacitanceBridge" fname="/Users/gaze/lab/lab/gpib_lib/../apps/capbridge/capbridge.C" lineno="526" colno="5" />

<MemberCall method_name="getFrequency" callee_name="capacitanceBridge" fname="/Users/gaze/lab/lab/gpib_lib/../apps/capbridge/capbridge.C" lineno="528" colno="10" />

<MemberCall method_name="setupBandpass" callee_name="Stream_SR560" fname="/Users/gaze/lab/lab/gpib_lib/../apps/capbridge/capbridge.C" lineno="531" colno="9" />

<MemberCall method_name="thr" callee_name="listOutputFilter" fname="/Users/gaze/lab/lab/gpib_lib/../apps/capbridge/capbridge.C" lineno="540" colno="5" />

<MemberCall method_name="setBias" callee_name="capacitanceBridge" fname="/Users/gaze/lab/lab/gpib_lib/../apps/capbridge/capbridge.C" lineno="546" colno="5" />

To process it into a list of unique class.method() invocations I use the following code. I filter out all calls that occur outside my codebase, which is located at fpath.

filename←'/Users/gaze/lab/lab/gpib_lib/out.xml'
fpath←'/Users/gaze/lab/lab'

getxml←{⎕XML ⊃⎕NGET ⍵}
doc←getxml filename
attrib←↑doc[;4]

colnames ← attrib[1;;1]
col ← {attrib[;(attrib[1;;1]⍳⊂⍵);2]}
oot ← ((⍴fpath)↑¨col'fname')∊⊂fpath

fn ← (1+⍴fpath)↓¨oot/(col'fname')
cn ← oot/(col'callee_name')
mn ← oot/(col'method_name')

allUsed ← ∪(⍉↑(cn mn))[⍋cn;]

OOT denotes "out of tree." I'm curious if you might have suggestions about how to structure this in a larger application. I'm mostly curious how one should set fpath, should it change from invocation to invocation of allUsed, if this code is factored such that you can change filename. I don't exactly want to make allUsed a dyad s.t. fpath is on the right and filename is on the left, since that seems a bit wrong, should there be a more general allUsed that should indeed be a dyad. Should fpath be a global variable which is changed throughout the execution of a larger program? What's the right structure here? If you have any little details you'd improve upon here, I'd love to know as well.

Regarding the primary question, your code is very close to being a well-defined function. Simply wrap the entire code in a function, making filename the right argument, fpath the left argument, and allUsed the result like so (with a couple of unreference lines omitted):

 GetAllUsed←{
         filename←⍵
         fpath←⍺
         getxml←{⎕XML⊃⎕NGET ⍵}
         doc←getxml filename
         attrib←↑doc[;4]
         col←{attrib[;(attrib[1;;1]⍳⊂⍵);2]}
         oot←((⍴fpath)↑¨col'fname')∊⊂fpath
         cn←oot/(col'callee_name')
         mn←oot/(col'method_name')
         ∪(⍉↑(cn mn))[⍋cn;]
     }

I cannot envision a scenario where one might not want it as a dyadic function, or where one would want to introduce a global variable of any sort. I prefer the file name as the right argument, leaving the opportunity to make the left argument optional (possibly having it default to the first few path segments of the right arg?)

Regarding the actual code, the biggest issue is that it relies on the set of attributes for each MemberCall element being exactly the same. An additional or missing attribute will cause the code to fail, with an index error most likely, or produce a bad result.

If it is indeed the case that the attribute sets are identical, then a much simpler approach can be taken by restructuring the attributes for all elements into a single matrix of name/value pairs. There is no need to construct a rank-3 array as is done currently.

If this is not the case, then one approach is to define a little getter function that picks out a given attribute for each element. This function can handle the issue of missing or extra attributes, attributes in any order, etc.


Select second field in text table with shell script I am writing a bash script to mount openshift service accounts into kubernetes objects. The right secret to use is highlighted in this text:

$ oc describe sa sa-build-webhook-realworld
Name:                sa-build-webhook-realworld
Namespace:           your-eng2
Labels:              app=sa-build-webhook-realworld
Annotations:         <none>
Image pull secrets:  sa-build-webhook-realworld-dockercfg-4qz9g
Mountable secrets:   sa-build-webhook-realworld-token-bqtnw
                     sa-build-webhook-realworld-dockercfg-4qz9g
Tokens:              sa-build-webhook-realworld-token-bqtnw
                     sa-build-webhook-realworld-token-k7lq8
Events:              <none>

I want the code to be reasonably robust so I am thinking of this as a job for awk where I need "anything in column 2 that is a 'Mountable secret' that isn't the docker secret". Here is the logic I have come up with:

MOUNTABLE_SECRETS='Mountable secrets:'
SECRET_NAME=$(\
   oc describe sa sa-build-webhook-realworld \
   | sed $(printf 's/./&,/%s' ${#MOUNTABLE_SECRETS}) \
   | awk  'BEGIN{FS=OFS=","} {if ($1 ~ /^[ \t]*$/) $1=ch; else ch=$1} 1'  \
   | grep "$MOUNTABLE_SECRETS" \
   | sed 's/[, ]*//g' \
   | awk -F':' '{print $2}' \
   | grep -v docker \
   | grep token)
echo "SECRET_NAME=$SECRET_NAME"

Basically, I insert a character just beyond the width of that phrase to cut the table in half, copy cells on the left into blanks below, then select the second column then grep what I am looking for.

To my mind, it breaks things into pieces that can be understood. It works, but past experiences have taught me that someone looking to maintain that may not be best pleased. Since performance is not an issue what I am really aiming for is maintainability. I also want portability and I am looking to stick with to typical bash and coreutils tools.

How might I improve that script?

Your idea to transform the table so that the implied keys explicitly appear on every row is an interesting one, but I think that it is overcomplicated.

The backslashes to indicate continuation lines are actually superfluous here, since an unfinished $( substitution automatically causes the command to be incomplete.  Similarly, ending a line with a | pipe would also cause the command to be continued, so that would be a better convention to follow than putting the | at the beginning of the following line.

In general, any combination of sed, awk, and grep would be better expressed using just an AWK script.  The AWK script below reads a key and value if there is a colon on a line, or just a value if there is no colon.

SECRET_NAME=$(
    oc describe sa sa-build-webhook-realworld |
    awk -F: '
        $2  { KEY=$1 ; VALUE=$2; sub("^ *", "", VALUE); }
        !$2 {          VALUE=$1; sub("^ *", "", VALUE); }
        KEY=="Mountable secrets" && VALUE !~ /docker/ { print VALUE }
    '
)