Reading source code and extracting json from a url I made a program to read the source code from a url. To reach this url I need to specify a token. After read the source code, I extract a url in json format, andIi redirect my domain to this url. It's not relevant to what I'm asking, but I want explain what I'm doing.

As you can see, on my domain http://localhost/test, I have a query string called token that is used to read the source code of a different domain. After this I redirect to a page.

I defined the domain which I will read the source code but I don't know if someone could pass a value to the $_GET['token'] in a way that this will go to a different domain. I'm reading the source code from a url and using the result on my website, so I don't know if someone could do some kind of attack to my server. I'm not sure how these kind of attacks works, and I felt that I should ask someone that has more knowledge than I.

What do you think about my code?

php:

 //i use this condition to make sure that nobody can access the url directly,
 //and later make sure that the request is from my domain
    if(!empty($_SERVER['HTTP_REFERER'])){
        //if $_GET[] is empty then $.. is = NULL, else $.. is = $_GET[]
        $source = (empty($_GET['source']) ? NULL : $_GET['source']);
        $token = (empty($_GET['token']) ? NULL : $_GET['token']);

        if(!empty($source) && !empty($token)){
            if($source == 'player'){
                //check if the request is from the same domain
                if(preg_match('/^http:\/\/localhost\/test/', $_SERVER['HTTP_REFERER'])){
                    //inside this if() i read the source
                    //code from this url bellow
                    $urlPost = 'https://www.player.com/?token='.$token;
                    $url = file_get_contents($urlPost);
                    preg_match('/CONFIG = (.*)/', $url, $matches);

                    $jsn =  json_decode($matches[1]);
                    $streams = $jsn->streams;

                    foreach($streams as $i){
                        //this variable will be used to redirect the src
                        //iframe to a url that contains a video
                        $vdUrl = $i->url;
                    }
                    //redirect to the url that i got from the source code
                    die(header('location:'.$vdUrl));
                }else{
                    //if the request is not from the same domain then
                    //redirect to 404 Not Found
                    die(header('HTTP/1.1 404 Not Found'));
                }
            }else{
                die(header('HTTP/1.1 404 Not Found'));
            }
        }

    }

html:

<iframe src="http://localhost/test/?source=player&token=145215d1ww"></iframe>

The Security
The main point here is your assumption that HTTP_REFERER could prevent someone from using this code. Unfortunately it doesn't. Referrer, just like any other HTTP header, is easily faked, one is setting it routinely with any software that is doing HTTP requests. So be advised that it is an ostrich-style defense.
You should understand that there is absolutely no way to protect the information which is shown in the browser. The only way to restrict an access to a script is to make it password protected.
Moreover, I suppose that a concerned user could easily guess the domain from which you are requesting the data, and then just bluntly take the token from the source of your page and then just use it at their own disposal. Consider storing the token inside your PHP code instead.
The code.
That said, your code could be syntactically improved as well. First of all, just like it said in the other answer, you could combine all conditions in one. But it mustn't be done at the expense of readability. So first define your conditions and then check them at once.
After that you could just write your code without any conditions. A loop is unnecessary if there is only one value in the array
$referrer = $_SERVER['HTTP_REFERER'] ?? '';
$access = preg_match('!^http://localhost/test!', $referrer);
$token = $_GET['token'] ?? '';
$source = $_GET['source'] ?? '';
$source_ok = $source == 'player';

if (!$referrer || !$access || !$token || !$source || !$source_ok) {
    header('HTTP/1.1 404 Not Found');
    die;
}
$urlPost = 'https://www.player.com/?token='.$token;
$url = file_get_contents($urlPost);
preg_match('/CONFIG = (.*)/', $url, $matches);
$jsn =  json_decode($matches[1]);
$vdUrl = $jsn->streams[0]->url;
header('location:'.$vdUrl);

Note: if $_GET['source'] ?? ''; operator is giving you an error, consider upgrading your PHP version immediately, because it is not supported anymore.


Find Index of SINGLE odd / even number in array So I was doing some challenges on codewars.com and came across the following challenge (I managed to close the browser so could not submit my answer and therefore could not see the best possible solutions).

Given an array of at least 3 integers, all the elements in the array are either even or oneven EXCEPT for 1 element -->

[2, 4, 0, 100, 4, 11, 2602, 36]

OR:

[160, 3, 1719, 19, 11, 13, -21]

All the elements are either even or oneven except for one where it is the other way around. Return the element that is the odd one out, so:

findOutlier([2, 4, 0, 100, 4, 11, 2602, 36]) //should return 11

findOutlier([160, 3, 1719, 19, 11, 13, -21]) //should return 160

I got a working solution but there has to be a better way of doing this.. My solution:

function findOutlier(integers){
  let evenCount = 0;
  let oddCount = 0;
  for(let i = 0; i < integers.length; i++) {
    if(integers[i] % 2 === 0) {
      evenCount += 1;
    } else {
      oddCount += 1
    }
  }
  let idx = integers.find(function(index) {
    if(evenCount > 2) {
      return index % 2 === 1
    } else {
      return index % 2 === 0
    }
  })
  return idx
}

```

You only need to iterate the first 3 numbers of the array.

for(let i = 0; i < 3; i++) {

You are iterating through the array twice. You should only iterate once. You can do this by setting the values in the first loop:

let evenCount = 0;
let oddCount = 0;
let evenNumber = 0;
let oddNumber = 0;

for(let i = 0; i < 3; i++) {
    if(integers[i] % 2 === 0) {
      evenCount++;
      evenNumber = integers[i];
    } else {
      oddCount++;
      oddNumber = integers[i];
    }
}

return evenCount > 1 ? oddNumber : evenNumber;


Best way to create a daemon in a JavaWeb application I have a purpose for an application to run a daemon in my web application.

The application is running with Struts1 on a tomcat7 server using Java7.

This is what it does:

Launch itself when the application is loaded
every minutes does a task (the sysout in the code)
when the application stops, it stops the timer and let the task 10 sec max to finish its process if needed

In the web.xml file i have these lines to launch the process:

<listener>
    <listener-class>com.toto.tata.applicationlayer.BiuDeamon</listener-class>
</listener>

And here is the code:

package com.toto.tata.applicationlayer;

import java.util.Date;
import java.util.Timer;
import java.util.TimerTask;
import java.util.concurrent.TimeUnit;

import javax.servlet.ServletContextEvent;
import javax.servlet.ServletContextListener;

import org.apache.log4j.Logger;

public class BiuDeamon implements ServletContextListener {
    private static boolean isDaemonWorking = false;
    private static Timer timer;
    private static TimerTask timerTask;

    static {
        timerTask = new TimerTask(){
            public void run() {
                if (!isDaemonWorking) {
                    try {
                        isDaemonWorking = true;

                        System.out.println("I RUN");

                        isDaemonWorking = false;
                    } catch (Throwable e) {
                        ex.printStackTrace();
                    }
                }
            }
        };
    }

    public BiuDeamon() {
        super();
    }

    public void contextInitialized(ServletContextEvent a_event) {
        timer = new Timer();
        timer.schedule(timerTask, new Date(), 60000);
    }

    public void contextDestroyed(ServletContextEvent a_event) {
        // we stop the timer
        timer.cancel();
        // we let the timertask 10 sec to stop its processing
        int i=0;
        for (; i<10; i++) {
            if (isDaemonWorking) {
                try {
                    TimeUnit.SECONDS.sleep(1);
                } catch(InterruptedException ex) {
                    Thread.currentThread().interrupt();
                }
            } else {
                break;
            }
        }
        // we kill it if needed
        if (i==10)
            timerTask.cancel();
    }
}

I know my code is working because I have tested it, but I wonder if there is a better way to achieve what I want.

Any comments / examples will be appreciated :)

Using a ServletContextListener is a good solution, I would use the same.
I would consider using a ScheduledExecutorService here (Executors.newScheduledThreadPool(1)). It has some advantages:

It has a scheduleWithFixedDelay method as well as scheduleAtFixedRate. Using one of these describes what's the developer's purpose. Currently its not completely obvious or explicit.
It supports shutdown and awaitTermination. awaitTermination returns immediately when your task finishes, no need to wait complete seconds.
awaitTermination returns false on timeout when you can call shutdownNow which interrupts the thread of the task.
It does not need manual and error-prone synchronization (see the next point).
newScheduledThreadPool can get a ThreadFactory. You can set a UncaughtExceptionHandler to handle uncatched exceptions with that factory. (See also this answer too.)

Usage of isDaemonWorking is not thread-safe. It is used by the timer's thread (inside the TimerTask.run method) as well as the ServletContextListener. Without any synchronization you risk that threads will not see each other's modification.

  [...] synchronization has no effect unless both read and write operations are synchronized.

From Effective Java, 2nd Edition, Item 66: Synchronize access to shared mutable data.
You could invert the if condition here:

for (; i<10; i++) {
    if (isDaemonWorking) {
        try {
            TimeUnit.SECONDS.sleep(1);
        } catch(InterruptedException ex) {
            Thread.currentThread().interrupt();
        }
    } else {
        break;
    }
}

It would keep the code flatten:

for (; i < 10; i++) {
    if (!isDaemonWorking) {
        break;
    }
    try {
        TimeUnit.SECONDS.sleep(1);
    } catch (InterruptedException ex) {
        Thread.currentThread().interrupt();
    }
}

I guess if you put the isDaemonWorking = false into a finally block:

try {
    isDaemonWorking = true;
    System.out.println("I RUN");
} catch (Throwable e) {
    e.printStackTrace();
} finally {
    isDaemonWorking = false;
}

then you could replace

if (i == 10)
    timerTask.cancel();

with

if (isDaemonWorking) {
    timerTask.cancel();
}

and use an usual for (int i = 0; i < 10; i++) loop too.
I would not think that initialization of TimerTask should be in a static block. The timer and isDaemonWorking also could be instance fields. I think (and I would expect that) the servlet container calls contextDestroyed on the same instance than contextInitialized.
Variable names in Java usually camelCase, so a_event should be aEvent or just event here:

public void contextInitialized(ServletContextEvent a_event) {

I guess you have a typo here: BiuDeamon (it should be BuiDaemon).


Button to follow / unfollow a user As a beginner in coding, I have just written this code.

The code should Add a form with a button to each user profile, it named Follow or Unfollow. It updates the user meta named "following" for the user who clicked the button. So we have to use the user IDs as a serialized array. Then we should use a query for the user meta "following" to get a list of all authors the current user has subscribed to and then query for the posts from these authors.

<?php

 if($_GET['follow']){fun1();}

 function fun1()
 {
$fauid = get_user_by( 'slug', get_query_var( 'author_name' ) );
$user_id= get_current_user_id();
$key = 'following';
$themeta = get_user_meta($user_id , $key, TRUE);
if($themeta != '') {
$user_id= get_current_user_id;
update_user_meta($user_id, 'following', $fauid); }

else {

$user_id= get_current_user_id;
add_user_meta($user_id , 'following' , $fauid , true );
update_user_meta($user_id, 'following', $fauid);
}
}

?>

<html>
    <button id="Button" name="Button" onClick='location.href="?follow=1"'>Follow Me <3 <3</button>
</html>

Is the code true or not?

if($_GET['follow']){fun1();}

A more typical way to write this would be

if ( $_GET['follow'] ) {
  fun1();
}

As a general rule, do not put statements on the same line as the curly brace that opens the block.

$key = 'following';
$themeta = get_user_meta($user_id , $key, TRUE);

Since you never use $key again, you don't need it.

$following = get_user_meta($user_id, 'following', true);

If you're only getting the meta data for 'following', $following is probably a better name than $themeta.

if($themeta != '') {
$user_id= get_current_user_id;
update_user_meta($user_id, 'following', $fauid); }

else {

$user_id= get_current_user_id;
add_user_meta($user_id , 'following' , $fauid , true );
update_user_meta($user_id, 'following', $fauid);
}

Two of the three statements are in both branches of the if.  Therefore, you could pull them out:

$user_id= get_current_user_id;
if($themeta != '') {
} else {
  add_user_meta($user_id , 'following' , $fauid , true );
}
update_user_meta($user_id, 'following', $fauid);

Note that the order of the statements will stay the same with the add_user_meta in the middle.

But if you do that, one of the branches is empty.  You might as well remove it.

$user_id = get_current_user_id;
if ( '' == $themeta ) {
  add_user_meta($user_id , 'following' , $fauid , true );
}
update_user_meta($user_id, 'following', $fauid);

You also call

$user_id= get_current_user_id;

multiple times.  You should be able to do this just once.

The update_user_meta function will overwrite the value that you just added, so you might be better off with

if ( '' == $themeta ) {
  add_user_meta($user_id , 'following' , $fauid , true );
} else {
  update_user_meta($user_id, 'following', $fauid);
}

Now you add or update, not both.

This model only allows you to follow one author at a time.  A second author will replace the first.  It's more common to be able to follow as many as you want.  You might be able to manage with a serialized array, but a more robust solution would be a table that matched readers to authors.

$following = get_user_meta($user_id, 'following', true);
if ( ! is_array($following) || empty($following) ) {
  add_user_meta($user_id , 'following' , array($fauid) , true );
} else if ( ! in_array($fauid, $following) ) {
  $following[] = $fauid;
  update_user_meta($user_id, 'following', $following);
}

If there's no following array, it creates one.  If one already exists and doesn't already have this user ID in it, add to the existing array and update.

It's also worth noting that this is not the normal way to write for WordPress.  WordPress usually makes additions by adding plugins, and this doesn't look like a plugin to me.  For one thing, it doesn't register any hooks.  Nor does it define an install/activate procedure.


Grouping the results in an array having same name results is a list of dict having panel name and work type and price to do that work on that panel. There can be two dicts for same name and work type where one of them is on car model level and another is on car type level. In such cases we would give preference to car model level dict.

output is the group of panels and prices per work type (removing the prices on car type if car model price exist for same panel and work type combination).

results = sorted(results, key=lambda datum: datum['car_model'])

#removing the car type if car model exists.
d = defaultdict(dict)
for l in results:
    d[l['name']+'-'+l['work_type']] = l

results = d.values()

#grouping the results at panel level.
d = defaultdict(dict)
for l in results:
    d.setdefault(l['name'],[]).append(l)

#creating dict such that panel name is key and price_list is array of all the panel prices.
output = []
for key,value in d.iteritems():
    output.append({"name":key,"price_list":value})

Example input:

results = [
        {
            "id":1,
            "car_type":1,
            "car_model":None,
            "name":"Door",
            "work_type":"Dent",
            "price":2300,
        },
        {
            "id":2,
            "car_type":1,
            "car_model":None,
            "name":"Door",
            "work_type":"Scratch",
            "price":1200,
        },
        {
            "id":3,
            "car_type":None,
            "car_model":2,
            "name":"Door",
            "work_type":"Dent",
            "price":2500,
        },
        {
            "id":4,
            "car_type":1,
            "car_model":None,
            "name":"Fender",
            "work_type":"Dent",
            "price":2300,
        },
        {
            "id":5,
            "car_type":1,
            "car_model":None,
            "name":"Fender",
            "work_type":"Scratch",
            "price":1300,
        }
    ]

Expected output:

[{'name': 'Fender',
  'price_list': [{'car_model': None,
    'car_type': 1,
    'id': 5,
    'name': 'Fender',
    'price': 1300,
    'work_type': 'Scratch'},
   {'car_model': None,
    'car_type': 1,
    'id': 4,
    'name': 'Fender',
    'price': 2300,
    'work_type': 'Dent'}]},
 {'name': 'Door',
  'price_list': [{'car_model': 2,
    'car_type': None,
    'id': 3,
    'name': 'Door',
    'price': 2500,
    'work_type': 'Dent'},
   {'car_model': None,
    'car_type': 1,
    'id': 2,
    'name': 'Door',
    'price': 1200,
    'work_type': 'Scratch'}]}]

General comments

results = sorted(results, key=lambda datum: datum['car_model'])

Good use of sorted!

#removing the car type if car model exists.
d = defaultdict(dict)
for l in results:
    d[l['name']+'-'+l['work_type']] = l

results = d.values()

Why are you using a defaultdict? A normal dict suffices.

#grouping the results at panel level.
d = defaultdict(dict)
for l in results:
    d.setdefault(l['name'],[]).append(l)

Again, why a defaultdict, a normal dict suffices.

#creating dict such that panel name is key and price_list is array of all the panel prices.
output = []
for key,value in d.iteritems():
    output.append({"name":key,"price_list":value})

Good enough, I suppose. Alternatively, use

output = [{"name": key, "price_list": value} for key, value in d.iteritems()]

Variable naming

Variable names such as d and l just don't cut it anymore. Please use descriptive variable names.

Revised code

results = sorted(results, key=lambda datum: datum['car_model'])

# Make sure results are unique by {name}-{work_type}
results = {l['name'] + '-' + l['work_type']: l for l in results}.values()

# Grouping the results at panel level.
d = {}
for l in results:
    d.setdefault(l['name'],[]).append(l)

# Creating dict such that panel name is key and price_list is array of all the panel prices.
output = [{"name": key, "price_list": value} for key, value in d.iteritems()]

Separate by functions

I see several actions: getting unique values, grouping by a key, and finally outputting it.

Getting unique values

def unique_by(lst, key=lambda x: x):
    return {key(v): v for v in lst}.values()

You can call it like

results = unique_by(results, key=lambda l: l['name'] + '-' + l['work_type'])

or better

results = unique_by(results, key=lambda l: (l['name'], l['work_type']))

and using operator.itemgetter:

results = unique_by(results, key=itemgetter('name', 'work_type'))

Grouping by a key

def group_by(lst, key=None):
    assert key is not None, 'You must supply a key function'
    retval = {}
    for v in lst:
        retval.setdefault(key(v), []).append(v)
    return retval

You can call it like

grouped = group_by(results, key=lambda l: l['name'])

or

grouped = group_by(results, key=itemgetter('name'))

Revised, again

from operator import itemgetter

results = sorted(results, key=itemgetter('car_model'))
results = unique_by(results, key=itemgetter('name', 'work_type'))
grouped = group_by(results, key=itemgetter('name'))
output = [{"name": name, "price_list": items} for name, items in grouped.iteritems()]

(where unique_by and group_by are defined as above.)

Note: untested, but it should work (modulo syntax errors).


TicTacToe with Python def tictactoe():
    Board = [1,2,3,4,5,6,7,8,9]
    w = True
    PlayerX = []
    PlayerO = []
    while len(PlayerO) < 9:
        while True:
            z = int(input("Player X: Select a place (1-9)\n"))
            if z in Board:
                Board[z-1] = []
                PlayerX.append(z)
                break
            else:
                print("Place not available! Choose another X")
        while True:
            y = int(input("Player O: Select a place (1-9)\n"))
            if y in Board:
                Board[y-1] = []
                PlayerO.append(y)
                break
            else:
                print("Place not available! Choose another O")
        if len(PlayerX) > 2:
            if set([1,2,3]).issubset(set(PlayerX)) or set([4,5,6]).issubset(set(PlayerX)) or set([7,8,9]).issubset(set(PlayerX)) or set([1,4,7]).issubset(set(PlayerX)) or set([2,5,8]).issubset(set(PlayerX)) or set([3,6,9]).issubset(set(PlayerX)) or set([1,5,9]).issubset(set(PlayerX)) or set([3,5,7]).issubset(set(PlayerX)):
                print("Player X won!")
                w = False
                break
            elif set([1,2,3]).issubset(set(PlayerO)) or set([4,5,6]).issubset(set(PlayerO)) or set([7,8,9]).issubset(set(PlayerO)) or set([1,4,7]).issubset(set(PlayerO)) or set([2,5,8]).issubset(set(PlayerO)) or set([3,6,9]).issubset(set(PlayerO)) or set([1,5,9]).issubset(set(PlayerO)) or set([3,5,7]).issubset(set(PlayerO)):
                print("Player O won!")
                w = False
                break
    if w is True:
        print("Tie")
    if input("Play Again (y/n)?\n") == "y":
        tictactoe()
tictactoe()

I'm trying to make this code as efficient as possible. I'm new to Python and kinda limited with the knowledge I have. How can I simplify the code and especially the line shown below?

if set([1,2,3]).issubset(set(PlayerX)) or set([4,5,6]).issubset(set(PlayerX)) or set([7,8,9]).issubset(set(PlayerX)) or set([1,4,7]).issubset(set(PlayerX)) or set([2,5,8]).issubset(set(PlayerX)) or set([3,6,9]).issubset(set(PlayerX)) or set([1,5,9]).issubset(set(PlayerX)) or set([3,5,7]).issubset(set(PlayerX)):

I guess from efficiency point of view this is fine, even that last line. Efficiency points:

You create player variable as list and then keep converting to set over and over again, why not have it as set from beginning?

From readability and maintenance point of view, you are having lots of duplicities.

Player moves are exactly same except for message and variable, you can extract that to function.
Same goes for checking if a player won.
I don't like variable w. Why not make variable winner instead and save name of winner into it? Related to extracting if player won to separate function.

And for your last line - you can put all possible winning combinations into array or tupple and then check that in cycle (you get the idea):

winning_combinations = [
[1, 2, 3],
[4, 5, 6],
...
]


Most elegant image to image frame comparison algorithm I'm interested in finding the most elegant and 'correct' solution to this problem. I'm new to JS/PHP and I'm really excited to be working with it. The goal is to compare the size of a given image to the size of a 'frame' for the image (going to be a <div> or <li> eventually) and resize the image accordingly. Users will be uploading images of many sizes and shapes and there are going to be many sizes and shapes of frames. The goal is to make sure that any given image will be correctly resized to fit any given frame. Additionally, images will be cropped with overflow:hidden; so they'll never be distorted. Final implementation will be in PHP, but I'm working out the logic in JS. Here's what I have:

//predefined variables generated dynamically from other parts of script, these are example values showing the 'landscape in landscape' ratio issue
var $imageHeight = 150, $imageWidth = 310, $height = 240, $width = 300;
//check image orientation
if ( $imageHeight == $imageWidth ) {
    var $imageType = 1; // square image
} else if ( $imageHeight > $imageWidth ) {
    var $imageType = 2; // portrait image
} else {
    var $imageType = 3; // landscape image
};
//check frame orientation and compare to image orientation
if ( $height == $width) { //square frame
    if ( ( $imageType === 1 ) || ( $imageType === 3 ) ) {
        $deferToHeight = true;
    } else {
        $deferToHeight = false;
    };
} else if  ( $height > $width ) { //portrait frame
    if ( ( $imageType === 1 ) || ( $imageType === 3 ) )  {
        $deferToHeight = true;
    } else {
        if (($imageHeight / $height) < 1) {
                $deferToHeight = true;
            } else {
                $deferToHeight = false;
        };
    };
} else { //landscape frame
    if ( ( $imageType === 1 ) || ( $imageType === 2 ) ) {
        $deferToHeight = false;
    } else {
        if (($imageWidth / $width) > 1) {
                $deferToHeight = true;
            } else {
                $deferToHeight = false;
        };
    };
};
//set values to match (null value scales proportionately)
if ($deferToHeight == true) { //defer image size to height of frame
    $imageWidth = null;
    $imageHeight = $height;
} else { //defer image size to width of frame
    $imageWidth = $width;
    $imageHeight = null;
};

I've tested it with a bunch of different values and it works, but I have a suspicion that I could have implemented a much more elegant solution. What could I have done better?

If we move past rewriting the boolean expressions and instead rework the formulas, I think we can get something a little easier to manage.

If I understand correctly, your goal is to have the image cover the frame. That is to say, for the frame to be filled and the image cropped.

In that case, All we need to do is scale the image to the frame.

var imageHeight = 150, imageWidth = 310, height = 240, width = 300;

//Find the larget discrepancy between frame size and corresponding image size
var scaleRatio = Math.max( height/imageHeight, width/imageWidth );

//Apply the scale to the whole image.
imageHeight *= scaleRatio;
imageWidth *= scaleRatio;

If instead your desire it to have the image fit in the frame, then all we need to do is replace Math.max with Math.min.


More Elegant Solution to Logging Class I got this class in C# which works fine. But I was wondering if there was a more elegant solution to what I am trying to do. It looks rather clumsy and inefficient for a Logging functionality.

The code should be fairly self-explanatory.

public static class Log
{
    public static string EngineName { get; set; }
    private static List<String> logdata = new List<string>();

    static void LogMessage(string msg)
    {
        Console.WriteLine("[{0}][LOG]: {1}", EngineName, msg);
        StringBuilder sb = new StringBuilder();
        sb.Append("[").Append(EngineName).Append("][LOG]: ").Append(msg);
        logdata.Add(sb.ToString());
    }

    public static void EngineMessage(string msg)
    {
        Console.WriteLine("[{0}][ENGINE]: {1}", EngineName, msg);
        StringBuilder sb = new StringBuilder();
        sb.Append("[").Append(EngineName).Append("][Engine]: ").Append(msg);
        logdata.Add(sb.ToString());
    }

    public static void ExceptionMessage(Exception e)
    {
        Console.WriteLine("[{0}][EXCEPTION]: {1}", EngineName, e.Message);
        StringBuilder sb = new StringBuilder();
        sb.Append("[").Append(EngineName).Append("][EXCEPTION]: ").Append(e.Message);
        logdata.Add(sb.ToString());
    }

    public static void CustomMessage(string title, string msg)
    {
        title.ToUpper();
        Console.WriteLine("[{0}][{1}]: {2}", EngineName, title, msg);
        StringBuilder sb = new StringBuilder();
        sb.Append("[").Append(EngineName).Append("][").Append(title).Append("]: ").Append(msg);
        logdata.Add(sb.ToString());
    }

    public static void ClearLogData()
    {
        logdata.Clear();
    }

    public static void PrintLog()
    {
        Console.WriteLine("===== LOG DATA =====\n");
        Console.WriteLine(new DateTime().ToString()+"\n");
        foreach(string s in logdata)
        {
            Console.WriteLine(s + "\n");
        }
    }

    public static void SaveLog(string path)
    {
        throw new NotSupportedException();
    }
}

EDIT

Based on feedback from the accepted answer here is my revised code:

public static class Log
{
    public static string EngineName { get; set; }
    private static List<String> logdata = new List<string>();

    public static void LogMessage(string msg, ELogflag flag, string title = "")
    {
        StringBuilder sb = new StringBuilder();
        sb.Append("[" + EngineName + "]");
        switch (flag)
        {
            case ELogflag.LOG:
                sb.Append("[LOG]");
                break;
            case ELogflag.ENGINE:
                sb.Append("[ENGINE]");
                break;
            case ELogflag.CRITICAL:
                sb.Append("[CRITICAL]");
                break;
            case ELogflag.CUSTOM:
                title = title.ToUpper();
                sb.Append("[" + title + "]");
                break;
            default:
                sb.Append("[UNKNOWN]");
                break;
        }
        sb.Append(msg);
        Console.WriteLine(sb.ToString());
        logdata.Add(sb.ToString());
    }

    public static void ClearLogData()
    {
        logdata.Clear();
    }

    public static void PrintLog()
    {
        Console.WriteLine("===== LOG DATA =====\n");
        Console.WriteLine(new DateTime().ToString() + "\n");
        foreach (string s in logdata)
        {
            Console.WriteLine(s + "\n");
        }
    }

    public static void SaveLog(string path)
    {
        throw new NotSupportedException();
    }
}

And my new Enum:

enum ELogflag
{
    LOG,
    ENGINE,
    CRITICAL,
    CUSTOM
}

C# is not my main language, so I apologize for any mistakes that may exist in the following code.

There is quite a bit of duplication in this class. It seems like LogMessage(), EngineMessage() and ExceptionMessage() could all delegate to CustomMessage(). For example:

public static void LogMessage(string msg)
{
    CustomMessage("LOG", msg);
}

// etc ...

It also looks like the same string is being built twice in each function. It would be better to create the string once. Maybe something like this instead:

string logMsg = String.Format("[{0}][{1}]: {2}", EngineName, title, msg);
Console.WriteLine(logMsg);
logdata.Add(logMsg);

title.ToUpper();

Does this work? I would not expect this to modify the string in place, but rather return a new string. You probably need to do this:

title = title.ToUpper();

With the above changes:

public static void LogMessage(string msg)
{
    CustomMessage("LOG", msg);
}

public static void EngineMessage(string msg)
{
    CustomMessage("ENGINE", msg);
}

public static void ExceptionMessage(Exception e)
{
    CustomMessage("EXCEPTION", msg);
}

public static void CustomMessage(string title, string msg)
{
    string logMsg = String.Format("[{0}][{1}]: {2}", EngineName, title.ToUpper(), msg);
    Console.WriteLine(logMsg);
    logdata.Add(logMsg);
}


Simple Censor Program I made a function to censor a certain word from a piece of text. Is there anything I should fix or do more efficiently? What are some other approaches I could have taken to detecting words and censoring them?(This took me a lot of time to brainstorm)

Also, I would appreciate it if you told me how you brainstorm how to write a program.

def detect_word_start(text,word):
    count = 0
    text_count = 0
    for x in text:
        text_count+=1
        if x == word[count]:
            count+=1
            if count == len(word):
                return text_count - len(word) + 1
        else:
            count = 0

def censor(text,word):
    word_start = detect_word_start(text,word)
    censored_text = ''
    count = 0
    for x in text:
        count += 1
        if count >= word_start and count < word_start+len(word):
            censored_text += '*'
        else:
            censored_text += x
        print(censored_text)

Let's start by removing print from the loop and add a return value

def censor(text,word):
    #[...]
        else:
            censored_text += x
    return censored_text

now we have a testable function. we add some tests

import unittest

class TestSome(unittest.TestCase):

    def test_not_found(self):
        self.assertEqual(censor("", "bar"), "")
        self.assertEqual(censor("f", "bar"), "f")
        self.assertEqual(censor("foo", "bar"), "foo")
        self.assertEqual(censor("fooo", "bar"), "fooo")

    def test_found(self):
        self.assertEqual(censor("bar", "bar"), "***")
        self.assertEqual(censor("bar!", "bar"), "***!")
        self.assertEqual(censor("cow bar", "bar"), "cow ***")

    def test_parts(self):
        self.assertEqual(censor("foobar", "bar"), "foobar")
        self.assertEqual(censor("bare", "bar"), "bare")

    def test_capital(self):
        self.assertEqual(censor("Bar", "bar"), "***")

    def test_multiple(self):
        self.assertEqual(censor("foo bar bar foo", "bar"), "foo *** *** foo")

if __name__ == '__main__':
    unittest.main()

Running the tests show that the functions are not meeting my expectations.

detect_word_start() does not return a value if nothing found
you miss words at the beginning of a sentence becaue of capitalization
you censor different words containing the character sequence
you miss multiple appearences


Lock free consumer producer using memory barrier I am looking for "complete" producer-consumer sample using C++11 memory barrier.

(I have derived following example from Jeff's article and added a line to make it complete.)

void SendTestMessage(void* param)
{
    // Copy to shared memory using non-atomic stores.
    g_payload.tick  = clock();
    g_payload.str   = "TestMessage";
    g_payload.param = param;

    // Release fence.
    std::atomic_thread_fence(std::memory_order_release);

    // Perform an atomic write to indicate that the message is ready.
    g_guard.store(1, std::memory_order_relaxed);
}

bool TryReceiveMessage(Message& result)
{
    // Perform an atomic read to check whether the message is ready.
    int ready = g_guard.load(std::memory_order_relaxed);

    if (ready != 0)
    {
        g_guard.store(0, std::memory_order_relaxed);

        // Acquire fence.
        std::atomic_thread_fence(std::memory_order_acquire);

        // Yes. Copy from shared memory using non-atomic loads.
        result.tick  = g_payload.tick;
        result.str   = g_payload.str;
        result.param = g_payload.param;

        return true;
    }

    // No.
    return false;
}

if you notice, i have added g_guard.store(0, std::memory_order_relaxed); just before acquire.

This will help following ways

It would avoid TryReciveMessage to cosnume same message if called multiple times before new message is written
It would not add explicit memory fence and hence won't affect performance
since std::memory_order_relaxed guarantees the ordering, it would get overwritten by SendTestMessage value if new load added after the std::memory_order_acquire is called. So, we will not miss any load.

Please provide your comments and/or suggestions.

It's always difficult, and sometimes impossible, to review a snippet of code without knowing how it is intended to be used. In this case, I can say confidently that this code will do the job. Whether that happens to be the job you want it to do is another thing entirely.

As long as you understand the limitations of the pattern you're using (and assuming g_guard is initialized to 0 properly before either of the functions are called), this will "work".

The limitations are:

SendTestMessage() can only be called once. Not once-per-thread; once for the entire program.
TryReceiveMessage() can be called repeatedly... but only non-concurrently in a single thread.

You didn't say how you intend to use this code, but from your questions, I can deduce that this won't work for you.

It would avoid TryReciveMessage to cosnume same message if called multiple times before new message is written

With this code, it is safe to call SendTestMessage() EXACTLY ONCE, concurrently with TryReceiveMessage() any number of times IN A SINGLE THREAD. So TryReceiveMessage() will not consume the same message more than once... but it can also not consume more than one message in total. There can be no "new message".

Safely synchronizing the guard does not safely synchronize the payload. This code "works" only because the payload is written ONCE, and then read, with the guard making sure the read is absolutely ordered after the write. If you want to write a second time (for a new message): you can't. You'd need a second guard to prevent the second write until after the read is finished. That would turn SendTestMessage() into a blocking call... and then you might as well just use regular synchronization techniques.
It would not add explicit memory fence and hence won't affect performance

"Performance" is always a vague term. There are many different ways for code to be performant.

If your goals are:

You want to minimize inter-thread synchronization costs.
You don't care how long you have to wait to receive the message.

Then this code is about as performant as it can be.

Note that "performance" in this case doesn't mean "fast". After you call SendTestMessage(), you could end up waiting a looong time before TryReceiveMessage() realizes that there's a message ready. If you have other things to do in the receive thread, that's fine I guess. But if all you're doing is a busy-wait spin... this is not the best design for this.
since std::memory_order_relaxed guarantees the ordering, it would get overwritten by SendTestMessage value if new load added after the std::memory_order_acquire is called. So, we will not miss any load.

I assume there's a typo here, and you mean to say: "since std::memory_order_ release guarantees the ordering...." Because std::memory_order_relaxed does literally the opposite of guaranteeing ordering.

But that's just a guess, because I can't make any sense of what you said. I can't even figure out what "it" is that you're concerned about being overwritten: Do you mean g_guard or g_payload? Are you asking about if you try to call SendTestMessage() twice? I can't figure out what you're asking.

Is this the best way to do this? That depends on what "this" is; you didn't say. It's a passable demonstration on how to use thread fences... but it doesn't look like anything that would be useful in a real-world program generally. This setup works for sending exactly one message in the entire lifetime of the program, and receiving it exactly once in a single thread. That's not very useful, generally.

If what you want is a way to set up a producer-consumer operation between threads of any kind - multi-consumer, multi-producer, whatever - there are much better ways to do it, generally.

Relaxed memory ordering is one of the deepest and darkest corners of C++, and you really shouldn't be screwing around with it unless you're an expert. Unless and until you've done everything else possible to speed up your code in a bottleneck, and have confirmed that it is inter-thread synchronization holding you up, you shouldn't even consider touching relaxed memory ordering. And even then, you shouldn't.

I'd say the bottom line of this review is: whatever you're planning to do with this code... don't.


Widget setup for Tkinter-based game I'm thinking of making a little strategic game using tkinter (player should interact with it using buttons). But my code is very repetitive.  This is a mock-up of my program:

import tkinter as tk
import random

def multi(*args):
    for func in args:
        func()

def show_hide():
    if not button["text"]:
        button.configure(text="Perform action #1", bd=2, bg="white", command=activate_deactivate)
        status_label["text"] = "Status: {}".format(status.get())
        button_2.configure(text="Perform action #2", bd=2, bg="white", command=activate_deactivate_2)
        status_label_2["text"] = "Status: {}".format(status_2.get())
    else:
        button.configure(text="", bd=0, bg="#F0F0F0", command=None)
        status_label["text"] = ""
        button_2.configure(text="", bd=0, bg="#F0F0F0", command=None)
        status_label_2["text"] = ""

def activate_deactivate():
    if status.get() == "Can be done":
        status.set("To be done")
        status_label.configure(text="Status: {}".format(status.get()), fg="blue")
    else:
        status.set("Can be done")
        status_label.configure(text="Status: {}".format(status.get()), fg="black")

def activate_deactivate_2():
    if status_2.get() == "Can be done":
        status_2.set("To be done")
        status_label_2.configure(text="Status: {}".format(status.get()), fg="blue")
    else:
        status_2.set("Can be done")
        status_label_2.configure(text="Status: {}".format(status.get()), fg="black")

def step_forward():
    if status.get() == "To be done":
        button.configure(text="", bd=0, bg="#F0F0F0", state="disabled")
        status_label["text"] = ""
        result = random.choice(["success", "failure"])
        if result == "success":
            status.set("Accomplished")
            status_label["fg"] = "green"
        else:
            status.set("Failed")
            status_label["fg"] = "red"
    else:
        button.configure(text="", bd=0, bg="#F0F0F0", command=None)
        status_label["text"] = ""

def step_forward_2():
    if status_2.get() == "To be done":
        button_2.configure(text="", bd=0, bg="#F0F0F0", state="disabled")
        status_label_2["text"] = ""
        result = random.choice(["success", "failure"])
        if result == "success":
            status_2.set("Accomplished")
            status_label_2["fg"] = "green"
        else:
            status_2.set("Failed")
            status_label_2["fg"] = "red"
    else:
        button_2.configure(text="", bd=0, bg="#F0F0F0", command=None)
        status_label_2["text"] = ""

root = tk.Tk()

status = tk.StringVar()
status.set("Can be done")

status_2 = tk.StringVar()
status_2.set("Can be done")

main = tk.Button(root, text="Show/Hide", bg="white", font="courier 30", command=show_hide)
main.pack()

frame = tk.Frame(root, pady=10)
frame.pack()

frame_1 = tk.Frame(frame, padx=10)
frame_1.pack(side="left")

frame_2 = tk.Frame(frame, padx=10)
frame_2.pack(side="left")

button = tk.Button(frame_1, text="", font="courier 20", bd=0)
button.grid(row=0, column=0)

status_label = tk.Label(frame_1, text="", font="courier 14")
status_label.grid(row=1, column=0)

button_2 = tk.Button(frame_2, text="", font="courier 20", bd=0)
button_2.grid(row=0, column=1)

status_label_2 = tk.Label(frame_2, text="", font="courier 14")
status_label_2.grid(row=1, column=1)

next_day = tk.Button(root, text="Next day", bg="white", font="courier 30",
                     command=lambda: multi(step_forward, step_forward_2))
next_day.pack()

root.mainloop()

This definitely looks terrible. With blank lines, it takes 100+ lines of code just to make 2 buttons (well, it's not only "2 buttons"), but I want to have many of these (a few groups each one containing around 4+ buttons which is a lot). If you know how "this" can be changed, I'd love to hear any suggestions (although this code is bad, it perfectly represents functionality and look of the interactive part of the UI, so these should remain the same).

The main issues, with your code, are that:

you do not factorize part of your code that are similar by correctly parametrizing your functions; and
you do not manage any common state between entities that are related.

But first, let's talk a bit about multi and __name__.

First, you can remove the need for a lambda by having multi return a callable that you will be able to provide to the command parameter. Writting something allong the lines of ..., command=multi(step_forward, step_forward_2)):

def combine(*callables):
    def runner():
        for function in callables:
            function()
    return runner

I also changed names, I feel these are more explicit.

Second, you should wrap your top-level code under an if __name__ == '__main__': clause. That way, if I want to use the building blocks you provide, I can import your file and build around it without having your UI pop up.

Now onto simplifying your code.

There is at least 3 things you are trying to achieve all at once:

Build logical blocks such as a button and a label in a frame;
Interact with such blocks from "the outside"; I believe the show/hide should be more generic than the "next step" one, but not knowing what you want to achieve with such UI, I can be misleading;
Manage states and control flow to have all your parts work together.

The first part is pretty straightforward: if you want several elements that behave like a single entity, you build a class to hold these elements together. Having done that, the second one comes naturally: you provide methods on your class so that "the outside" can interact with it. Managing states are thus nearly done: combine fed by instance method is very helpful. You only need to set it as a command on UI elements once each of them has been built.

In the following improvements, you’ll see that I provide several level of customization to be able to interact with the group of item both from the inside and the outside. Depending on your needs, you can merge some or detail even more:

import tkinter as tk
import random

class ButtonWithStatus:
    def __init__(self, root, pack_side, frame_config=None, button_config=None, label_config=None):
        if frame_config is None:
            frame_config = {}
        self._status_config = {} if label_config is None else label_config
        self._button_config = {} if button_config is None else button_config
        self._button_config['command'] = self.on_click
        self.hidden = True

        frame = tk.Frame(root, **frame_config)
        frame.pack(side=pack_side)
        self._hidden_background_color = frame.cget('bg')
        # Build hidden button
        parameters = self.hidden_parameters(self._button_config)
        self.button = tk.Button(frame, **parameters)
        self.button.grid(row=0, column=0)
        # Build label with hidden text
        parameters = self.hidden_parameters(self._status_config, False)
        self.label = tk.Label(frame, **parameters)
        self.label.grid(row=1, column=0)

    def hidden_parameters(self, configuration, is_button=True):
        parameters = configuration.copy()
        parameters.update(text='', bg=self._hidden_background_color)
        if is_button:
            parameters['command'] = None
        return parameters

    def on_click(self):
        raise NotImplementedError

    def show_hide(self):
        if self.hidden:
            self.button.configure(**self._button_config)
            self.label.configure(**self._status_config)
        else:
            parameters = self.hidden_parameters(self._button_config)
            self.button.configure(**parameters)
            parameters = self.hidden_parameters(self._status_config, False)
            self.label.configure(**parameters)
        self.hidden = not self.hidden

class ActivableButton(ButtonWithStatus):
    STATUS = 'Status: {}'

    def __init__(self, root, button_text, inactive_status, active_status):
        super().__init__(root, 'left', {'padx': 10},
                         {'text': button_text, 'font': 'courier 20', 'bd': 0},
                         {'text': self.STATUS.format(inactive_status), 'font': 'courier 14'})
        self.activated = False
        self._active = active_status
        self._inactive = inactive_status

    def on_click(self):
        self.activated = not self.activated
        if self.activated:
            self._status_config.update(text=self.STATUS.format(self._active), fg='blue')
        else:
            self._status_config.update(text=self.STATUS.format(self._inactive), fg='black')
        self.label.configure(**self._status_config)

class ValidableActivableButton(ActivableButton):
    def __init__(self, root, button_text):
        super().__init__(root, button_text, 'Can be done', 'To be done')
        self._done = False

    def step_forward(self):
        if self._done:
            return

        if self.activated:
            self._button_config.update(text='', bd=0, bg='#F0F0F0', state='disabled')
            if random.randint(0, 1):
                self._status_config.update(text='Accomplished', fg='green')
            else:
                self._status_config.update(text='Failed', fg='red')
            self._done = True
        self.button.configure(**self._button_config)
        self.label.configure(**self._status_config)

def combine(*callables):
    def runner():
        for function in callables:
            function()
    return runner

if __name__ == '__main__':
    root = tk.Tk()
    main = tk.Button(root, text="Show/Hide", bg="white", font="courier 30")
    main.pack()

    frame = tk.Frame(root, pady=10)
    frame.pack()

    button1 = ValidableActivableButton(frame, 'Perform action #1')
    button2 = ValidableActivableButton(frame, 'Perform action #2')

    main['command'] = combine(button1.show_hide, button2.show_hide)

    next_day = tk.Button(root, text="Next day", bg="white", font="courier 30",
                         command=combine(button1.step_forward, button2.step_forward))
    next_day.pack()

    root.mainloop()

Several other improvements:

usage of booleans instead of text to determine toggable state;
usage of the "default" background color of the container to determine the background color of the hidden state;
usage of dictionaries to store/restore widget configuration.

Depending on your needs, you may want to add/remove some of the parameter of the constructors. Or even combine ValidableActivableButton and ActivableButton into a single class.

You also don't really need to manage the hidden state yourself by removing text (and optionnally changing background color) as your can hide widgets pretty easily. This allows you to manipulate your widgets directly or using configure without worying about storing their state for future hide/show:

import tkinter as tk
import random

class ButtonWithStatus:
    def __init__(self, root, pack_side, frame_config=None, button_config=None, label_config=None):
        if frame_config is None:
            frame_config = {}
        if button_config is None:
            button_config = {}
        if label_config is None:
            label_config = {}
        button_config['command'] = self.on_click

        frame = tk.Frame(root, **frame_config)
        frame.pack(side=pack_side)
        self.button = tk.Button(frame, **button_config)
        self.button.grid(row=0, column=0)
        self.label = tk.Label(frame, **label_config)
        self.label.grid(row=1, column=0)

        # Hide label and button
        self.button.grid_remove()
        self.label.grid_remove()
        self.hidden = True

    def on_click(self):
        raise NotImplementedError

    def show_hide(self):
        if self.hidden:
            self.button.grid()
            self.label.grid()
        else:
            self.button.grid_remove()
            self.label.grid_remove()
        self.hidden = not self.hidden

class ActivableButton(ButtonWithStatus):
    STATUS = 'Status: {}'

    def __init__(self, root, button_text, inactive_status, active_status):
        super().__init__(root, 'left', {'padx': 10},
                         {'text': button_text, 'font': 'courier 20', 'bd': 0},
                         {'text': self.STATUS.format(inactive_status), 'font': 'courier 14'})
        self.activated = False
        self._done = False
        self._active = active_status
        self._inactive = inactive_status

    def on_click(self):
        self.activated = not self.activated
        if self.activated:
            self.label['text'] = self.STATUS.format(self._active)
            self.label['fg'] = 'blue'
        else:
            self.label['text'] = self.STATUS.format(self._inactive)
            self.label['fg'] = 'black'

    def step_forward(self):
        if self._done:
            return

        if self.activated:
            self.button['text'] = ''
            self.button['bd'] = 0
            self.button['bg'] = '#F0F0F0'
            self.button['state'] = 'disabled'
            if random.randint(0, 1):
                self.label['text'] = 'Accomplished'
                self.label['fg'] = 'green'
            else:
                self.label['text'] = 'Failed'
                self.label['fg'] = 'red'
            self._done = True

def combine(*callables):
    def runner():
        for function in callables:
            function()
    return runner

if __name__ == '__main__':
    root = tk.Tk()
    main = tk.Button(root, text="Show/Hide", bg="white", font="courier 30")
    main.pack()

    frame = tk.Frame(root, pady=10)
    frame.pack()

    button1 = ActivableButton(frame, 'Perform action #1', 'Can be done', 'To be done')
    button2 = ActivableButton(frame, 'Perform action #2', 'Can be done', 'To be done')

    main['command'] = combine(button1.show_hide, button2.show_hide)

    next_day = tk.Button(root, text="Next day", bg="white", font="courier 30",
                         command=combine(button1.step_forward, button2.step_forward))
    next_day.pack()

    root.mainloop()


I need some suggestions to speed up my array manipulation code I have a function which from an array of days return the array of days with it's date like from array of
['Sun', 'Mon', 'Tue', 'Wed' ] my function returns the days from today if there are some past days i switch the date to next week so the array will become  ['Wed 23', 'Sun 27', 'Mon 28', 'Tue 29' ]
And here is how my function looks like (i've added all comments that specify what i'm doing on each step:
  moment.locale('it'); // setting locale to italian as API response returns it formatted days

    const days = ['lun', 'mar', 'mer', 'gio', 'ven', 'sab', 'dom']; // mapping weekdays which will be used to get index of the day to use in isoWeekday

    const momentToday = moment().isoWeekday(); // setting today date
    const arrayGiorni = giorni.map((g) => {
      const dayIndex = days.indexOf(g.giorno.toLowerCase()); // getting index of day from the object array es if g.giorno is 'mar' index will be 2
      if (momentToday <= dayIndex) { // checking if today is <= to mapping object then adding it to array
        return {
          id: g.id,
          giorno: moment().isoWeekday(dayIndex),
        };
      } else { // else skipping to next week and adding the object to array
        return {
          id: g.id,
          giorno: moment()
            .add(1, 'weeks')
            .isoWeekday(dayIndex),
        };
      }
    });

// sorting the array to get an ordered array of dates
    arrayGiorni.sort((a, b) => moment(a.giorno).valueOf() - moment(b.giorno).valueOf());

// formatting the moment object to get string formatted date
    this.giorni = arrayGiorni.map(g => g.giorno.format('ddd DD MMM'));

    console.log(this.giorni);

As asked in comments here is the model of giorni:
export class NegozioGiorni {
    constructor(
        public id: number,
        public giorno: string,
        public orari: string[] = []
    ) {}
}

That in array of objects will be [ { id: 1, giorno: 'lun', orari: ['12:00', '13:00'] }, { id: 2, giorno: 'mer', orari: ['12:00', '13:00'] }, { id: 2, giorno: 'ven', orari: ['15:00', '17:00'] } ]
I was wondering if there is something that i can reduce in this come and if there is a way to make it more performable.

Speed You say you want the program to run more quickly, but it looks pretty good to me already. Are you really processing such a huge number of giorni objects that the function doesn't run fast enough? (It's possible, but it sounds unlikely.) But there are some improvements that could be made.
indexOf vs object You use indexOf to find the position of the day in the days array. This has O(n) complexity; the interpreter searches through the array indicies one-by-one until it finds a match. It would be less computationally expensive to use an object mapping day names to their indicies, then look up the name on the object, in a single operation.
Conditional values Inside the .map, the only part that changes between the if and else is the giorno property. To do this more efficiently and concisely, use the conditional operator instead:
return {
  id: g.id,
  giorno: momentToday <= dayIndex
    ? moment().isoWeekday(dayIndex)
    : moment().add(1, 'weeks').isoWeekday(dayIndex),
};

. Or, since it you don't actually care about the id properties, you can remove them entirely, and just use an array of times.
Moment Moment is a non-trivial library (and even it recommends to use modern APIs instead, like Date). If speed is an issue, you could use the native Date object instead.

const giorni = [{
  id: 1,
  giorno: 'lun', // Monday; turns into Mon 28 Sep
  orari: ['12:00', '13:00']
}, {
  id: 2,
  giorno: 'mer', // Wednesday: turns into Wed 23 Sep
  orari: ['12:00', '13:00']
}, {
  id: 2,
  giorno: 'ven', // Friday: turns into Fri 25 Sep
  orari: ['15:00', '17:00']
}];
const dayIndiciesByDayName = Object.fromEntries(
  ['dom', 'lun', 'mar', 'mer', 'gio', 'ven', 'sab']
    .map((name, i) => [name, i])
);

const todayDayIndex = new Date().getDay(); // Zero-indexed and starts at Sunday
const todayDayOfMonth = new Date().getDate();
const inputDays = giorni.map(({ giorno }) => giorno);
const itemDates = giorni.map((g) => {
  const itemDayIndex = dayIndiciesByDayName[g.giorno.toLowerCase()];
  // Below will be 0 to 6:
  const itemDayDifferenceFromToday = (itemDayIndex - todayDayIndex + 7) % 7;
  const itemDateObj = new Date();
  itemDateObj.setDate(todayDayOfMonth + itemDayDifferenceFromToday);
  return itemDateObj;
});
const output = itemDates
  .sort((a, b) => a - b)
  .map((date) => {
    const match = date.toString().match(/(\w+ )(\w+) (\d+ )/);
    return match[1] + match[3] + match[2];
  });
console.log(output);


Writing a utility class for converting between datetime and timestamp I'm writing an Python application that uses frequently datetime and Unix timestamp. I know Python is 'batteries included', however, I found that converting between datetime and timestamp in Python 2.6 is not trivial. That's why I wrote this utility class. This code works as expected, for Python 2.6, 2.7, and 3.4.

I want the code style of my code reviewed. I don't know whether my function names like dttomicrotimestamp and code styles (like @staticmethod) are suitable for pythonic way of coding.

#!usr/bin/python
# -*- coding: utf-8 -*-
from __future__ import absolute_import, print_function, division

import time
import datetime
import pytz
from tzlocal import get_localzone

class datetimeutil:
    @staticmethod
    def strtodt(text):
        try:
            dt = datetime.datetime.strptime(text, "%Y-%m-%d %H:%M:%S.%f")
        except:
            dt = datetime.datetime.strptime(text, "%Y-%m-%d %H:%M:%S")
        return get_localzone().localize(dt)

    @staticmethod
    def dttotimestamp(dt):
        if dt.tzinfo is None:
            dt = get_localzone().localize(dt)
        return datetimeutil._totalseconds(
            (dt - dt.utcoffset()).replace(tzinfo=None) -
            datetime.datetime(1970, 1, 1))

    @staticmethod
    def dttomicrotimestamp(dt):
        return int(datetimeutil.dttotimestamp(dt) * 1e6)

    @staticmethod
    def timestamptodt(timestamp):
        return get_localzone().localize(datetime.datetime.fromtimestamp(timestamp))

    @staticmethod
    def microtimestamptodt(microtimestamp):
        return datetimeutil.timestamptodt(microtimestamp / 1e6)

    @staticmethod
    def _totalseconds(timedelta):
        return ((timedelta.seconds + timedelta.days * 24 * 3600) * 1e6
                + timedelta.microseconds) / 1e6

Let's examine the code:

#!usr/bin/python

you don't need to execute the code directly, so why this line?

class datetimeutil:
    @staticmethod

why a class with static methods? Keep it simple, write global functions. You already have the module which encapsulates the functions.

    def strtodt(text):

I don't like too much the names of these functions. I know that also python uses ugly names (words without any separator) but I think you could improve this... for example: str_to_dt or str2dt seem better to me.

About the functionality. Managing dates is a quite difficult task. The python library in my opinion makes a great job in distinguishing naive and aware datetimes. Your wrapper module simplifies this by considering each naive datetime as a localtime (if I understand it correctly). I think it is not good to hide this decision in your code. Explicit is better, even if it could me more verbose.

Also I think you missed the timedelta.total_seconds function, which could be used in your code.


Perform sum of squared numbers without using for loop I have to find the sum of number after performing square operation in the given input the negative numbers need to be ignored. The input Format is, first line is number of test cases followed by N as number of test inputs in new line followed by N space seperated numbers. The task need to be done using golang without using for loop. Output need to be print in new line for each test case.
3
4
3 -1 1 14
5
9 6 -53 32 16
6
3 -4 12 2 5 7

Below is my attempt to achieve the task using recurrsion.
package main

import "fmt"

func findSquareSum(x int, a []int, iteration int) {
    var num int
    if x > 0 {
        fmt.Scanf("%d", &num)
        if num > 0 {
            a[iteration-1] += num * num
        }
        findSquareSum(x-1, a, iteration)
    }
}

func readTest(x int, a []int){
    var input int
    if x > 0{
        fmt.Scanf("%d", &input)
        findSquareSum(input,a,x)
        readTest(x-1,a)
    }
}

func printResult(x int, a []int){
    if (x > 0){
        fmt.Println(a[x-1])
        printResult(x-1,a)
    }
}

func main() {
    var test int

    fmt.Scanf("%d", &test)

    a := make([]int, test)

    readTest(test,a)

    printResult(test,a)
}

My question is as there is no base case in the recursive function. How the programm is working as per expectations?

There is a base case... it's the line if x > 0 {
compare to the for loop version
for x=input: x>0: x=x-1 {
findSquareSum(input,a,x) sets x=input
if x > 0 { is the condition check
findSquareSum(x-1, a, iteration) decrements x
also consider the modified fucntion
func findSquareSum(x int, a []int, iteration int) {
    var num int
    if x <= 0 { return }

    fmt.Scanf("%d", &num)
    if num > 0 {
        a[iteration-1] += num * num
    }
    findSquareSum(x-1, a, iteration)
}

now it should look more like the base cases we are to seeing. It should also be clear that this is equivalent to the askers version (unless my go syntax is wrong!)
similarly
func readTest(x int, a []int){
    var input int
    if x <= 0 { return }
    fmt.Scanf("%d", &input)
    findSquareSum(input,a,x)
    readTest(x-1,a)
}


Character pattern replacement using gsub, loops, and data.table I was trying to see if data.table could speed up a gsub pattern matching function over a list.

Data for reprex. It's a list of 3 data frames with some asterisks placed here and there. Each data frame is 6500 rows, 2 columns, and generally representative of my actual data. My data does have multiple columns per data frame that need to be looped over, which is why I'm using the mapply.

library(data.table)
library(microbenchmark)
    df1 <- data.frame(name = rep(LETTERS, 250), code = rep(letters, 250), stringsAsFactors = FALSE)
df1$name[df1$name == "D" | df1$name == "F" | df1$name == "L"] <- "foo*"
df1$code[df1$code == "d" | df1$code == "f" | df1$code == "l"] <- "*foo"
df2 <- data.frame(name = rep(LETTERS, 250), code = rep(letters, 250), stringsAsFactors = FALSE)
df2$name[df2$name == "A" | df2$name == "R" | df2$name == "T"] <- "foo*"
df2$code[df2$code == "a" | df2$code == "r" | df2$code == "t"] <- "*foo*"
df3 <- data.frame(name = rep(LETTERS, 250), code = rep(letters, 250), stringsAsFactors = FALSE)
df3$name[df3$name == "C" | df3$name == "Q" | df3$name == "W"] <- "foo*"
df3$code[df3$code == "c" | df3$code == "q" | df3$code == "w"] <- "*f*oo"

df <- list(df1, df2, df3)
dt <- lapply(df, as.data.table)

In this example, I am trying to remove any * symbols from character strings. First function was just using an mapply and gsub. It deletes any *, looping over elements. Second was an attempt to do it using the data.table library.

mapply.remove.asterisk = function(x){
  df2 <- data.frame(mapply(gsub, "\\*", "", x, perl = TRUE))
  colnames(df2) <- colnames(x)
}

dt.remove.asterisk = function (x) {
  x[, lapply(.SD, function(x) gsub("\\*", "", x, perl = TRUE))]
}

Testing them out doesn't show a big difference, but the mapply is slightly slower.

mapgsubtest = function(x) {
  df.test <- lapply(x, mapply.remove.asterisk)
}

dtgsubtest = function(x) {
  dt.test <- lapply(x, dt.remove.asterisk)
}

microbenchmark(mapgsubtest(df), dtgsubtest(dt), neval = 100)

Unit: nanoseconds
            expr     min      lq       mean  median      uq      max neval
 mapgsubtest(df) 7161991 7388846 7780101.83 7483794 7651907 27860732   100
  dtgsubtest(dt) 6759663 6991926 7181127.95 7109710 7275418 10102686   100
           neval       0       0      12.26       0       1      902   100

Is there something I'm doing within data.table that could be improved? I tried to see if a few things sped everything up, like having * only at the end of strings (only foo*), using an end of string regex anchor $, and setting an index key. Nothing changed noticeably.

Is there a reason that you are using mapply to gsub through the number columns as well? You can just replace in the first column if that is all you need, which gets some speed improvement, about 2x on my machine. I also tried using stringi instead of gsub but it was not faster. This is also a speedup on the order of milliseconds though!

library(data.table)
#> Warning: package 'data.table' was built under R version 3.5.1
library(microbenchmark)
#> Warning: package 'microbenchmark' was built under R version 3.5.1
library(stringi)
df1 <- data.frame(name = rep(LETTERS, 250), number = rep(c(1:26), 250), stringsAsFactors = FALSE)
df1$name[df1$name == "D" | df1$name == "F" | df1$name == "L"] <- "foo*"
df2 <- data.frame(name = rep(LETTERS, 250), number = rep(c(1:26), 250), stringsAsFactors = FALSE)
df2$name[df2$name == "A" | df2$name == "R" | df2$name == "T"] <- "*foo*"
df3 <- data.frame(name = rep(LETTERS, 250), number = rep(c(1:26), 250), stringsAsFactors = FALSE)
df3$name[df3$name == "C" | df3$name == "Q" | df3$name == "W"] <- "f*oo"

df <- list(df1, df2, df3)
dt <- lapply(df, as.data.table)

mapply.remove.asterisk = function(x){
  df2 <- data.frame(mapply(gsub, "\\*", "", x, perl = TRUE))
  colnames(df2) <- colnames(x)
}

dt.remove.asterisk = function (x) {
  x[, lapply(.SD, function(x) gsub("\\*", "", x, perl = TRUE))]
}

stringi.remove.asterisk = function (x) {
  out <- x
  out$name <- stri_replace_all_regex(x$name, "\\*", "")
  out
}

gsub.remove.asterisk = function(x) {
  out <- x
  out$name <- gsub("\\*", "", x$name)
  out
}

mapgsubtest = function(x) {
  df.test <- lapply(x, mapply.remove.asterisk)
}

dtgsubtest = function(x) {
  dt.test <- lapply(x, dt.remove.asterisk)
}

strisubtest = function(x) {
  str.test <- lapply(x, stringi.remove.asterisk)
}

gsubtest = function(x){
  gsub.test <- lapply(x, gsub.remove.asterisk)
}

microbenchmark(mapgsubtest(df), dtgsubtest(dt), strisubtest(df), gsubtest(df))
#> Unit: milliseconds
#>             expr      min       lq     mean   median       uq       max
#>  mapgsubtest(df) 8.031179 8.789332 9.429985 9.164945 9.740215 12.913776
#>   dtgsubtest(dt) 7.276307 7.867076 8.553440 8.217892 8.855339 22.473660
#>  strisubtest(df) 8.149333 8.745572 9.391304 9.221469 9.846153 13.492875
#>     gsubtest(df) 4.153983 4.667258 5.053101 4.789789 5.231771  9.813332
#>  neval
#>    100
#>    100
#>    100
#>    100

Created on 2018-10-20 by the reprex package (v0.2.0).


Which is a better style to write default return case in if-else private String GetFacilityName(String facilityHexID) {
    if (facilityHexID.equals(FACILITY_AIRCON_HEX_ID)) {
       return FACILITY_AIRCON_NAME;
    }
    else if (facilityHexID.equals(FACILITY_FAN_HEX_ID)) {
       return FACILITY_FAN_NAME;
    }

    return facilityHexID;
}

Or

private String GetFacilityName(String facilityHexID) {
    if (facilityHexID.equals(FACILITY_AIRCON_HEX_ID)) {
       return FACILITY_AIRCON_NAME;
    }
    else if (facilityHexID.equals(FACILITY_FAN_HEX_ID)) {
       return FACILITY_FAN_NAME;
    }
    else {
       return facilityHexID;
    }
}

They essentially do the same thing. I'm thinking first one is better from the standpoint of "Write more code only if you have to" and the second one is better from readability aspect.

I'd use a switch statement with default case but I'm using a old version of JDK.. so I can't use it on Strings.

Which one is better?

I'd remove both elses:

private String getFacilityName(String facilityHexId) {
    if (FACILITY_AIRCON_HEX_ID.equals(facilityHexId)) {
       return FACILITY_AIRCON_NAME;
    }
    if (FACILITY_FAN_HEX_ID.equals(facilityHexId)) {
       return FACILITY_FAN_NAME;
    }
    return facilityHexId;
}

I think it's easier to follow. (It's called Guard Clause.)

Some other notes:

I've switched the equals to CONSTANT.equals(inputParameter). Now it handles null inputs too and does not throw NullPointerException.
I've renamed the method to camelCase. It's more common in Java to start method names with small letters.  (Code Conventions for the Java Programming Language, 9 - Naming Conventions)
I've changed ID to Id in the variable names. I prefer camelCase here too because it's usually easier to read. From Effective Java, 2nd edition, Item 56: Adhere to generally accepted naming conventions:

  While uppercase may be more common,
  a strong argument can made in favor of capitalizing only the first
  letter: even if multiple acronyms occur back-to-back, you can still
  tell where one word starts and the next word ends.
  Which class name would you rather see, HTTPURL or HttpUrl?


Grouping sorted coordinates based on proximity to each other I created an algotrithm that groups a sorted list of coordinates into buckets based on their proximity (30) to one another.

Steps:

Create a new key with a list value and pop the first point in the list into it
Scan the list of points for points that are close to it. Push matches to the new list and replace values with None
After scan, filter the list, removing None values
Back to one, until points is empty.

I can't modify the list by deleting elements because I use indexes.

After I'm done grouping, I take the average of each group of points.

def group_points(points):
    groups = {}
    groupnum = 0
    while len(points) > 1:
        groupnum += 1
        key = str(groupnum)
        groups[key] = []
        ref = points.pop(0)
        for i, point in enumerate(points):
            d = get_distance(ref, point)
            if d < 30:
                groups[key].append(points[i])
                points[i] = None
        points = list(filter(lambda x: x is not None, points))
    # perform average operation on each group
    return list([[int(np.mean(list([x[0] for x in groups[arr]]))), int(np.mean(list([x[1] for x in groups[arr]])))] for arr in groups])

def get_distance(ref, point):
    # print('ref: {} , point: {}'.format(ref, point))
    x1, y1 = ref[0], ref[1]
    x2, y2 = point[0], point[1]
    return math.hypot(x2 - x1, y2 - y1)

If possible, I would like to reduce the amount of variables and total loops over the points array. Do I need to use indexes? Is it possible to achieve this in one pass over points?

General comments:

You are basically using the groups dict like a list.  Might as well just use a list.

An empty data structure (list, dict, set, tuple) is False in a boolean context, so while len(points) > 1: can be simplified to while points:

It is generally slower to pop from the front of a list than the back of a list, because after removing the first item all the rest of the items get moved up one spot.

points.pop() actually changes the list passed in.  Make sure that's what you want.

filter(None, points) filters out all "False" items.

[ ... ] creates a list.  So, list( [ ... ] ) is redundant.

You can just use x1, y1 = ref.

Put that all together and you get something like:

def group_points(points):
    groups = []
    while points:
        far_points = []
        ref = points.pop()
        groups.append([ref])
        for point in points:
            d = get_distance(ref, point)
            if d < 30:
                groups[-1].append(point)
            else:
                far_points.append(point)

        points = far_points

    # perform average operation on each group
    return [list(np.mean(x, axis=1).astype(int)) for x in groups]

def get_distance(ref, point):
    # print('ref: {} , point: {}'.format(ref, point))
    x1, y1 = ref
    x2, y2 = point
    return math.hypot(x2 - x1, y2 - y1)

You also might want to look at functions in scipy.cluster.


Automating a set a of weekly reports, including graphs and delivery of reports I have been writing code to automate some weekly reports. I had help over on Stack Overflow. I have code that works for the most part, however there are a few things that I just can't seem to fix.

In short, I loop through the data and create a dictionary of dataframes based on 'location' key unique values. I can use the dictionary to make summary reports for each location. I wanted to make another dictionary from this based on 'sublocation.' Instead with some advice, I make a list of each sublocation, access each item in the df-dict, loop to find corresponding sublocations and make plots.

My problems are as follows:

Code is slow
Graphs are not formatted properly (overlapping even with tight_layout)
For the reports in sublocation, I am having a hard time saving to the right folder. I think this has to do with the way I want to format the string in the savefig text. For each sublocation I want reference the name using value['location'], I think this is always updated every loop so it doesn't work.
I have the error exception because when looking to match subloc to loc. not every subloc will appear in the dict value dataframe

f = 'path'
d = pd.DataFrame()
d= pd.read_csv(f)
dfs = dict(tuple(d.groupby('location')))
for key, value in dfs.items():
    try:
        fig, axs = plt.subplots(2, 3);
        sns.countplot(y='ethnic', data=value, orient='h', palette ='colorblind', ax=axs[0,0]);
        sns.countplot(y='Ratio', data=value,orient='v', palette ='colorblind',ax=axs[1,0]);
        sns.countplot(y='site', data = value, ax=axs[0,1]);
        sns.countplot(y='STATUS', data = value, ax = axs[1,1])
        sns.countplot(y='Assessment', data = value, ax = axs[0,2])
        #pth = os.path.join(tmppath, '{0}'.format(key))
        for p in axs.patches:
            ax.text(p.get_x() + p.get_width()/2., p.get_width(), '%d' % int(p.get_width()),
            fontsize=12, color='red', ha='center', va='bottom')
        plt.tight_layout(pad=2.0, w_pad=1.0, h_pad=2.0);
        plt.set_title('{0}'.format(key)+'Summary')
        plt.savefig("basepath/testing123/{0}/{1}.pdf".format(key,key), bbox_inches = 'tight');
        plt.clf()

        #plt.show()
    except:
        plt.savefig("basepath/{0}/{1}.pdf".format(key,key), bbox_inches = 'tight');
        #plt.savefig("{0}.pdf".format(key), bbox_inches = 'tight');
        pass

#####Now for sublocations

dfss = dict(tuple(d.groupby('site')))

#%%

for key, value in dfss.items():
    a =(repr(value['school_dbn'][:1]))

    try:
        fig, axs = plt.subplots(2, 3);
        #tmppath = 'basepath/{0}'.format(key);
        sns.countplot(y='ethnic', data=value, orient='h', palette ='colorblind', ax=axs[0,0]);
        sns.countplot(y='Program]', data=value,orient='v', palette ='colorblind',ax=axs[1,0]);
        sns.countplot(y='AltAssessment', data = value, ax = axs[0,2])
        pth = os.path.join(tmppath, '{0}'.format(key))
        plt.tight_layout(pad=2.0, w_pad=1.0, h_pad=2.0);
        plt.set_title('{0}'.format(key)+'Summary')
        plt.savefig("basepath/{0}/{1}_{2}.pdf".format(value['location'][-6:],value['location'][-6:],key), bbox_inches = 'tight');
        plt.clf()

        #plt.show()
    except:
        plt.savefig("basepath/testing123/{0}/{1}_{2}.pdf".format(value['location'][-6:],value['location'][-6:],key), bbox_inches = 'tight');
        #plt.savefig("{0}.pdf".format(key), bbox_inches = 'tight');
        pass

The reason why I want to save like this is because each location has a folder with same name. Sublocation belongs to only one location, therefore I want to save as 'location_sublocation.pdf'.

I got this done by making a second dictionary, which takes locations as keys and values as list of sublocations
dfs = dict(tuple(data.groupby('location')))
dfss = dict(tuple(data.groupby('sublocation')))

dd = {}

for key, value in dfs.items(): #dictionary is made of groupby object, key is
                               #location, value is datafram
    a = []
    dee={}
    for i in value['sublocation']:
        if i in a:
            pass
        else:
            a.append(str(i))
    dee = {key:a}
    dd.update(dee)
for key, value in dfss.items():
    try:
        for k, v in dd.items():
            if key in v:
                dur=str(k)
            else:
                pass
    except:
        pass

Then in the next cell,
for key, value in dfss.items():
    try:
        for k, v in dd.items():
            if key in v:
                dur=str(k)
            else:
                pass
        #tmp = value[value['sublocation']==i]
        sns.set(style='white', palette=sns.palplot(sns.color_palette(ui)), font='sans-serif')

I think I can make the overall script run even faster by employing more regex expressions for filtering the dataframe in various steps.
This set-up works because I can save the files according to the key's from the two dictionaries. It allows me to save the nearly 375 files automatically. I use another script to move the files to their respective folders.
plt.savefig("path/{0}/{1} @ {2}.pdf".format(dur,dur,key), bbox_inches = 'tight')

Having a slightly different case, take three data sets and make mini data sets based on some column such as location
oct_dict = dict(tuple(oct.groupby('location')))
oct2_dict = dict(tuple(oct2.groupby('location')))
for k, v in oct_dict.items():
    for k2, v2 in stu_dict.items(): #replace with v2 = stu_dict[k] if you know for sure it exits
        for k3, v3 in oct2_dict.items(): #replace with v3 = oct2_dict[k] if you know for sure it exits
            if k == k2 and k == k3: #can delete this if not needed
                plt.close('all')
                with PdfPages(r'path\{}.pdf'.format(k)) as pdf:


History homework class I'm yrying to practice some OO PHP and I'm just wondering if what I'm doing is okay or not. Please tell me if there are any alarms in my method so that I can stop doing it and learn a better way.

In my conditional statement, I was following the rule of no else keyword, based on this article by William Durand, which talks about Object Calisthenics.

I also don't know if I'm using the abstract properly, being that I learned it just a few minutes ago - but it works, so I suppose it's being used correctly.

<?php

        abstract class Homework
        {
            protected $coursesTaken = 0;
            protected $minimumCourses = 10;

            abstract function completedOrNot();
        }

        class HistoryHomework extends Homework
        {
            public function __construct($coursesTaken)
            {
                $this->coursesTaken = $coursesTaken;
            }

            public function completedOrNot()
            {
                if ($this->coursesTaken >= $this->minimumCourses)
                {
                    return "You completed the course in " . $this->coursesTaken
                    . " classes, but you only needed " . $this->minimumCourses
                    . " classes to complete the course! \n\n";
                }

                return "Sorry, you did not complete this course. You only took "
                . $this->coursesTaken . " classes, and you need a minimum of "
                . $this->minimumCourses . " to pass. \n\n";
            }
        }

        $student1 = new HistoryHomework(11);
        echo $student1->completedOrNot(); // You have completed the course in 11 classes, but you only needed 10 classes to complete the course!

        $student2 = new HistoryHomework(7);
        echo $student2->completedOrNot(); // Sorry, you did not complete this course. You only took 7 classes, and you need a minimum of 10 to pass.

It's opinion-driven, but general points:

You return large strings. Use try/catch for custom errors and warnings.
Manage specific messages separately from your OOP (for example, language files).
Separate layers, such as presentation, application, business logic, etc.

About the further structure: correct, but you didn't post serious logic.

There are many patterns to solve problems, such as Model-View-Controller (separating responsibilities), Adapter pattern, Factory pattern, etc. Read more here.


Should I copy list both in constructor and in getter? I have a simple immutable class:

public class ColumnsWrapper implements Columns {
    private final List<String> columnNames;

    public ColumnsWrapper(List<String> columnNames) {
        this.columnNames = new ArrayList<String>(columnNames);
    }

    @Override
    public List<String> getNames() {
        return new ArrayList<String>(columnNames);
    }

}

Should I always save a copy of passed list in constructor and return copy of saved list in getter?

This class from library project and can be used in several projects in the future.

The purpose of this class is only to store immutable list of column names.

It depends on what the goal is. Can the list never change after construction? Can it change but only the ColumnWrapper can do so? Or can it change and everyone is allowed to do that?

If the list can not change after construction, consider using an ImmutableList (from google's guava). You should then declare the field columnNames and the return value of getNames() to be of that type. You then either create an immutable list during construction or use that type for the constructor argument as well.

If the list can change after construction (but only ColumnsWrapper can do so), the getter should return an unmodifiableList. Note that this will cause exceptions if the client of your class tries to manipulate the list. You should then also copy during construction (as you currently do).

In any way should you document the behavior with comments on the respective public members (i.e. the constructor and the getter).

Edit

Ok, so the class has to be immutable. As @rolfl explains, it can be subclassed so this is not yet the case. You can either make the class final or make the constructor private and provide a static factory method.

Furthermore you have to make sure, that the list can not be modified. The easiest and most intention revealing way I know of is the ImmutableList I mentioned above. Another solution would look like this:

public class ColumnsWrapper implements Columns {

    private final List<String> columnNamesUnmodifiable;

    public ColumnsWrapper(List<String> columnNames) {
        List<String> columnNamesCopy = new ArrayList<>(columnNames);
        columnNamesUnmodifiable = Collections.unmodifiableList(columnNamesCopy);
    }

    // OPTION A

    @Override
    public List<String> getNamesUnmodifiable() {
        return columnNamesUnmodifiable;
    }

    // OPTION B

    @Override
    public Iterable<String> getNamesUnmodifiable() {
        return columnNamesUnmodifiable;
    }

}

Note that I changed the name to inform callers that they will get an unmodifiable instance.

I also provided another additional option (you have to choose one) with a different return type. If you are sure that callers will only iterate over the returned instance (as is often the case) the iterable will suffice. But it can not be used to add methods and since removal is an optional method (i.e. many iterators support no removal) it better conveys immutability.

I any way the interface documentation should also make that clear.


Bash script to determine a distro, update, upgrade and install the apps I use This is my first bash script. I am trying to learn scripting and needed an idea. This is the first idea I came up with. When I used to distro hop I had to reinstall everything. This helped me out once I got the first part working. Then I wanted to add if then else for more than just Debian distros.

#!/bin/bash

#Script Name : app_install.sh
#Author inflatus
#Created 4 Sept 2015
#Version 0.4

#Description
 #This script will determine the distributon of Linux and use the      appropriate package manager to install my core apps

current_distro=`cat /etc/*-release | grep "^ID=" | grep -E -o "[a-z]\w+"`
close_distro=`cat /etc/*-release | grep "^ID_LIKE=" | grep -E -o "[a-z]\w+"`

echo "$current_distro"
echo "$close_distro"

if [ "$close_distro" = "debian" ]; then

  #Debian
  #Install my apps on a fresh install of a Debian based Linux

  echo "This script will install your apps after a fresh install"

  apt-get update && apt-get dist-upgrade -y
  apt-get install htop traceroute whois gparted curl nmap openvpn rsync iptraf openssh-client git gimp hexchat unison -y

  #Adding repositories

  echo "Adding the repos"

  add-apt-repository ppa:atareao/telegram -y
  add-apt-repository ppa:mumble/release -y
  add-apt-repository ppa:stebbins/handbrake-releases -y
  add-apt-repository ppa:maarten-baert/simplescreenrecorder -y
  add-apt-repository ppa:remmina-ppa-team/remmina-next -y
  add-apt-repository ppa:libreoffice/libreoffice-5-1 -y
  add-apt-repository ppa:wireshark-dev/stable -y

  apt-get update
  apt-get install telegram mumble handbrake simplescreenrecorder remmina remmina-plugin-rdp libfreerdp-plugins-standard libreoffice wireshark -y
  apt-get dist-upgrade -y

  #Time to clean

  echo "Alright, cleaning now."

  apt-get autoclean

  #Finished install

  echo "Finished."

elif [ "$close_distro" != "debian" ]; then

  #Fedora
  #Install my apps on a fresh install of a Fedora based Linux

  echo "This script will install your apps after a fresh install"

  dnf check-update -y
  dnf update -y
  dnf install htop traceroute jwhois gparted curl nmap openvpn rsync iptraf openssh-clients git gimp mumble remmina-plugins-rdp wireshark hexchat -y
  dnf clean all

  #Time to clean

  echo "Alright, cleaning now."
  dnf clean all

  #Finished install

   echo "Finished."

 else

   echo "This is not for you."
fi

Simplifying if-elif-else

The else statement here is dead code, it can never happen:

if [ "$close_distro" = "debian" ]; then
  # commands to run if debian
elif [ "$close_distro" != "debian" ]; then
  # commands to run if NOT debian
else
   echo "This is not for you."
fi

The conditions can be simplified to this:

if [ "$close_distro" = debian ]; then
  # commands to run if debian
else
  # commands to run if NOT debian
fi

I also dropped the unnecessary double-quoting around the literal string debian.

Prefer $(...) over `...`

When using sub-shells, instead the obsolete `...`,
always use the modern syntax $(...).

Simplify a chain of commands with awk when possible

In each of these commands, there are 3 processes executed: cat, grep and another grep:

current_distro=`cat /etc/*-release | grep "^ID=" | grep -E -o "[a-z]\w+"`
close_distro=`cat /etc/*-release | grep "^ID_LIKE=" | grep -E -o "[a-z]\w+"`

You can make that more efficient by rewriting each using a single awk:

current_distro=$(awk -F= '$1 == "ID" { print $2 }' /etc/*-release)
close_distro=$(awk -F= '$1 == "ID_LIKE" { print $2 }' /etc/*-release)


Datagridview filter with BindingSource I want to perform a filter on a DataGridView using 3 different TextBox.
The solution below works, but does not consider very effective and good. Is there a way to improve it?

private void textBoxCod_TextChanged(object sender, EventArgs e)
{
    Filter();
}

private void textBoxArt_TextChanged(object sender, EventArgs e)
{
    Filter();
}

private void textBoxDesc_TextChanged(object sender, EventArgs e)
{
    Filter();
}

private void Filter()
{
    string queryFilter = "1=1"; //With this we avoid to check if there is the AND in the query
    var filterCod = textBoxCod.Text;
    var filterArt = textBoxArt.Text;
    var filterDescr = textBoxDesc.Text();
    if (filterCod != "")
        queryFilter += " AND product_code LIKE '%" + filterCod + "%'";
    if(filterArt != "")
        queryFilter += " AND article LIKE '%" + filterArt + "%'";
    if (filterDescr != "")
        queryFilter += " AND description LIKE '%" + filterDescr + "%'";

    bSource.Filter = queryFilter;
    dataGridView.DataSource = bSource;
}

Before I used the string.Format but I realized that not having optional parameters could not combine several filters.

Adding few improvements to @Steve's answer:

It is always good to check for spaces(blanks):     if(!string.IsNullOrWhiteSpace(filterCod))
Wouldn't be nicer to have queries.Any() instead of queries.Count >= 1 ?


Checking for a win along the top row of a Tic Tac Toe board This code works so far and it is a TicTacToe game (Personal Project of Mine). I'm kinda new to programming. My question is I really do not like how the Connected method is at all, because well it's fairly large for no reason. Is there a way to make it more compact because I have to add the middle and bottom row then do the vertical columns then do the diagonal lines and that will make method very big so what do you guys think?

import javax.swing.*;
import java.awt.*;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;

/**
 * Created by Usman on 5/17/2016.
 */
public class TicTacToeGUI extends JPanel {

    boolean turn = true;
    JButton[] board;

    public TicTacToeGUI() {
        setLayout(new GridLayout(3,3));

        board = new JButton[9];
        for (int i = 0; i < board.length; i++) {
            board[i] = new JButton("");
            board[i].setPreferredSize(new Dimension(70, 60));
            board[i].setFont(new Font("Arial", Font.PLAIN, 40));
            int finalI = i;
            board[i].addActionListener(new ActionListener() {
                @Override
                public void actionPerformed(ActionEvent e) {

                    if(board[finalI].isEnabled()){
                        if(turn){
                            board[finalI].setText("X");
                            turn = false;
                        }
                        else{
                            board[finalI].setText("O");
                            turn = true;
                        }
                        board[finalI].setEnabled(false);
                    }
                    Connected();
                }
            });
            add(board[i]);
        }
    }
    public void Connected(){
        //Horizontal top row
        if((!board[0].isEnabled()) || (!board[1].isEnabled()) || (!board[2].isEnabled())){
            if((board[0].getText() == board[1].getText()) && (board[0].getText() == board[2].getText())){
                if((!board[0].isEnabled()) == (!board[1].isEnabled()) == (!board[2].isEnabled())){
                    JOptionPane.showMessageDialog(null, "You've Won The Game!");
                }
            }
        }
    }
    public static void main(String []args){
        TicTacToeGUI game = new TicTacToeGUI();
        JFrame frame = new JFrame("Tic Tac Toe");
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.getContentPane().add(game);
        frame.pack();
        frame.setVisible(true);
    }
}

You're checking if each section of your board is enabled as part of your check, this check makes your test look more complicated than it needs to be.  Part of this is because your board is tightly integrated into its graphic representation.  Generally speaking you want to try to avoid this, it makes both layers easier to change.

Rather than having a signature of Connected() consider writing something more like this (pseudo code):

bool CheckWin(string currentPlayer) {
    foreach row in rows
        return true if CheckWinningRow(row, currentPlayer)
    // Check Columns
    // Check Diagonals
}

bool CheckWinningRow(int row, string currentPlayer) {
    return true if (board[row*3] == currentPlayer &&
                    board[row*3+1] == currentPlayer &&
                    board[row*3+2] == currentPlayer);
}

Checking each row is going to be the same action, it's just going to be a different starting location in your array.

You only need to check if the last player (currentPlayer, either X or O) has won.  If you work by checking the current player, you don't need to know if the button is enabled (has already been played) it's not going to have the current player's value in it if it hasn't been played on yet.


List of word to list of index in a vocabulary I have a dataset composed of ~700k samples. Each sample is composed of n_features features. Each feature is a word. Each feature has his own vocabulary. The size of the vocabularies range from 18 to 32000.

  np.shape(x) -> (n_samples, n_features)

Instead of having lists of features I would like to have lists of indexes corresponding to the indexes in vocabularies. This is my code:

vocs = [np.array(list(set(x[:,i]))) for i in range(np.shape(x)[1])]
x_new = [[np.argwhere(vocs[j]==x[i,j]) for j,feature in enumerate(features)] for i,features in enumerate(x)]

This code works but I wonder if there is a way to improve the performances. These 2 lines take 10min to run on my i7-7700HQ @ 2.8GHz.

Edit:

For more context, what I'm working on is natural language processing. I want to train a classifier to predict relations between words in a sentence. For this I have a conllu file which give sentences and for each word of each sentence a list of features and with which word it's related and how. These features may be the word itself, its lemma, its position in the sentence etc... I'm trying a different set of features and type of embedding and I want to test the embedding described above.

When using NumPy, stick to NumPy functions where possible instead of going via pure Python and back again (which is usually slower). So instead of getting the unique elements of an array by writing np.array(list(set(...))), call numpy.unique.
The expression np.argwhere(vocs[j]==x[i,j]) has to search the whole vocabulary for the word x[i,j], and this has to be done for every feature in every sample. This means that if there are \$s\$ samples and \$f\$ features, and each feature has \$w = O(s)\$ words, the overall runtime is proportional to \$sfw = O(s^2f)\$. That is, it's quadratic in the number of samples. This is why it takes so long.

To avoid this, we need to construct, for each feature, an inverse mapping from words in the samples to their indexes in the vocabulary. How do we construct such inverse mappings? Well, looking at the numpy.unique documentation, we find that it takes a keyword argument:

  return_inverse : bool, optional

  If True, also return the indices of the unique array (for the specified axis, if provided) that can be used to reconstruct ar.

These inverse arrays are exactly what you need, and so your code becomes:

vocs, inverses = zip(*(np.unique(feature, return_inverse=True) for feature in x.T))
x_new = np.vstack(inverses).T


Creating a bijection between integers and a set of strings I have a set of unique strings, I want to create a unique integer identifier for each string.

Usage I want a function to move back and forth, if I give it an integer it returns the corresponding string and vice versa.

Here is how I am doing it

def str_to_int(S):
    integers = list(range(len(S)))
    my_dict = dict(zip(S,integers))
    rev_dict = dict(zip(integers,S))
    return my_dict, rev_dict

If I need to get the integer identifier of an item of S, I need to call the function and then the appropriate returned dictionary.

I want something simpler, given an integer or a string, it knows, somehow automatically if it's an int or str and return the other identifier (i.e. if I give an int it returns the str identifier and vice versa). Is it possible to do it in a single function ? (if possible without being obliged to recreate dictionaries for each call)

Edit: I thought of doing to functions str_to_int(S:set, string:str)->int and int_to_str(S:set ,integer:int)->str but the problem is 1) that's two functions, 2) each time two dictionaries are created.

Since what you want is something a little more complicated than what a normal dictionary can do, I think you want to encapsulate all of this in a class that behaves the way you want the dict to behave.  You can make it "look like" a dict by implementing __getitem__, something like:

from typing import Dict, List, Set, Union, overload

class StringTable:
    """Associate strings with unique integer IDs."""

    def __init__(self, strings: Set[str]):
        """Initialize the string table with the given set of strings."""
        self._keys: List[str] = []
        self._ids: Dict[str, int] = {}
        for key in strings:
            self._ids[key] = len(self._keys)
            self._keys.append(key)

    @overload
    def __getitem__(self, o: int) -> str: ...

    @overload
    def __getitem__(self, o: str) -> int: ...

    def __getitem__(self, o: Union[int, str]) -> Union[str, int]:
        """Accepts either a string or int and returns its counterpart."""
        if isinstance(o, int):
            return self._keys[o]
        elif isinstance(o, str):
            return self._ids[o]
        else:
            raise TypeError("Bad argument!")

    def __len__(self) -> int:
        return len(self._keys)

Now you can use it like:

strings = {"foo", "bar", "baz"}
bijection = StringTable(strings)
for s in strings:
    print(s, bijection[s])
    assert bijection[bijection[s]] == s

etc.