Overriding properties at runtime but not for serialization I have a use case where I need to deserialize some settings from a JSON file and override their values at runtime but when then configuration is saved, I want to keep the original value.
Imagine a simple JSON like this:

var json = @"{ 'WorkingDirectory': '\\bin' }";

You specify a relative path in the config but at runtime you resolve the path and use it in the entire application: @"c:\temp\bin".
Later when you save the configuration (because there are other saveable setting) you don't want to change this particular relative path but leave it untouched.

I solved this problem by creating a new type that I call Overrideable<T>. It has two properties. Value always returns the original value and gets serialized. Current returns the updated value and can be used at runtime. Current has no public setters. You use Value to actually override it and under the hood it sets the new value to Current.
public class Overrideable<T>
{
    private T _value;

    [JsonConstructor]
    public Overrideable(T value)
    {
        _value = value;
        Value = value;
    }

    public T Value { get => _value; set => Current = value; }

    [JsonIgnore]
    public T Current { get; private set; }

    public static implicit operator T(Overrideable<T> overridable) => overridable.Current;
}

To support this new type a new JsonConverter was necessary. It takes care of deserializing the value and creating an instance of the Overridable<T> and of serializing it by extracting the Value from it.
public class JsonOverridableConverter : JsonConverter
{
    public override bool CanConvert(Type objectType)
    {
        var genericArguments = objectType.GetGenericArguments();
        if (genericArguments.Length == 1)
        {
            var overridable = typeof(Overrideable<>).MakeGenericType(genericArguments[0]);
            return (objectType == overridable);
        }
        return false;
    }

    public override object ReadJson(JsonReader reader, Type objectType, object existingValue, JsonSerializer serializer)
    {
        // .Single is safe here because CanConvert will prevent invalid types.
        var overridableGeneringArgument = objectType.GetGenericArguments().Single();
        var value = serializer.Deserialize(reader, overridableGeneringArgument);
        return Activator.CreateInstance(objectType, new[] { value });
    }

    public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer)
    {
        // .Single is safe here because CanConvert will prevent invalid types.
        var valueType = value.GetType().GetGenericArguments().Single();
        var actualValue = value.GetType().GetProperty(nameof(Overrideable<object>.Value)).GetValue(value);
        serializer.Serialize(writer, actualValue);
    }
}

Example:
Here's some example code in case someone wanted to run it:
void Main()
{
    var json = @"{ 'WorkingDirectory': '\\bin' }";
    var config = JsonConvert.DeserializeObject<Configuration>(json, new JsonSerializerSettings
    {
        Converters = { new JsonOverridableConverter() }
    });

    config.WorkingDirectory.Value = @"c:\temp\bin";

    config.Dump();

    json = JsonConvert.SerializeObject(config, new JsonSerializerSettings
    {
        Converters = { new JsonOverridableConverter() }
    });

    json.Dump();
}

public class Configuration
{
    public Overrideable<string> WorkingDirectory { get; set; }
}

Can you think of a better solution or of any improvements?

As far as the actual code goes, the implementation is clean and I don't really have any comments :)

As for the idea, I don't think it's quite right. Settings should be that - just settings. As soon as you start adding behaviour to them, they're not settings any more and you should look at what you're missing. Taking your example:

public interface IFilePathService
{
    string GetPath(string file);
}

public class LocalFilePathService : IFilePathService
{
    private readonly string resolvedDirectory;

    public LocalFilePathService(
        string baseDirectory,
        string workingDirectory)
    {
        // validation ignored.
        resolvedDirectory = Path.Combine(baseDirectory, workingDirectory);
    }

    public string GetPath(string file)
    {
        // validation ignored.
        return Path.Combine(resolvedDirectory, file);
    }
}

Now you create your service with 2 parts, one is your setting (working directory) and the other is the path to your base directory:

new LocalFilePathService(@"C:\temp", Settings.WorkingDirectory);

You just inject this service into all the places you need to get files in the working directory. You don't have any magic that you need to worry about. If you change the setting and save it, then you expect (and want) it to be saved.


Grabbing resolution and offset from xrandr (using GNU sed)
I want to grab some output from xrandr and leave out everything but the resolution and offset.
Input:
eDP1 connected 1366x768+0+0 (normal left inverted right x axis y axis) 290mm x 160mm
   1366x768      60.02*+
   1280x720      59.74
   ...
   640x360       59.84    59.32    60.00
HDMI1 connected primary 2560x1440+1366+0 (normal left inverted right x axis y axis) 610mm x 350mm
   3840x2160     30.00 +  25.00    24.00    29.97    23.98
   2560x1440     59.95*
   ...
   720x400       70.08

Desired output:
1366x768+0+0
2560x1440+1366+0

This is my current command:
xrandr | sed -e '/^\ /d' -e 's/.*\(\<.*x[0-9]*+[0-9]*+[0-9]*\).*/\1/'

The first expression deletes all the lines that begin with spaces, leaving
eDP1 connected 1366x768+0+0 (normal left inverted right x axis y axis) 290mm x 160mm
HDMI1 connected primary 2560x1440+1366+0 (normal left inverted right x axis y axis) 610mm x 350mm

Then the second expression find the resolution/offset and throws away everything else.
How could I make the regex cleaner?

Cleaner is very much in the eye of the beholder.
Your regex is fine - although for the digits part - I would use \d+ because you expect at least 1 digit.  And \< only works in GNU sed (not on OSX for instance).
Alternative you can look at it differently .. e.g only print the lines you care about, and on those lines drop everything you don't care about).
sed -n -e '/connected/ { s/ (.*$//; s/^.*ected //; s/^primary //; p; }'


Generating random XML documents I'm trying to generate a fresh XML file using Perl XML::Mini::Document. It's working fine, but I don't know whether it's the right way to do it.  Performance is the problem for me; it takes more time when the when record count increases.

Is there any other module better doing than this one in performance and easier way?

#!/usr/bin/perl -w

use warnings;
use XML::Mini::Document;

my $outfile = "D:/Test.xml";

my $nSequence = 1000;
my $sRandom_Name = "";
my $sRandom_Desc = "";
my $newDoc = XML::Mini::Document->new();
my $newDocRoot = $newDoc->getRoot();
my $xmlHeader = $newDocRoot->header('xml');
$xmlHeader->attribute('version', '1.0');
$xmlHeader->attribute('encoding', 'UTF-8');

my $records= $newDocRoot->createChild('records');

for(0..9) {

   for(1..6) {
      $sRandom_Name = $sRandom_Name.(chr(int(rand(25) + 65)));
   }
   for(1..15) {
      $sRandom_Desc = $sRandom_Desc.(chr(int(rand(25) + 97)));
   }

   my $record = $records->createChild('record');

   $record->createChild('ID')->text($nSequence=$nSequence+1);
   $record->createChild('Name')->text($sRandom_Name);
   $record->createChild('Desc')->text($sRandom_Desc);

   print $newDoc->toFile($outfile);
}

My output should look like this one:

<?xml version="1.0" encoding="UTF-8" ?>
   <records>
    <record>
         <ID>1001</ID>
         <Name>ASDSDF</Name>
         <Desc>ASDFsdsdfcwefSC</Desc>
      </record>
      <record>
         <ID>1002</ID>
         <Name>KDFNND</Name>
         <Desc>WEFsdssccwefSC</Desc>
      </record>
      <record>
         <ID>1003</ID>
         <Name>PORJDX</Name>
         <Desc>XceFsdsdfcASmsd</Desc>
      </record>
      .
      .
      .
   </records>

Yes, that's pretty good. It's essential to include use strict at the top of every Perl program you write, and use warnings is preferable to -w on the command line. You should also avoid capital letters in identifiers for lexical variables, as they are reserved for use in global identifiers such as package names

I would write something more Perlish like this

#!/usr/bin/perl

use strict;
use warnings;

use XML::Mini::Document;

use constant OUTFILE => 'D:/Test.xml';

sub rand_letter { chr(ord('A') + rand(26)) }

my $new_doc = XML::Mini::Document->new;
my $root    = $new_doc->getRoot;
my $header  = $root->header('xml');
$header->attribute(version  => '1.0');
$header->attribute(encoding => 'UTF-8');

my $records= $root->createChild('records');

my $sequence = 1000;

for ( 0..9 ) {

    my $record = $records->createChild('record');

    my ($random_name, $random_desc);
    $random_name .= rand_letter for 1 .. 6;
    $random_desc .= rand_letter for 1 .. 15;

    $record->createChild(ID   => ++$sequence);
    $record->createChild(Name => $random_name);
    $record->createChild(Desc => $random_desc);
}

open my $fh, '>', OUTFILE or die $!;
select $fh;
print $new_doc->toString;


Image-resizing methods I took a little heat on a method I had to resize an image and I'm hoping I can get a little help on the best way to do it. This is intended for a simple Java Helper Library I'm developing. I have a few ways to do it.

New Method:

/**
* This method resizes the given image
*
* @param image
* @param width
* @param height
* @param max if true, sets the width and height as maximum heights and widths, if false, they are minimums
* @return
*/
public static BufferedImage resizeImage(BufferedImage image, int width, int height, boolean max) {
  int type = (image.getTransparency() == Transparency.OPAQUE)
          ? BufferedImage.TYPE_INT_RGB : BufferedImage.TYPE_INT_ARGB;
  BufferedImage bi = new BufferedImage(width, height, type);
  Graphics2D graphics = bi.createGraphics();
  graphics.drawImage(image, 0, 0, width, height, null);
  graphics.dispose();
  return bi;
}

Original Method:

/**
* This method resizes the given image
*
* @param width
* @param height
* @param max if true, sets the width and height as maximum heights and widths, if false, they are minimums
* @return
*/
public static ImageIcon resizeImage(ImageIcon imageIcon, int width, int height, boolean max) {
  Image image = imageIcon.getImage();
  Image newimg = image.getScaledInstance(-1, height, java.awt.Image.SCALE_SMOOTH);
  int width1 = newimg.getWidth(null);
  if ((max && width1 > width) || (!max && width1 < width)) {
    newimg = image.getScaledInstance(width, -1, java.awt.Image.SCALE_SMOOTH);
  }
  return new ImageIcon(newimg);
}

Are these kinds of convenience methods ok?

/**
* Convenience method. Creates ImageIcon from the given file location and calls resizeImage with it
*
* @param location
* @param width
* @param height
* @param max
* @return
*/
public static ImageIcon resizeImageFromFile(String location, int width, int height, boolean max) {
  ImageIcon imageIcon = new ImageIcon(location);
  return resizeImage(imageIcon, width, height, max);
}

/**
* Convenience method. Creates ImageIcon from the given resource location and calls resizeImage with it
*
* @param location
* @param width
* @param height
* @param max
* @return
*/
public static ImageIcon resizeImageFromResource(String location, int width, int height, boolean max) {
  ImageIcon imageIcon = new ImageIcon(SwingHelper.class.getResource(location));
  return resizeImage(imageIcon, width, height, max);
}

Edit: about the new method, I'd say Graphics2D is a bit overkill (look at Konrad Rudolph's comment for another opinion). And you don't use the max parameter anymore. Does it stretch the image?

The original method is better, but you should have a real if else: resizing the image twice is a hack. It would look like this: you just have to fill in the if.

public static ImageIcon resizeImage(ImageIcon imageIcon, int width, int height, boolean max) {
  Image image = imageIcon.getImage();
  Image newimg;
  if(/* think what condition would work here */) {
    newimg = image.getScaledInstance(-1, height, java.awt.Image.SCALE_SMOOTH);
  } else {
    newimg = image.getScaledInstance(width, -1, java.awt.Image.SCALE_SMOOTH);
  }
  return new ImageIcon(newimg);
}

I think the helper methods are useless since there is no abstraction: it just does two separate things. This does not help that much, and it's better if opening the file is explicit: the code is easier to read.


Create a triangle arrow block dynamically I wrote a function to create one of those CSS triangle blocks and insert it right after one of the already created blocks.

function createArrow() {
    var arrowDown = {
        parent: document.querySelector('.landingItem'),
        div: document.createElement('div'),
        initDiv: function() {
            this.div.classList.add('arrow-down');
            this.div.style.borderLeftWidth = parseInt(document.documentElement.clientWidth)/2 + 'px';
            this.div.style.borderRightWidth = parseInt(document.documentElement.clientWidth)/2 + 'px';
            this.parent.appendChild(this.div);
        },
        checkDiv: function () {
            if(this.parent.querySelector('.arrow-down')) {
                this.parent.removeChild(this.parent.querySelector('.arrow-down'));
            }
            this.initDiv();
        }

    };

    arrowDown.checkDiv();
}

createArrow();
window.addEventListener('resize', createArrow);
.arrow-down {
  margin-top: -2px;
  border-top: 60px solid #501a70;
  border-left: 160px solid transparent;
  border-right: 160px solid transparent;
}

What can I improve?

Is it an utility function or a module?

You named your function createArrow() and it contains an object with methods and at the end you just call one.

I think this is a bit of over engineered for such piece of code.

The first improvement I suggest you is to remove the plain object and method, and refactor the function to do stright forward the task.

Names are all

Please pay attention to the names you give to the functions/method.

When I use some kind of module or object, it is natural to me to call the initializer as the first thing.

But looking at your code, it would be the wrong way.

You just called a checkDiv().

Does is not just bad because it is unaspected, but aslo because if I look at your code, you didn't check the div!

    checkDiv: function () {
        if(this.parent.querySelector('.arrow-down')) {
            this.parent.removeChild(this.parent.querySelector('.arrow-down'));
        }
        this.initDiv();
    }

Here you search an element in the portion of the DOM tree of the this.parent, and if you find, you remove!

A check should be a safe action, not something that remove things.

I suggest you to call such method: reset()

This is more near to the task, and tell to other developer that if they call it they are going to loose what they have now.

Another thing is to remove from the method the last line:

 this.initDiv();

You should call reset() at the beginning of the init(), not the init() at the end of the reset().

And yes, about names: you don't need the Div part in the name in this case.

So, I would refactor your code as follow:

function createDownArrow() {
    const parent = document.querySelector('.landingItem');

    var arrow = parent.querySelector('.arrow-down');
    if (arrow) {
        parent.removeChild(arrow);
    }

    const element = document.createElement('div');

    element.classList.add('arrow-down');
    const borderWidth = parseInt(document.documentElement.clientWidth)/2;

    element.style.borderLeftWidth = borderWidth + 'px';
    element.style.borderRightWith = borderWidth + 'px';

    parent.appendChild(element);
}

window.addEventListener("load", createDownArrow);
window.addEventListener("reset", createDownArrow);
.arrow-down {
  margin-top: -2px;
  border-top: 60px solid #501a70;
  border-left: 160px solid transparent;
  border-right: 160px solid transparent;
}


Control of empty fields in a form via JavaScript I would like to know if it is safe enough to create via JavaScript control of empty fields. Here is my example in detail:

    <script language='javascript'>
        function validate () {
            if(document.getElementById('title').value=="") {
                alert("Enter a Title!");
                return false;
            }
            if(document.getElementById('description').value=="") {
                alert("Enter a Description!");
                return false;
            }
            if(document.getElementById('image').value=="") {
                alert("Inser an Image!");
                return false;
            }
    alert("Success");
    return true;
}
</script>

It's okay to do the validations in the client side for a better user experience, so the user does not have to wait the response of the server or make a request to know if there is any field that he left incomplete. But you have to do the same validations on the server, because if someone makes a direct request to your server with empty fields, that request is not going through the javascript validations, so your application is going to crash.


Longest common subsequence length and backtracking the string Problem: Given two sequences, print the longest subsequence present in both of them. A subsequence is a sequence that appears in the same relative order, but not necessarily contiguous. For example, “abc”, “abg”, “bdf”, “aeg”, ‘”acefg”, .. etc are subsequences of “abcdefg”. So a string of length n has 2^n different possible subsequences.

Examples:
LCS for input Sequences “ABCDGH” and “AEDFHR” is “ADH” of length 3.
LCS for input Sequences “AGGTAB” and “GXTXAYB” is “GTAB” of length 4.

Python 3 code:

def calculate_lcs_length(a,b):
    a_len = len(a)
    b_len = len(b)
    dp = []
    for i in range(a_len + 1):
        dp.append([0 for j in range(b_len + 1)])
    for i in range(1, a_len + 1):
        for j in range(1, b_len + 1):
            if a[i - 1] == b[j - 1]:
                dp[i][j] = dp[i - 1][j - 1] + 1
            else:
                dp[i][j] = max(dp[i-1][j], dp[i][j - 1])
    max_length = dp[a_len][b_len]
    return dp, max_length

def get_path(a, b, dp, i, j):
    if i == 0 or j == 0:
        return ""
    if a[i-1] == b[j-1]:
        return get_path(a, b, dp, i-1, j-1) + a[i-1]
    else:
        if dp[i-1][j] > dp[i][j-1]:
            return get_path(a, b, dp, i-1, j)
        else:
            return get_path(a, b, dp, i, j-1)

if __name__ == "__main__":
    a = "ABCDGH"
    b = "AEDFHR"
    dp, max_length = calculate_lcs_length(a,b)
    lcs_str = get_path(a, b, dp, len(a), len(b))
    print(lcs_str)

Output: ADH

I wonder if I could use one single method (without using recursion) to get the length and string both.

Is this code can be more reader friendly. I am not asking for one liner or complex optimization improve.

Reference: Longest common subsequence problem, From Wikipedia

Yes, you can easily convert the get_path function to an iterative version.

def get_path(a, b, dp, i, j):
    seq = ""
    while(i != 0 and j != 0):
        if a[i-1] == b[j-1]:
            i-=1
            j-=1
            seq += a[i]
        else:
            if dp[i-1][j] > dp[i][j-1]:
                i-=1
            else:
                j-=1
    return seq[::-1]

And now you can merge this function with calculate_lcs_length into one if you want.

I guess you have read this, but I just want to remind you that all described optimizations are still applicable to your code.


Read through a log file to find "Fail" lines Please bear with me, I'm only about a week into Python.

This code is something that I wanted to do once I completed the chapters dealing with opening and reading files, and RegEx. I always feel like I'm over complicating the code. Please take a look and let me know if this is sound and where it can be improved. There are a lot of concepts I haven't learned yet. I'm teaching myself out of a book.

This code is meant to open a log file (will eventually be retrieved real-time), go through it line by line using RegEx to find "Fail", add those lines reporting failures to a list and return the completed list as a string joined with new lines.

#!/usr/bin/python

import os
import re

myFile = '/home/nick/python/chap8ReadingWritingFiles/15Jan2016_000000.txt'

def RegEx(file):
    openFile = open(myFile, 'r')
    myResList = []
    myList = openFile.readlines()
    myReg = re.compile(r'Fail')
    for i in (myList):
            #print(i, end='')
            result = myReg.search(i)
            if result != None:
                    #print(i, end='')
                    myResList.append(i.rstrip())
    final = '\n'.join(myResList[:])
    return final

print()
print('Results:')
print(RegEx(myFile))
print()

The standard indentation for Python, specified by PEP 8, is four spaces.  This is a pretty strong convention for Python, since indentation matters a lot.

You almost always want to call open() in the context of a with-block, so that the file will get closed automatically when exiting the block.

This task does not require you to read the entire file at once.  I recommend working line by line so that you can handle large files better.  (You also don't need to make a copy of myResList at the end using myResList[:].)

You don't need a regular expression to search for a fixed literal string.  A substring search will do just fine.  This also means that your function is misnamed.  In general, you should name functions after the task that they accomplish, rather than how they accomplish the task.

I'd generalize your function to search for any text.

def search(filename, text):
    with open(filename) as f:
        for line in f:
            if text in line:
                print(line)

if __name__ == '__main__':
    search('/home/nick/python/chap8ReadingWritingFiles/15Jan2016_000000.txt', 'Fail')

If you're interested more in passing back the results, a better way would be to yield the results back to the caller.

def search(filename, text):
    with open(filename) as f:
        for line in f:
            if text in line:
                yield line

if __name__ == '__main__':
    for result in search('/home/nick/python/chap8ReadingWritingFiles/15Jan2016_000000.txt', 'Fail'):
        print(result)

The if __name__ == '__main__': boilerplate is optional, but considered standard practice.


Generic dynamic array implementation in C I'm working on my own generic-type dynamic array in C99 and I would like to hear your input, suggestions, ideas, anything!

What I most care about is: 1) correctness and 2) performance.

So far, I've tested both with standard malloc and mimalloc, although with the standard implementation it seems to be running 50% faster.

I've also ran benchmarks against libsrt, vec and Nim's implementation of dynamic sequences and my code seems to be beating them all.

Also, of major importance is to note that this code is not meant to be used as a library, but as part of a bytecode VM/intepreter that I've been working on - which means I am the one who is going to use it.

Here's the source:

#ifndef __VARRAY_H__
#define __VARRAY_H__

/**************************************
  Type definitions
 **************************************/

#define Array(TYPE) \
    struct { \
        size_t size; \
        size_t cap; \
        size_t typeSize; \
        TYPE *data; \
    }

/**************************************
  Macros
 **************************************/

#define aInit(DEST,TYPE,CAP) { \
    int k=0;                                                                \
    while (powerOfTwo[++k]<CAP);                                            \
                                                                            \
    DEST = malloc(sizeof(Array(TYPE)));                                     \
    DEST->size = 0;                                                         \
    DEST->typeSize = sizeof(TYPE);                                          \
    DEST->cap = powerOfTwo[k];                                              \
    DEST->data = calloc(DEST->cap, DEST->typeSize);                         \
}

#define aNew(NAME,TYPE,CAP) \
    Array(TYPE)* NAME;                                                      \
    aInit(NAME,TYPE,CAP);

#define aResize(DEST) { \
    DEST->cap *= 2;                                                         \
    DEST->data = realloc(DEST->data, DEST->cap * DEST->typeSize);           \
}

#define aAdd(DEST,X) \
    if (++(DEST->size) >= DEST->cap) aResize(DEST);                         \
    DEST->data[DEST->size-1] = X

#define aAppend(DEST,X) \
    DEST->cap += X->cap;                                                    \
    DEST->data = realloc(DEST->data, DEST->cap * DEST->typeSize);           \
    memcpy(DEST->data + DEST->size, X->data, X->size * X->typeSize);        \
    DEST->size += X->size

#define aEach(DEST,INDEX) \
    for (int INDEX=0; INDEX<DEST->size; INDEX++)

#define aFree(DEST) \
    free(DEST->data);                                                       \
    free(DEST)

#endif

The powerOfTwo lookup constant:

/**************************************
  Constants
 **************************************/

static size_t powerOfTwo[] = {
    0,
    1, 2, 4, 8, 16, 32, 64, 128,
    256, 512, 1024, 2048, 4096, 8192, 16384, 32768,
    65536, 131072, 262144, 524288, 1048576, 2097152, 4194304, 8388608,
    16777216, 33554432, 67108864, 134217728, 268435456, 536870912, 1073741824, 2147483648,
    4294967296, 8589934592, 17179869184, 34359738368, 68719476736, 137438953472, 274877906944, 549755813888
};

And a little usage example:

Array(int)* arr;
aInit(arr,int,0);
aAdd(arr,0);
aAdd(arr,1);

aEach(fastArr,i) {
    printf("item @ %d = %d\n",i,arr->data[i]);
}

aFree(arr);

See
this Stackoverflow answer for
a discussion on the optimal growth factor for dynamic
arrays. The gist of it is that it depends, but growth
factors around the golden ratio are easier for the memory allocator to
handle. I'd recommend using 1.5 because it is easy to implement:

DEST->cap += DEST->cap / 2

It likely fares worse on artifical benchmarks but better on real
workloads. This requires that the minimum capacity of the dynamic
array is 2 which is reasonable. But a more realistic minimum size
likely should be 8 or 16 since
the minimum malloc size is 32
bytes.

In aAppend you are always realloc:ing which will cause performance
to suffer if many small dynamic arrays are collected into one big one
in a loop. Instead, you want something like:

size_t req = DEST->size + X->size;
if (req > DEST->cap) {
    aResize(DEST);
}
memcpy(DEST->data + DEST->size, X->data, X->size * X->typeSize);
DEST->size = req;

The last thing I'd change is the alignment of the capacity to a power
of two in aInit. On realistic workloads, a good chunk of all
variable arrays never grow so the alignment just wastes space. But
if you insist on alignment, this function is better than the loop:

static inline int
next_pow2(int v) {
    v--;
    v |= v >> 1;
    v |= v >> 2;
    v |= v >> 4;
    v |= v >> 8;
    v |= v >> 16;
    v++;
    return v;
}

I also wonder why you are using macros when C99 has inline functions?
They have many advantages in comparison to code macros.


Convert data frame to list of lists by row I am rewriting some old code where I take a dataframe in r and convert it using the tidyverse packages to a list of lists, where each element is one row of the original dataframe - which is itself a list with each column an element.

My previous function achieved it like so:

library(tidyverse)

feat <- function(coords, height = 20, vjust = 75,  fill = "orange"){
  inst <- purrr::flatten(purrr::by_row(coords, ..f = function(x)
    rec <- list(type = "rect",
                x = as.numeric(x[1]),
                y = vjust,
                width = as.numeric(x[2]-x[1]),
                height = height,
                fill = fill),
    .labels = FALSE))
  return(inst)
}

However by_row() has been depreciated from the purrr package and so I would like to rewrite it. This is my attempt:

feat2 <- function(coords, height = 20, vjust = 75,  fill = "orange"){
  inst <- coords %>%
    mutate(type = "rect",
           x = as.numeric(start),
           y = vjust,
           width = as.numeric(end-start),
           height = height,
           fill = fill) %>%
    select(-start, -end) %>%
    mutate(count = 1:n()) %>%
    nest(-count) %>%
    select(-count) %>%
    mutate(data = map(data, ~ flatten(.x))) %>% pull()
  return(inst)
}

which does the job but I feel there should be a quicker, more elegant way to achieve this. Do you have any ideas on that aspect?

Here is an example data set:

coords <- structure(list(start = c(126, 433, 603, 1604), end = c(327, 495,
644, 1831)), .Names = c("start", "end"), row.names = c(NA, -4L
), class = "data.frame")

and the desired output:

result <- list(structure(list(type = "rect", x = 126, y = 75, width = 201,
    height = 20, fill = "blue"), .Names = c("type", "x", "y",
"width", "height", "fill")), structure(list(type = "rect", x = 433,
    y = 75, width = 62, height = 20, fill = "blue"), .Names = c("type",
"x", "y", "width", "height", "fill")), structure(list(type = "rect",
    x = 603, y = 75, width = 41, height = 20, fill = "blue"), .Names = c("type",
"x", "y", "width", "height", "fill")), structure(list(type = "rect",
    x = 1604, y = 75, width = 227, height = 20, fill = "blue"), .Names = c("type",
"x", "y", "width", "height", "fill")))

without any external packages:

feat3 <- function(coords, height = 20, vjust = 75,  fill = "blue"){
  xx <- apply(coords, 1, function(x) {
    list(type = "rect",
         x = as.numeric(x[1]),
         y = vjust,
         width = as.numeric(x[2] - x[1]),
         height = height,
         fill = fill)
    }
  )
  return(xx)
}

all.equal(feat3(coords), result)
# [1] TRUE

Benchmarks:

microbenchmark::microbenchmark(
  feat3(coords),
  feat2(coords), unit = "relative"
)
# Unit: relative
# expr      min       lq     mean   median       uq      max neval cld
# feat3(coords)   1.0000   1.0000   1.0000   1.0000   1.0000   1.0000   100  a
# feat2(coords) 315.8261 297.2919 254.5025 258.7822 248.2652 286.9575   100   b


Console-based TicTacToe game in Python I created a console-based TicTacToe game in Python. So far, everything works as expected. I decided to keep the class static, because I figured there would be no use for multiple game states at once. Is there anything I can improve on?
class TicTacToe:
    """Plays a 2-player TicTacToe Game."""

    __curr_player = False  # simple bool player switch
    __game_matrix = [  # representation of game state
        [0, 0, 0],
        [0, 0, 0],
        [0, 0, 0],
    ]

    @classmethod
    def play(cls):
        """Play a single round."""
        while not cls.__game_finished():

            # set current player
            player = 2 if cls.__curr_player else 1

            # get player move
            while 1:
                try:
                    print(f"Player {player}'s turn.")
                    x = int(input("X: "))
                    y = int(input("Y: "))
                    if cls.__is_valid_field(x, y):
                        break
                    else:
                        print("Invalid field.")
                except ValueError:
                    print("Invalid Input.")

            cls.__update(x, y, player)
            print("Current game state: ")
            cls.__display()

            # change player
            cls.__curr_player = not cls.__curr_player

        print("Game finished: ")
        cls.__display()

        # reset state
        cls.__reset()

    @classmethod
    def __game_finished(cls):
        """Returns True if one player has won or all fields are filled."""
        # all filled?
        if all([value for line in cls.__game_matrix for value in line]):
            return True
        # horizontal win?
        if any(all([x == line[0] and x for x in line]) for line in cls.__game_matrix):
            return True
        # vertical win?
        if any(all([line[column] == cls.__game_matrix[0][column] and line[column] for line in cls.__game_matrix]) for column in range(3)):
            return True
        # diagonal win?
        if (cls.__game_matrix[0][0] == cls.__game_matrix[1][1] == cls.__game_matrix[2][2] != 0) or \
           (cls.__game_matrix[0][2] == cls.__game_matrix[1][1] == cls.__game_matrix[2][0] != 0):
            return True
        return False

    @classmethod
    def __is_valid_field(cls, x, y):
        """Checks if field is already filled or out of range."""
        if x > 3 or x < 1 or y > 3 or y < 1:
            return False
        return not cls.__game_matrix[y - 1][x - 1]

    @classmethod
    def __update(cls, x, y, player):
        """Update game state."""
        cls.__game_matrix[y - 1][x - 1] = player

    @classmethod
    def __display(cls):
        """Displays current game state."""
        for line in cls.__game_matrix:
            print(line)

    @classmethod
    def __reset(cls):
        """Reset game state."""
        cls.__curr_player = False
        cls.__game_matrix = [
            [0, 0, 0],
            [0, 0, 0],
            [0, 0, 0],
        ]

def main():
    TicTacToe.play()

if __name__ == "__main__":
    main()

When i started the game, i found it pretty confusing as to what what coordinates should we input. I added an instructions method:

    @classmethod
    def __instructions(cls):
        print(
            'Welcome to TicTacToe\n'
            'Here is the board:\n'
            )
        cls.__display()
        print(
            'The coordinates start at top left\n'
            '(1, 1) (2, 1) (3, 1)\n'
            '(1, 2) (2, 2) (3, 2)\n'
            '(1, 3) (2, 3)  (3, 3)\n'
            )

I found having 0, 1, 2 on the board is pretty confusing. I replaced empty cells with a symbol, in this case -

    @classmethod
    def __display(cls):
        """Displays current game state."""
        for line in cls.__game_matrix:
            symbols = []
            for elem in line:
                if elem == 0:
                    symbols.append('-')
                else:
                    symbols.append(str(elem))
            print(' '.join(symbols))

I rearraged private methods before

I added a win annoncement to know who won:

print("Game finished: Player {} won!".format(player))

If you used a normal class you would not have needed

@classmethod
def __game_finished(cls):

but would have used the normal
def __game_finished(self):

less writing

Here is my code:
class TicTacToe:
    """Plays a 2-player TicTacToe Game."""

    __curr_player = False  # simple bool player switch
    __game_matrix = [  # representation of game state
        [0, 0, 0],
        [0, 0, 0],
        [0, 0, 0],
    ]

    @classmethod
    def __game_finished(cls):
        """Returns True if one player has won or all fields are filled."""
        # all filled?
        if all([value for line in cls.__game_matrix for value in line]):
            return True
        # horizontal win?
        if any(all([x == line[0] and x for x in line]) for line in cls.__game_matrix):
            return True
        # vertical win?
        if any(all([line[column] == cls.__game_matrix[0][column] and line[column] for line in cls.__game_matrix]) for column in range(3)):
            return True
        # diagonal win?
        if (cls.__game_matrix[0][0] == cls.__game_matrix[1][1] == cls.__game_matrix[2][2] != 0) or \
           (cls.__game_matrix[0][2] == cls.__game_matrix[1][1] == cls.__game_matrix[2][0] != 0):
            return True
        return False

    @classmethod
    def __is_valid_field(cls, x, y):
        """Checks if field is already filled or out of range."""
        if x > 3 or x < 1 or y > 3 or y < 1:
            return False
        return not cls.__game_matrix[y - 1][x - 1]

    @classmethod
    def __update(cls, x, y, player):
        """Update game state."""
        cls.__game_matrix[y - 1][x - 1] = player

    @classmethod
    def __display(cls):
        """Displays current game state."""
        for line in cls.__game_matrix:
            symbols = []
            for elem in line:
                if elem == 0:
                    symbols.append('-')
                else:
                    symbols.append(str(elem))
            print(' '.join(symbols))

    @classmethod
    def __reset(cls):
        """Reset game state."""
        cls.__curr_player = False
        cls.__game_matrix = [
            ['-', '-', '-'],
            ['-', '-', '-'],
            ['-', '-', '-'],
        ]

    @classmethod
    def __instructions(cls):
        print(
            'Welcome to TicTacToe\n'
            'Here is the board:\n'
            )
        cls.__display()
        print(
            'The coordinates start at top left\n'
            '(1, 1) (2, 1) (3, 1)\n'
            '(1, 2) (2, 2) (3, 2)\n'
            '(1, 3) (2, 3)  (3, 3)\n'
            )

    @classmethod
    def play(cls):
        cls.__instructions()
        """Play a single round."""
        while not cls.__game_finished():

            # set current player
            player = 2 if cls.__curr_player else 1

            # get player move
            while 1:
                try:
                    print(f"Player {player}'s turn.")
                    x = int(input("X: "))
                    y = int(input("Y: "))
                    if cls.__is_valid_field(x, y):
                        break
                    else:
                        print("Invalid field.")
                except ValueError:
                    print("Invalid Input.")

            cls.__update(x, y, player)
            print("Current game state: ")
            cls.__display()

            # change player
            cls.__curr_player = not cls.__curr_player

        print("Game finished: Player {} won!".format(player))
        cls.__display()

        # reset state
        cls.__reset()

def main():
    TicTacToe.play()

if __name__ == "__main__":
    main()

The code was well commented, broken down, easy to follow and clean!


Assembling an Interrupt Handler - INTERRUPT HANDLER -

> READ FROM IN.1 THROUGH IN.4
> WRITE THE INPUT NUMBER WHEN
  THE VALUE GOES FROM 0 TO 1
> TWO INTERRUPTS WILL NEVER CHANGE
  IN THE SAME INPUT CYCLE

I was stuck on this one when I went to bed last night.  But of course, like any good programmer, sleeping is when I get most of my work done.

My primary goal here is fewer cycle counts.  If the above... erm, graph... is anything to go by, it looks like I could have quite a bit less cycle counts.  Secondary would be reducing the instruction count (but not at the expense of cycle counts).  I can imagine how to get it down to 7 nodes rather than 9, but I can't imagine that also makes the program more efficient (in terms of cycle counts).

So, how can I reduce the cycle counts here?

ROW 1, COLUMN 1 (IN.1)

MOV UP, ACC
START:
 MOV 0, DOWN
 JEZ CHECK
 JNZ CONTINUE
CHECK:
 MOV UP, ACC
 JEZ START
 MOV 1, DOWN
CONTINUE:
 MOV UP, ACC
 JMP START

ROW 1, COLUMN 2 (IN.2)

MOV UP, ACC
START:
 MOV 0, DOWN
 JEZ CHECK
 JNZ CONTINUE
CHECK:
 MOV UP, ACC
 JEZ START
 MOV 2, DOWN
CONTINUE:
 MOV UP, ACC
 JMP START

ROW 1, COLUMN 3 (IN.3)

MOV UP, ACC
START:
 MOV 0, DOWN
 JEZ CHECK
 JNZ CONTINUE
CHECK:
 MOV UP, ACC
 JEZ START
 MOV 3, DOWN
CONTINUE:
 MOV UP, ACC
 JMP START

ROW 1, COLUMN 4 (IN.4)

MOV UP, ACC
START:
 MOV 0, DOWN
 JEZ CHECK
 JNZ CONTINUE
CHECK:
 MOV UP, ACC
 JEZ START
 MOV 4, DOWN
CONTINUE:
 MOV UP, ACC
 JMP START

ROW 2, COLUMN 1

MOV UP, RIGHT

ROW 2, COLUMN 2

ADD LEFT
ADD UP
MOV ACC, RIGHT
MOV 0, ACC

ROW 2, COLUMN 3

ADD LEFT
ADD UP
ADD RIGHT
MOV ACC, DOWN
MOV 0, ACC

ROW 2, COLUMN 4

MOV UP, LEFT

ROW 3, COLUMN 3 (OUT)

MOV ANY, DOWN

You can swap JEZ and JNZ and then remove the noop jump to check, and because the instruction before start is the same as before the JMP to it at the end you can remove the two instructions:

MOV UP, ACC
START:
 MOV 0, DOWN
 JNZ CONTINUE
 MOV UP, ACC
 JEZ START
 MOV 1, DOWN
CONTINUE:

If you don't mind using more nodes you can simplify Node 2,3 by letting it ADD UP ADD RIGHT and MOV ACC, DOWN (and MOV 0, ACC)

Then Node 3,2 MOVs UP, RIGHT and node 3,3 is a copy of node 2,2. This removes 2,3 as a bottleneck that needs to gather 3 values and instead will let the other nodes help gather them.


Port Availability Scanner, written in Java (9) For purpose of practice and experience, I was working on a "Port Availability Scanner" written in Java. So far it's working but it's really slow. Especially on relatively large quantities of ports (1000+).

It's quite faster scanning localhost then my external IP (If it actually scans externally correctly).

As an additional note for if it really matters:
I am using Java 9.

Code:

package test;

import java.io.IOException;
import java.net.Socket;
import java.util.Arrays;

public class Test {

    private static final Test instance = new Test("157.97.56.42", 80, 443, 21, 22, 25, 8080);
    private static String ip;
    private int[] ports = new int[1_000_000];

    private Test(String ip, int start, int end) {
        Test.ip = ip;

        ports: {
            int c = 0;

            for (int i = start; i < end; i++) {
                ports[c++] = i;
            }

            ports = Arrays.copyOfRange(ports, 0, c + 1);
        }
    }

    private Test(String ip, int... ports) {
        Test.ip = ip;
        this.ports = ports;
    }

    public static void main(String[] args) {
        long start = System.currentTimeMillis();
        Thread thread = new Thread(instance::check);
        thread.start();
        long end = System.currentTimeMillis();

        System.out.println("Took: " + (end - start) + "ms.");
    }

    public void check() {
        if(ports.length > 1) {
            for (int port : ports) {
                System.out.println("Available " + port + ": " + Test.isAvailable(port));
            }
        } else if(ports.length == 1) {
            System.out.println("Available " + ports[0] + ": " + Test.isAvailable(ports[0]));
        } else {
            throw new IllegalStateException();
        }
    }

    private static boolean isAvailable(int port) {
        Socket socket = null;

        try {
            socket = new Socket(ip, port);
            return false;
        } catch (IOException e) {
            return true;
        } finally {
            if(socket != null) {
                try {
                    socket.close();
                } catch (IOException ignored) { }
            }
        }
    }
}

Please can you review my code, especially performance-wise?

Edit:
Would it be possible by using NIO or any other form of MultiThreading the sake of speed?

Why do you create a million element array? (int[] ports = new int[1_000_000]) It'd better to make it as large as the ports in constructor (end-start or ports.length)
Why all of this code?

if(ports.length > 1) {
    for (int port : ports) {
        System.out.println("Available " + port + ": " + Test.isAvailable(port));
    }
} else if(ports.length == 1) {
    System.out.println("Available " + ports[0] + ": " + Test.isAvailable(ports[0]));
} else {
    throw new IllegalStateException();
}

You only really need

for (int port : ports) {
    System.out.println("Available " + port + ": " + Test.isAvailable(port));
}

The result will be equivalent and the code is now more readable
Why did you make ip and instance static? Do you want to make the class a singleton?
The ports label is unnecessary in the first constructor
Use InetAddress instead of String for IPs' as from what I know Sockets convert the strings into InetAdresses anyway. It should somewhat improve performance (change ip variable from String to InetAddress so you don't recreate the address on and on again)
Shouldn't this:

try {
    socket = new Socket(ip, port);
    return false;
} catch (IOException e) {
    return true;
} finally {
    if(socket != null) {
        try {
            socket.close();
        } catch (IOException ignored) { }
    }
}

work the other way round? Why method isAvailable returns true when the
socket throws a exception? Looks like a bug to me I misunderstood what you
meant by available. Your code is correct


Stack implementation with exceptions I would like to know if this is a great stack implementation. I created my own exception. Was that a bad idea or unnecessary? Should I have just thrown a IndexOutOfBounds exception instead of my own.

import java.util.*;
public class Stack2{
    private int[] stack;
    private int size;

    public Stack2(){
        stack = new int[10];
        size = 0;
    }

    public Stack2(int height){
        if(height <= 0){
            throw new EmptyStackException();
        }
        stack = new int[height];
        size = 0;
    }

    public void add(int value){
        if (size == stack.length){
            throw new StackOverflowException();
        }
        stack[size] = value;
        size++;
    }

    public int pop(){
        if(size == 0){
            throw new EmptyStackException();
        }
        size--;
        return stack[size + 1];
    }

    public int peek(){
        if(size == 0){
            throw new EmptyStackException();
        }
        return stack[size - 1];
    }

    public boolean isEmpty(){
        if(size == 0){
            return true;
        }
        return false;
    }

    public int getSize(){
        return this.size;
    }

    class StackOverflowException extends RuntimeException{
        public StackOverflowException(){
            super("Nothing can be added to the stack. The stack was full and has overflowed");
        }

        public StackOverflowException(String message){
            super(message);
        }
    }
}

Your stack returns 0 instead of 3 on this simple case:

public static void main(String[] args) {
    Stack2 s = new Stack2();
    s.add(3);
    System.out.println(s.pop());
}

Traditionally, the stack operations are called "push" and "pop".  It's unusual to have "add" and "pop".

The isEmpty() method would be better written as:

public boolean isEmpty(){
    return size == 0
}

Throwing ArrayIndexOutOfBoundsException would be worse, since it would result in a "leaky" abstraction.  The user shouldn't know that the stack is actually backed by an array.


Using Support Vector Machine algorithm to analyze satellite image I am using Support Vector Machine (SVM) algorithm to perform a classification. The satellite image I am using is really big (5GB) that's why I am trying to take advantage of multiprocessing tool to speed up the process.

My problem is that my PC does not use all the available cores. I run the code to my laptop (4 cores) but it takes for ever for the process to finish. It uses all the 4 CPU cores though. When I try to run the same code on the desktop PC which has 12 CPU cores, only 5 of them reaches 100%.
below, we see the image to be classified and the training data that are used on the right

import os
import numpy as np
from osgeo import gdal, gdal_array, gdalconst
from osgeo import ogr
import pandas as pd
import image_slicer

from sklearn.svm import SVC
from sklearn.preprocessing import MinMaxScaler
from sklearn.model_selection import train_test_split
from sklearn.metrics import confusion_matrix
from sklearn.metrics import classification_report
from sklearn.metrics import accuracy_score
from sklearn.model_selection import cross_val_score, StratifiedKFold, StratifiedShuffleSplit
from sklearn.model_selection import validation_curve, GridSearchCV
from sklearn.pipeline import Pipeline

from matplotlib.pylab import *
from multiprocessing import Pool
import time

img = 'sea_ice.tif' #image to be used for classification, 3000 x 3000 pixels
roi = 'training_data.shp' #training data

X = img[roi_int > 0, 2:] #X is the matrix containing our features
y = roi[roi>0] #y contains the values of our training data

#Split our dataset into training and testing. Test data will be used to make predictions
split_test_data = 0.30
X_train, X_test, y_train, y_test = train_test_split(X, y, test_size=split_test_data, random_state=42)
stratified = StratifiedShuffleSplit(len(y_train), 10, split_test_data, random_state=0)

#use pipeline method to do all the steps automatically
pip = Pipeline([ ('scale', MinMaxScaler()),
             ('svm', SVC())])
pip.fit(X_train, y_train)

def predict(input_data):
    img_predict = pip.predict(input_data)
    return img_predict

start = time.time()

tfs_shape = (img.shape[0] * img.shape[1], img.shape[2]-2 )
tfs_2D = img[:, :, 2:].reshape(tfs_shape)

# split good data into chunks for parallel processing
tfsChunks = np.copy(tfs_2D)
split = np.array_split(tfsChunks, 9)

# run parallel processing of all data with SVM
pool = Pool(9)
svmLablesGood = pool.map(predict, split)

# join results back from the queue and insert into full matrix
svmLabelsGood = np.hstack(svmLablesGood)

# reshape labels from vector into 2D raster map
svm_reshape = svmLabelsGood.reshape(img.shape[0], img.shape[1])
plt.imshow(svm_reshape)
plt.show()

end = time.time()
print 'the processing time is:', end - start, '\n'

#Evaluate the model using K-fold cross-validation
scores_svm = cross_val_score(pip, X_test, y_test, scoring='accuracy', cv=stratified)
print ('Accuracy: %0.2f (+/- %0.2f)' % (scores_svm.mean(), scores_svm.std())), '\n'
print ('10 parts cross-validation:' , scores_svm), '\n'

score_train = pip.score(X_train,y_train)
score_test = pip.score(X_test,y_test)
print 'training score:', score_train, '\n'
print 'testing score:', score_test, '\n'

below, the results from the classification are shown (after 3 days of processing). I need to classify areas with ice and ice free areas. Yellow areas show the ice.

I cannot wait for 3 days for my laptop to finish the process. I want to take advantage of the 12 CPU cores of the desktop PC to speed up the process, but I do not understand why only 5 cores are used.

When you want to parallelize a program you have to actually do it. When you're using C, you can hope that the compiler will spread the CPU "tension" to other CPU, which it will do with the right compiler command. But you're using Python so it cannot do the optimization since there is no (separate) compiler.

But even with C it doesn't work that well. Normally you have to do it yourself with threads. If you can make your calculation on threads you should do it. I didn't read your code in details, but when it comes to SVN I guess that the calculation doesn't depend on previous calculation. If that's the case you might be able to parallelize your program with as many threads as you have cores.

But I think that you have to do that step of parallelizing by yourself, you can't count on the machine to guess where it can or cannot parallelize.

And that will possibly reduce the execution time by the number of threads. (even if that not exactly true, that's a max).


Basic CRUD in Laravel 5.2.37 Controller class

namespace App\Http\Controllers\Website\SportsType;
use App\Classes\Contract\SportsType\ISportsType;
use \App\Http\Requests\SportsType\SportsTypeRequest as SportsTypeRequest;
class SportsTypeController extends \App\Http\Controllers\BaseController
{
    private $AllSportsTypes = "SportsTypes";
    private $sportstype;

    public function __construct(ISportsType $_sportstype) {
        $this->sportstype = $_sportstype;
        parent::__construct();
    }

    public function index() {
        $SportsTypes = $this->sportstype->All();
        return view('SportsType.List')->with('SportsTypes', $SportsTypes);
    }

    public function create() {
        return view('SportsType.Create');
    }

    public function edit($SportsTypeID) {

        $SportsType = $this->sportstype->Get($SportsTypeID);
        return view('SportsType.Edit', array('SportsType' => $SportsType));
    }

    public function save(SportsTypeRequest $request) {
        $data = [
            'SportsType'    =>  $request['SportsType'],
            'SportsTypeID'  =>  $request['SportsTypeID'],
        ];
        $result = $this->sportstype->Save($data);
        return redirect()->route($this->AllSportsTypes);
    }
}

Business logic class

namespace App\Classes\BusinessLogic\SportsType;
use App\Classes\DatabaseLayer\SportsType\SportsTypeDb;
use App\Classes\Contract\SportsType\ISportsType;

class SportsTypeBL implements ISportsType {

    public function All() {
        $SportsTypes = (new SportsTypeDb())->All();
        return $SportsTypes;
    }

    public function Get($SportsTypeID) {
        $SportsType = (new SportsTypeDb())->Get($SportsTypeID);
        if($SportsType == null) {
            \App::abort(404);
            return;
        }
        return $SportsType;
    }

    public function Save($data) {
        return (new SportsTypeDb())->Save($data);
    }
}

Database class

namespace App\Classes\DatabaseLayer\SportsType;

class SportsTypeDb {

    public function All() {
        $SportsTypes = \App\Models\SportsType\SportsTypeModel::all();
        return $SportsTypes;
    }

    public function Get($SportsTypeID) {
        $SportsType = \App\Models\SportsType\SportsTypeModel
                ::where('SportsTypeID', $SportsTypeID)
                ->first();
        return $SportsType;
    }

    public function Save($data) {
        if($data["SportsTypeID"] == 0) {
            $SportsType = new \App\Models\SportsType\SportsTypeModel();
        }
        else {
            $SportsType = $this->Get($data["SportsTypeID"]);
        }

        $SportsType->SportsType     = $data["SportsType"];
        $SportsType->save();
        return true;
    }
}

Which class should have code for caching?
Am I writing bad or very bad code? Can you please suggest good ways to improve it?

Which class should have code for caching?

I recommend a separate Cache class that handles storing and retrieving from the cache.  The argument for a separate class is simple: you want each class to have a narrow purpose. If you choose a particular cache driver/server today, but change your mind in a year, you should be able to change just one class -- the one that interacts with the driver -- to update your code. Start with this basic framework for a Cache class:

//reference to the only allowed instance of this class. It's private so
//outside scripts and objects cannot see it
private static $instance=null;

//private constructor will prevent outside code from instantiating objects.
private function __construct(){/*connect to the cache driver*/}
private function __destruct(){/*disconnect from cache driver*/}

//only way to get an instance (since constructor is private). We return
//the previously created instance instead of making a new one. As a result
//there is at most one instance of the class in memory.  This is a singleton.
public static function getInstance(){
    if(!self::$instance) self::$instance = new Cache();
    return self::$instance;
}

//takes key, returns cached object, or FALSE if not found
public function get(string $cache_key){}

//saves $value to the cache under $cache_key. Overwrite previous value
public function set(string $cache_key, mixed $value){}

//deletes whatever is stored under $cache_key, if it exists
public function delete(string $cache_key)

//connect to and disconnect from the cache server
private function connect(){}
private function disconnect(){}

Notice that this is a singleton . There can never be more than one Cache object in memory. This is to ensure that all your scripts are storing and retrieving data to and from the same cache.  You would use the static method getInstance to grab a reference to the cache. Since the method is static, you call it on the class itself (Cache::getInstance()).

Once you have this in place, when you need a value that may be cached, you check for it in the cache first. For instance, suppose you previously saved the sport type data under cache key "sportsType_123". In your controller's edit function,  change your code from:

$SportsType = $this->sportstype->Get($SportsTypeID);

To:

$SportsType = Cache::getInstance()->get('sportsType_'.$SportsTypeID)? :
              $this->sportstype->Get($SportsTypeID);

Likewise when you're saving data that you want cached, cache it after you've successfully saved to the DB. I've been using Memcache as my PHP cache driver with no problem, but Memcached may be even more feature rich. Because I have a Cache class as I described above, it wouldn't be too hard to switch. The rest of my scripts don't care because they interact with my Cache class, never directly with the cache driver.

  Am I writing bad or very bad code? Can you please suggest good ways to
  improve it?

I have a couple of concerns with your code:

From what you posted, I cannot tell whether you're sanitizing the data before saving it to the database. Hopefully you're not just inserting strings you received from the client into your DB queries (this would expose you to SQL Injection attacks), but are instead using parameterized queries.
How do you handle a SELECT query not returning any result? For instance when I look at SportsTypeDb::Get, you call ->first() on your results without checking whether there is such a result. I'm not familiar with Laravel so maybe it handles this for you? Be sure to test how your script handles getting a non-existent resource.
In your SportsTypeBL class, you create a new instance of SportsTypeDb every time a method is called. This is a waste because you will have a lot of parallel connections to your database that you never close and all those objects must be kept in memory. A better approach would be to instantiate a new SportsTypeDb() in the constructor of SportsTypeBL, assign it to a class variable, and use it for all DB queries. Then in the __destruct() function of SportsTypeBL, disconnect from the DB.


Calculating edges of a social network I am working on an open Chess data-set (~15500 rows after cleaning), and I create nodes and edges. But the way I create the edges takes a bit of time.
A sample of my nodes tibble:

player

1
bougris

2
a-00

3
ischia

...
...

A sample picture of the per_game tibble:

The way I do it:

I iterate for each node/player in nodes tibble,
searching the games where he played as black in per_game,
and exchanging values of column white_d with black_id, while changing the winner in winner column (using the swap() method I created).
Then, with calc_victories() method, I group all games for the specific player, with every opponent he has faced, and calculating how many times he won, or lost, from the opponent (I store it to player_result). An example picture:

Then I append the player_result to a tibble, with all previous players' results.
Finally, I delete from the per_game tibble the node I just handled, both from black and white columns.

Here is my code:
for(i in 1:dim(nodes)){
    # Exchange values of white with black column, only where black_id is the specific player
    per_game[per_game$black_id == nodes[[1]][i], c('white_id', 'black_id', 'winner')] <-
        per_game[per_game$black_id == nodes[[1]][i], c('black_id', 'white_id', swap('winner'))]

    # Calculate the victories, for each opponent of the specific player
    player_results <- calc_victories(nodes[[1]][i])

    # Append the player's matches with the rest.
    all_results <- rbind(all_results, player_results)

    # Delete all matches with the specific player, either if he/she is black or white
    per_game <- subset(per_game, white_id != nodes[[1]][i] & black_id != nodes[[1]][i])
}
all_results

Here are the functions calc_victories() and swap():
# A method to group the matches for a player, and sum his victories against each opponent
calc_victories <- function(i='-') {
    player_results <- per_game %>%
        filter(white_id==i | black_id==i) %>% # Finds matches with the specific player
        group_by(white_id, black_id) %>%
        rename(player1=white_id, player2=black_id) %>%
        summarise_at(vars(total_matches), list(victories = sum)) %>% # Summarize total matches
        arrange(desc(victories)) %>% # Sorts descending
        ungroup()

    return (player_results)
}

# A method to change the winner, because of white-black column exchange
swap <- function(winner='draw') {
    if(winner=='black'){
        gets = 'white'
    } else if(winner=='white'){
        gets = 'black'
    } else {
        return (winner)
    }
    return(gets)
}

The code executes for about 5 minutes, to handle all nodes. I think that this is happening, mainly because I iterate for each node. Maybe I should use something like map, but I am not so sure. Thank you.

General tips

Avoid defining your own loops. This is especially true with R because the chances are that there is another more elegant way to solve the problem using highly optimized functions.
Avoid rbind function inside loops especially if your are using it as an appending mechanism. This function creates a new empty data frame with a number of rows equal to number or rows of the old plus the number or rows of the new. Then it will copy the old and the new into this newly created data frame. This operation can be really expensive if you have large dataset. An optimizing solution can be achieved by creating an empty data frame with right size once outside the loop and populate it with the results using i index e.g all_results[i] = new_results inside the loop.
Avoid subset inside loops. The function documentation states that this is only a convienet function and should only be used in interactive setting.It also recommends that you use [ for subsetting.

Warning
This is a convenience function intended for use interactively. For programming it is better to use the standard subsetting functions like [, and in particular the non-standard evaluation of argument subset can have unanticipated consequences.

Moreover, it has the same issue as in the tip #2 since you are writing a new data frame minus the processed row in every go inside the loop.
Alternative solution
The problem can be solved using much less code than provided. Also it will only involves where the real data is the per_game table.
First you need to breakdown winner column in 3 for each winner. Since you seem familiar with the tidyverse family packages I will use functions from these packages.
library("tidyr")
library("dplyr")

per_game %>%
# Here we breakdown the winner columns into 3 using `spread()` function from tidyr
spread(winner, total_matches) %>%
# Then we add the black and white columns into new column called `victories`
rowwise() %>% # to ensure summation excute on per row.
mutate(victories  = sum(black, white,na.rm = TRUE))

# the results should look like this

# Rowwise:
   white_id           black_id           black  draw white victories
   <chr>              <chr>              <dbl> <dbl> <dbl>     <dbl>
 1 --jim--            "voodooo"              1    NA    NA         1
 2 -1-jedi_knighl_-1- "erik123678"          NA    NA     1         1
 3 -1-jedi_knighl_-1- "kaY\\ran0098"         2    NA     2         4
 4 -1-jedi_knighl_-1- "pav1ngt"              1    NA    NA         1
 5 -mati-             "astronavtearl101"    NA    NA     1         1
 6 -pavel-            "hishamtheman"        NA    NA     1         1
 7 1063314            "dccbc,ss"            NA    NA     1         1
 8 1111112222         "crusova_ 33"          1    NA    NA         1
 9 1111112222         "steelviper"           1    NA    NA         1
10 1240100948         "aa22bb"               1    NA    NA         1


Returns two elements from a list whose sum is a target variable def search(a,b):
    for d in b:
        if a==d:
            m=True
            break
        else:
            m=False
    return m

x=[1,4,5,7,9,6,2]
target=int(raw_input("Enter the number:"))
for i in x:
    if i<target:
        pair=int(target)-int(i)
        in2=search(pair,x)
        if in2==True:
            print "the first number= %d the second number %d"%(i,pair)
            break

Your search function is terribly unclear. search is a vague name, and a, b, d and m don't communicate anything. Looking at the usage I can see you're testing if a number is in the list, but Python can already do that for you much more readably.

if number in list

The in keyword tests if a value exists in a list, returning either True or False. So you can forget about search and just directly test for pair's existence in the main loop:

x = [1,4,5,7,9,6,2]
target = int(raw_input("Enter the number:"))
for i in x:
    if i < target:
        pair = int(target) - int(i)
        if pair in x:
            print "the first number= %d the second number %d"%(i,pair)
            break

Back to the naming, x is not clear, at least using numbers indicates what it contains. x implies a single value which makes using for i in x extra confusing. pair also sounds like it's multiple items, when really it's the difference you want.

You also don't need to call int on i when the list is populated with integers already.

Lastly, you're using the old form of string formatting. You should instead use str.format, like so:

            print("the first number= {} the second number {}".format(i,pair))

It works very similarly, except that it's type agnostic. It will just do it's best to get a string representation of the two values and insert them. There's lots of other uses for this approach so it's good to get used to.


Speed up lights-out variant solver in pure Python I'm doing some challenges to learn new and interesting concepts, this one is about a Lights out variant, instead of toggling only the adjacent tiles, it toggles the whole row and col.

It must be solved in pure python, so no numpy or other libraries that might help are allowed here.

I adapted the algorithm of this excellent ressource. Using a n2*n2 toggle matrix and a flattened puzzle P we can use a linear algebra to calculate T*x = P mod 2, where x is the matrix of buttons we need to push to solve the puzzle.

Since 1 < n < 16, this algorithm may need to solve matrices up to 225x225, which takes ~420ms on my machine. The exact time constraints aren't given, but I get a timeout error after submitting.

I pasted a running example here, this generates a random 15x15 puzzle and prints out a few timings and the solution to that particular puzzle.

The method performGaussianElimination(toggle, puzzle) takes by far the longest and seems to be the only bottleneck, but I don't see a place where I could use memoization or other shortcuts to save time. I've looked for other pure python linear algebra solvers, but there are only a few (numpy is so much easier) and don't seem to be a lot different.

def performGaussianElimination(toggle, puzzle):
    nextFreeRow = 0
    n = len(puzzle)
    for col in xrange(n):
        pivotRow = findPivot(toggle, nextFreeRow, col)
        if pivotRow is None:
            continue

        swap(toggle, nextFreeRow, pivotRow)
        swap(puzzle, nextFreeRow, pivotRow)

        for row in xrange(pivotRow + 1, n):
            if toggle[row][col]:
                for i in xrange(len(toggle[row])):
                    toggle[row][i] ^= toggle[nextFreeRow][i]
                puzzle[row] ^= puzzle[nextFreeRow]

        nextFreeRow += 1

    return toggle, puzzle

Can I speed this up? Might this traditionally fast algorithm not be the fastest for this variant?

You have a matrix of bits. You could represent each row by a single integer. (Remember that in Python the size of int is unlimited, so working with 255 bits is not a problem.)  Then you would be able to avoid the inner loop here

for row in xrange(pivotRow + 1, n):
    if toggle[row][col]:
        for i in xrange(len(toggle[row])):
            toggle[row][i] ^= toggle[nextFreeRow][i]

and do this instead

col_mask = 1 << col
for row in xrange(pivotRow + 1, n):
    if toggle[row] & col_mask:
        toggle[row] ^= toggle[nextFreeRow]


Firefox profile specs to speed up web scraping I am scraping a pool of URLs with a python/selenium framework using Firefox and Geckodriver.

For each URL, all I am doing is to find one element by class name and save it to disk. In order speed this up as much as possible, I am creating a custom Firefox profile with the specs defined below in firefox_profile.set_preference.

The challenge is that testing all combinations of all Firefox profile parameters in order to know which specs are best for speed is laborious. Therefore, I am hoping to get some input.

Are there any preferences parameters beyond the ones mentioned below that also would speed up Firefox?
Is any of those preference parameters mentioned below not suitable to speed up Firefox?

def create_driver():
        firefox_profile = webdriver.FirefoxProfile()
        firefox_profile.set_preference('browser.download.animateNotifications', False)
        firefox_profile.set_preference('browser.fullscreen.animate', False)
        firefox_profile.set_preference('browser.preferences.animateFadeIn', False)
        firefox_profile.set_preference('browser.tabs.animate', False)
        firefox_profile.set_preference('browser.cache.use_new_backend', 1)
        firefox_profile.set_preference('browser.sessionhistory.max_total_viewers', 0)
        firefox_profile.set_preference('browser.safebrowsing.enabled', False)
        firefox_profile.set_preference('browser.shell.checkDefaultBrowser', False)
        firefox_profile.set_preference('browser.startup.page', 0)
        firefox_profile.set_preference('layout.animated-image-layers.enabled', False)
        firefox_profile.set_preference('extensions.checkCompatibility', False)
        firefox_profile.set_preference('extensions.checkUpdateSecurity', False)
        firefox_profile.set_preference('extensions.logging.enabled', False)
        firefox_profile.set_preference('extensions.update.autoUpdateEnabled', False)
        firefox_profile.set_preference('extensions.update.enabled', False)
        firefox_profile.set_preference('print.postscript.enabled', False)
        firefox_profile.set_preference('toolkit.storage.synchronous', 0)
        firefox_profile.set_preference('image.animation_mode', 'none')
        firefox_profile.set_preference('images.dither', False)
        firefox_profile.set_preference('content.notify.interval', 1000000)
        firefox_profile.set_preference('content.switch.treshold', 100000)
        firefox_profile.set_preference('nglayout.initialpaint.delay', 1000000)
        firefox_profile.set_preference('network.dnscacheentries', 200)
        firefox_profile.set_preference('network.dnscacheexpiration', 600)
        firefox_profile.set_preference('network.prefetch-next', False)
        firefox_profile.set_preference('permissions.default.image', False)
        firefox_profile.set_preference('dom.ipc.plugins.enabled.libflashplayer.so', False)
        firefox_profile.set_preference('dom.ipc.plugins.flash.disable-protected-mode', False)
        firefox_profile.set_preference('app.update.enabled', False)
        firefox_profile.set_preference('app.update.service.enabled', False)
        firefox_profile.set_preference('app.update.auto', False)
        firefox_profile.set_preference('app.update.staging', False)
        firefox_profile.set_preference('app.update.silent', False)
        firefox_profile.set_preference("javascript.enabled", False)
        driver = webdriver.Firefox(firefox_profile = firefox_profile)
    return driver

Regardless of if these settings are correct, or if you could get by without a webbrowser, this is not a very good way to set all these values. At the very least extract the key, value pairs to a dictionary, which you ideally read from a config file.

import webdriver

SETTINGS = {'browser.download.animateNotifications': False,
            ...}

def create_driver():
    firefox_profile = webdriver.FirefoxProfile()
    for key, value in SETTINGS.items():
        firefox_profile.set_preference(key, value)
    driver = webdriver.Firefox(firefox_profile = firefox_profile)
    return driver


Struggling to unit test Angular, was this module the best approach? As a development team we've recently started to transition from MVC to Angular. None of us have had prior experience with it so a lot of creating the new client was very much trial and error. I've been put in charge of sorting out unit tests however I'm finding it very difficult to unit test.

In my experience if something is difficult to unit test it generally means some refactoring is in order. Problem is I do not have enough experience with Angular to say yay or nay on this.

Main angular module:

angular.module('serviceApp', ['mgcrea.ngStrap', 'ngRoute', 'LocalStorageModule', 'account.module', 'admin.module', 'design-code-review.module', 'early-load-test.module', 'integration-load-test.module', 'production-validation.module', 'risk-modelling.module', 'unit-test-profiling.module','error.module'])
.config(['$routeProvider', function ($routeProvider) {
    $routeProvider
        .when('/login', {
            templateUrl: '/views/account/login.html',
            controller: 'account.controller'
        })
         .when('/admin', {
             templateUrl: '/views/admin/project-admin.html',
             controller: 'admin.controller'
         })
        .when('/register', {
            templateUrl: '/views/account/register.html',
            controller: 'account.controller'
        })
        .otherwise('/login');
}])
.config(function($httpProvider){
    $httpProvider.interceptors.push('authentication.interceptor')
})
.run(['authentication.service', function (authentication) {
    authentication.populateAuthentication();
}]);

Declared in other js files are factories and services for our serviceApp

Example factory class:

angular.module('serviceApp')
.factory('storage.factory', ['$http', 'applicationSettings', 'localStorageService', function ($http, applicationSettings, localStorageService) {

    var getItems = function () {
        return $http.get(applicationSettings.apiLocation + 'Items/GetAll').then(function (response) {
            localStorageService.set('items', response.data);
        },
        function (error) {
            console.log("Could not retrieve data.");
        });
    };

    return {
        getItems: getItems
    };
}]);

Currently I'm having a lot of trouble trying to test these factories and services because of the sheer number of dependencies that serviceApp has. When looking for examples online I see that a lot of modules have only 1 or 2 dependencies which got me thinking whether the above is typical of an Angular application. Is having a lot of dependencies for a module like this ok in Angular? Or am I being a bit dense and need to get my unit testing in order?

Injecting the $injector service can help by facilitating the use of optional dependencies:

app.factory('templateRepository', [ '$injector', function($injector) {
  function lookupUrlFor(contentType) {
   // Do your translation here
  }

  return {
    getTemplate: function(contentType) {
      var templateCache = $injector.get('$templateCache');
      return templateCache.get(lookupUrlFor(contentType));
      }
    }
}])

That way the unit tests only needs to inject $injector and mocks can be avoided.

References

Optional Dependencies in Angular.js
Avoid circular dependencies with AngularJS Directives | endjin blog
ngMock / $httpBackend


Function to validate a file name This function checks if a file name is valid or not.  I think it's not good enough for integration (not optimized, maybe I forgot about some checks), so need your comments.

static
int ReservedName( const char * name )
{
    char * reservedFileNames[] = { "CON",  "PRN",  "AUX",  "NUL",
                                   "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9",
                                   "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9" };

    char * reservedFileNamesWithDot[] = { "CON.",  "PRN.",  "AUX.",  "NUL.",
                                          "COM1.", "COM2.", "COM3.", "COM4.", "COM5.", "COM6.", "COM7.", "COM8.", "COM9.",
                                          "LPT1.", "LPT2.", "LPT3.", "LPT4.", "LPT5.", "LPT6.", "LPT7.", "LPT8.", "LPT9." };

    int i;
    int n = sizeof( reservedFileNames ) / sizeof( reservedFileNames[0] );

    for( i = 0; i < n; i++ )
    {
        if( stricmp( reservedFileNames[i], name ) == 0 )
        {
            return 1;

        }else{

            if( strnicmp( reservedFileNamesWithDot[i], name, strlen( reservedFileNamesWithDot[i] ) ) == 0 )
            {
                return 1;
            }
        }
    }

    return 0;
}

static
int GetCharsNumInPath( IN const char * path )
{
    int i, cnt;

    for( i = 0; path[i]; i++ )
    {
        if( ( path[i] < 0x80 ) || ( path[i] > 0xbf ) )
            cnt++;
    }

    return cnt;
}

int ValidName( const char * name )
{
    if( name != NULL )
    {
        if( GetCharsNumInPath( path ) <= MAX_PATH )
        {
            if( ! ReservedName( name ) )
            {
                int i;
                int n = strlen( path );

                for( i = 0; i < n; i++ )
                {
                    switch( path[i] )
                    {
                        // ? " / < > * |
                        // these characters can not be used in file or folder names
                        //
                        case '<':
                        case '>':
                        case '/':
                        case '\\':
                        case '\"':
                        case '?':
                        case '*':
                        case '|':
                            return 0;

                        case ' ':
                        case '.':
                        {
                            if( ( i + 1 == n ) )
                            {
                                return 0;

                            }else{

                                continue;
                            }
                        }
                    }
                }

                return 1;

            }else{ // if( ! ReservedName( name ) )

                return 0;
            }

        }else{ // if( GetCharsNumInPath( IN path ) <= MAX_PATH_LEN )

            return 0;
        }

    }else{ // if( name != NULL )

        return 0;
    }
}

I'm not sure what you're using this for, but my advice: Don't bother validating the filename.  Just open the file.  The system knows the rules better than you do, and by adding too much validation you can cause problems down the line.

On Windows these rules are especially complex.  Frankly you will never find them all.  Here's a few you missed, off the top of my head:

Filenames may contain unicode characters.  You are using char*.  Unless you're using UTF-8 and passing all your filenames to MultiByteToWideChar before they reach a file API, you're ruling out a lot of potential valid paths based on that.  (Not to mention the way these filenames get mapped to Unicode is now subject to the user's language settings, which one can argue is a bad thing in its own right...)
MAX_PATH.  You're assuming that all paths will fit in a buffer of that size.  On Windows you can actually have a path longer than MAX_PATH if it starts with \\?\; also things like \\?\C:\foo are valid paths.
Names containing trailing spaces or dots, which you seem to be filtering on, are also valid and distinct if the path begins with \\?.  Without that prefix, trailing dots and spaces get truncated.
Paths under \\.\, \\?\GLOBALROOT.

Not to mention, these rules could potentially be subject to change.  Note that Win9X had different rules than NT.  What if such a change occurs again and breaks your logic?  Don't bother duplicating these rules in your code; don't second-guess the filesystem either.  Just pass the name to CreateFile.

(Aside: If your filename happens to come from an untrusted source, then it probably is fair to be overly restrictive...)


Await and interactive Android ADB shell I'm writing an app to let me interact and debug my android over USB using the android SDK and the ADB command.  I create a Process with redirected input and output streams and basically run "ADB shell", and I want to be able to wait for the prompt, enter commands and parse the results.

Here's a sample running ps and getting the results:

CreateProcess("shell");
string data = await ReadToPrompt(":/ $ ");
_Process.StandardInput.WriteLine("ps");
string echo = await ReadLine(); // my command echoed back
string dataAndPrompt = await ReadToPrompt(":/ $ ");

It works (although I'm getting extra CR in the byte stream, CR/LF is coming as CR/CF/LF), but I can't help but wonder if there's a better way than what I have:

Process _Process;
MemoryStream _Remaining; // bytes left after ReadToPrompt
byte[] _Buffer = new byte[0x10000];

public async Task<string> ReadLine()
{
    return await ReadToPrompt("\r\n");
}

public async Task<string> ReadToPrompt(string prompt)
{
    MemoryStream ms = _Remaining == null ? new MemoryStream() : _Remaining;
    int promptSize = prompt.Length;
    int pos = 0;

    while (true)
    {
        // read what we can from the process's standard output
        int count = await _Process.StandardOutput.BaseStream.ReadAsync(_Buffer, 0, _Buffer.Length);
        ms.Write(_Buffer, 0, count); // write new data to memory stream

        // convert all new bytes to check for prompt
        string testString = Encoding.ASCII.GetString(ms.GetBuffer(), pos, (int)(ms.Position - pos));
        int promptPos = testString.IndexOf(prompt);
        if (promptPos > 0)
        {
            // we found it, result is up to and including prompt
            int stripLength = pos + promptPos + promptSize;
            string result = Encoding.ASCII.GetString(ms.GetBuffer(), 0, stripLength);

            // store remaining data in a MemoryStream for the next call
            _Remaining = new MemoryStream();
            _Remaining.Write(ms.GetBuffer(), stripLength, (int)(ms.Position - stripLength));
            return result.Replace("\r\r", "\r");
        }

        // we didn't find it but we know it isn't in what we got, so move first position
        // to check to the position - promptSize
        pos = (int)(ms.Position - promptSize);
        pos = (pos < 0) ? 0 : pos;
    }
}

Is there something I can reliably do to keep from creating new MemoryStreams?  For instance, could I do an array copy on MemoryStream.GetBuffer() to itself and use MemoryStream.SetLength()?  Is there an entirely better way to do it than using async and await?

MemoryStream ms = _Remaining == null ? new MemoryStream() : _Remaining;

This is exactly what the null-coalescing operator is for:

MemoryStream ms = _Remaining ?? new MemoryStream();

int count = await _Process.StandardOutput.BaseStream.ReadAsync(_Buffer, 0, _Buffer.Length);

Why are you using BaseStream? Without it, you could work directly with chars instead of bytes, which saves you having to use Encoding to convert those bytes back to string.

  Is there something I can reliably do to keep from creating new MemoryStreams?

Yes: don't use MemoryStream at all. Instead, I would use StringBuilder, since it allows you to remove characters from its start, which is exactly what you need. As an added advantage, it works with chars, not bytes.

  Is there an entirely better way to do it than using async and await?

Your use of async-await seems reasonable to me. Though I'm not sure it's actually necessary: I think synchronous methods would have worked just as well.


Pixel bit-wise LSB extraction to byte array I wrote the following code below to extract the least significant bit (LSB) of each pixel and store it into a byte[] array. I was wondering if Java has a more efficient way to do this?

Right now, I am looping over every pixel, extracting the LSB, and updating the current byte by shifting it to the left and adding the LSB. When a byte is filled up, the next byte is then set as the current one. The process is repeated until all pixels' LSBs have been extracted and fed into the byte array.

import java.util.Arrays;

public class Main {
    public static void main(String[] args) {
        int[] pixels = new int[] {
            0xFFFF0000, 0xFFFF0001, 0xFFFF0000, 0xFFFF0001,
            0xFFFF0000, 0xFFFF0001, 0xFFFF0001, 0xFFFF0001,
            0x00000001, 0x00000000
        };
        // Result should be 3 bytes [0101 (5), 0111 (7), 10 (2)]
        System.out.println(Arrays.toString(extractLSB(pixels)));
    }

    public static byte[] extractLSB(int[] pixels) {
        int pixelCount = pixels.length;
        int numberOfBytes = (int) Math.ceil(pixelCount / 4.0);
        int extraBits = pixelCount % 4;
        int overflowIndex = pixelCount - extraBits;
        int currByteIndex = 0;
        int currByteValue = 0;
        byte[] byteBuffer = new byte[numberOfBytes];

        for (int i = 0; i < pixelCount; i++) {
            int currPixel = pixels[i];
            int bitIndex = i % 4;
            int bit = currPixel & 1;

            currByteValue |= bit;

            if (i > overflowIndex && bitIndex == extraBits - 1) {
                byteBuffer[currByteIndex] = (byte) currByteValue;
            } else if (bitIndex < 3) {
                currByteValue <<= 1;
            } else if (bitIndex == 3) {
                byteBuffer[currByteIndex++] = (byte) currByteValue;
                currByteValue = 0;
            }
        }

        return byteBuffer;
    }
}

I believe your basic algorithm is fine. Shifting only 4 bits on to each byte is not exactly filling the byte, but, there are ways I would consider neatening up the process. Note that this does not change the algorithm much at all.

Here are some tricks to note, though...

You should pass in the number of bits you want in a byte as a parameter value, or it should be a constant. The value 4 is a magic number that should not be there in the method.

private static final int BITS_IN_BYTE = 4;

You have: int numberOfBytes = (int) Math.ceil(pixelCount / 4.0);. This can be done "better" with:

int numberOfBytes = (pixelCount + BITS_IN_BYTE - 1) / BITS_IN_BYTE;

You declare convenience variables which really are unnecessary....
You are working with the data 'backwards'.... you build up the byte, then set the value in to the output, instead of just shifting the bits in to the output.

Here's how I would have written your code. It is more concise, but essentially the same.

public static byte[] extractLSBRL(final int perByte, final int[] pixels) {
    final int outSize = (pixels.length + perByte - 1) / perByte;
    final byte[] lsbs = new byte[outSize];
    for (int i = 0; i < pixels.length; i++) {
        // which output byte is the one for this pixel
        final int j = i / perByte;
        // make space for the new bit to arrive.
        lsbs[j] <<= 1;
        // copy the bit in to the output byte's position.
        lsbs[j] |= pixels[i] & 1;
    }
    return lsbs;

}

Note that I have a concern about the output here. are you sure the last byte should be 10, and not 1000 (i.e. shifted two bits to the left?). Just checking... different algorithms expect different things....


Count number of pairs of strings which on joining contain all vowels Problem

I have \$N\$ words. For each valid \$i\$, word \$i\$ is described by a string \$D_i\$ containing only lowercase vowels, i.e. characters 'a', 'e', 'i', 'o', 'u'.

We check the number of strings \$D_i\$ and \$D_j\$ which, when concatenated into a new string \$M\$, contain all 5 vowels. What is the total number of (unordered) pairs of words such that when concatenated, they contain all the vowels?

Input

The first line of the input contains a single integer \$T\$ denoting the number of test cases. The description of \$T\$ test cases follows.
The first line of each test case contains a single integer \$N\$.
\$N\$ lines follow. For each valid \$i\$, the \$i^{th}\$ of these lines contains a single string \$D_i\$.

Output

For each test case, print a single line containing one integer - the number of concatenated words that contain all vowels.

Test Case

3
aaooaoaooa
uiieieiieieuuu
aeioooeeiiaiei

Output

2

Constraints

\$1≤T≤1,000\$
\$1≤N≤10^5\$
\$1≤|D_i|≤1,000\$ for each valid \$i\$
the sum of all |\$D_i\$| over all test cases does not exceed \$3⋅10^7\$

I have written some code but I want to optimize it:

#include<bits/stdc++.h>
using namespace std;

int main()
{
  int t;
  cin>>t;
  while(t--)
  {
    int n;
    cin>>n;
    vector<string> v;
    long long  con_s[n];
    for(int i=0;i<n;i++)
    {
      string s;
      cin>>s;
      con_s[i]=0;
      for(int j=0;j<s.length();j++)
      {
        con_s[i] = con_s[i] | (1<<(s[j]-'a'));
      }
    }
    long long complete = 0;
    complete = (1<<('a'-'a')) | (1<<('e'-'a')) | (1<<('i'-'a')) | (1<<('o'-'a')) | (1<<('u'-'a'));
    // cout<<complete;
    int count = 0;
    for(int i=0;i<n-1;i++)
    {
      for(int j=i+1;j<n;j++)
      {
        if((con_s[i] | con_s[j])==complete)count++;
      }
    }
    cout<<count<<"\n";
  }
  return 0;
}

Can you please suggest a more optimized solution...

Your Code:

Don't use <bits/stdc++.h>, it's unportable and inefficient. See "Why should I not #include <bits/stdc++.h>?" for the details.
Don't use using namespace std;, that namespace just isn't designed for it. See "Why is “using namespace std” considered bad practice?" for more background.
Invest in some better names. Being too parsimonious there hurts, even though brevity is a virtue.
Format your code consistentently.

Sometimes you have space around binary operators (other than comma, which only should be followed by a space), sometimes you forget.
Add a space after ; when it isn't followed by the next line, meaning a loop's head.
Also add a space after #include, for, if, and while.
You don't have to use a block for single statements, but please don't put them on the same line, at the least separate them with a space.

C++ does not have VLAs. Just use a std::vector instead.
While in general, keeping the scope of a variable minimal is a good idea, there are exceptions. One of them is efficiency. Always spinning up a new std::string means being unable to re-use the buffer. Not that you really need the whole string at once, anyway.
One reason you should minimise a variables scope, is that it allows you to initialise it to the proper value, instead of leaving it uninitialised or, horrors of horrors, adding a spurious dummy-initialisation.
That also allows you to avoid writing the type (Almost Always auto), and making it const or even constexpr.
There are compound-assignment-operators for most binary operators. Like a |= b for a = a | b. Using them leads to shorter, more readable code.
When you want to output a single character, why not use a character-literal? It's potentially even slightly more efficient.
return 0; is implicit for main(). Take it or leave it, but be aware.

The Algorithm:

Your algorithm uses \$O(\#characters+\#words^2)\$ time and \$O(max\_word\_length + \#words)\$ space.

An optimal algorithm only needs \$O(\#characters+2^{\#vowels})\$ time and \$O(2^{\#vowels})\$ space:

For every word:

Set the bits for all the vowels contained.
Increment the count on the indicated bin.

For every vowel:

Iterate the bins containing words with that vowel.
Add the count to the respective bin without that vowel.

For all bins:
Multiply the count in the bin with the count in the complementary bin, and add that.
Subtract twice the count in the bin for words containing all vowels. Those account for all self-pairings.
The answer is half the calculated number, as we counted double.

Exercise for the attentive reader: Save half the multiplications this algorithm uses.


Simple stack implementation using linked list Please review my Stack using linked list:

template<class T>
struct Node {
    T item;
    Node* next;
    Node(T t = {}, Node* link = nullptr) :item{t}, next{link} { }
    ~Node() { delete next; }
};

template<class T>
class Stack {
public:
    int size() const { return n; }
    bool empty() const { return n == 0; }
    void push(const T&);
    T pop();
    T peek();
private:
    Node<T>* first;
    std::size_t n;
};

template<class T>
void Stack<T>::push(const T& t) {
    Node<T>* old{first};
    first = new Node<T>{t,old};
    ++n;
}

template<class T>
T Stack<T>::pop() {
    if (empty()) {
        throw std::out_of_range("underflow");
    }
    auto t = first->item;
    first = first->next;
    --n;
    return t;
}

template<class T>
T Stack<T>::peek() {
    if (empty()) {
        throw std::out_of_range("underflow");
    }
    return first->item;
}

A few notes:

I would like to experiment to try to avoid allocating in the heap, but I really can't because I don't know how to keep the newly created and added node in push() alive.
Is it correct to use std::size_t when tracking the size of the stack?

I thought that since my Stack class acts like a handle to nodes, I thought I should write my own copy and move operations so I can avoid member-wise copying, but something very funny happens when I tested my Stack:

int main() {
    Stack<std::string> x{};
    x.push("str");
    x.push("tring");
    x.push("string");
    Stack<std::string> y = x;
    y.pop();
    std::cout << x.peek() << '\n';
    y.pop();
    std::cout << x.peek() << '\n';
    y.pop();
    std::cout << x.peek() << '\n';
    y.push("nox");
    std::cout << x.peek() << '\n';
    std::cout << x.size() << '\n';
    std::cout << y.size() << '\n';
}

When x.peek() is called the result is always "string", x.size() is 3 and y.size() is 1. It seems the default copy operation works correctly. I thought it shouldn't because first is a pointer so they should refer to the same elements.
So I did not write a copy and move operation because it seems the default operations work correctly but I really don't know why and how. Can anyone explain why?

How can I make this code better?

I see some things that you might want to use to improve your code.

Use appropriate #include files

The implementation requires #include <stdexcept> so that should part of the template.

Eliminate unneeded variables

The push function doesn't really need any temporary variables:

template<class T>
void Stack<T>::push(const T& t) {
    first = new Node<T>{t,first};
    ++n;
}

Create a default constructor for Stack

An instance of Stack is not necessarily initialized to any particular values when it's created.  To address this, you should implement a default constructor.

Stack() : first{nullptr}, n{0} {}

Match new with delete

If you allocate memory using new, you must also free it using delete or your program will leak memory.  Since you use new in push(), you should use delete in pop():

template<class T>
T Stack<T>::pop() {
    if (empty()) {
        throw std::out_of_range("underflow");
    }
    Node<T>* oldnode = first;
    T t = first->item;
    first = first->next;
    --n;
    delete oldnode;
    return t;
}

Create a copy constructor if you need one

The compiler will create a default copy constructor which does a shallow copy, but this won't work for data structures, like yours, which use pointers.  Instead, if you really want to create a new Stack from an existing one, you'll need to creat your own copy constructor:

template<class T>
Stack<T>::Stack(const Stack<T> &s) : first(nullptr), n(0) {
    for (auto t=s.first; t; t = t->next)
        push(t->item);
}

The default-constructed operator= will now use this copy constructor.

Create a destructor if you need one

For similar reasons, you need have a delete for Stack and not for Node.  The one for Stack could be like this:

template<class T>
Stack<T>::~Stack() {
    while (!empty())
        pop();
}

Use const where possible

The peek() function doesn't (and shouldn't) alter the underlying Stack so it should be declared const.

Consider using references

While this code works (with the adjustments listed above), ther is still some room for improvement.  First, things inserted into the stack are copied rather than moved there.  Also things that are popped from the stack are also copied rather than moved.  This means that there are many more constructor and destructor calls made than are strictly necessary.

To see how this works, here's a class that contains a string, but includes instrumentation so we can see what's happening:

class MyString {
public:
    MyString() : str(), id(serial++) { std::cout << "creating empty string " << id << "\n"; }
    MyString(const char *msg) : str(msg), id(serial++) {
        std::cout << "creating string \"" << str << "\" " << id << "\n"; }
    MyString(const MyString &s) : str(s.str), id(serial++) {
        std::cout << "copying string \"" << str << "\" " << id << "\n"; }
    MyString(MyString &&s) : str(), id(serial++) {
        std::swap(str, s.str);
        std::cout << "moving string \"" << str << "\" " << id << "\n"; }
    ~MyString() { std::cout << "destroying string " << id << "\n"; }
    friend std::ostream &operator<<(std::ostream &out, const MyString &m) {
        return out << m.str;
    }
private:
    static unsigned serial;
    std::string str;
    unsigned id;
};

unsigned MyString::serial = 0;

Now we specialize your templated stack and try it out:

int main() {
    Stack<MyString> x{};
    x.push("one");
    x.push("two");
    x.push("three");
    std::cout << x.peek() << '\n';
}

Output from this program (with the other fixes included):

creating string "one" 0
copying string "one" 1
copying string "one" 2
destroying string 1
destroying string 0
creating string "two" 3
copying string "two" 4
copying string "two" 5
destroying string 4
destroying string 3
creating string "three" 6
copying string "three" 7
copying string "three" 8
destroying string 7
destroying string 6
copying string "three" 9
three
destroying string 9
copying string "three" 10
destroying string 8
destroying string 10
copying string "two" 11
destroying string 5
destroying string 11
copying string "one" 12
destroying string 2
destroying string 12

As you can see, even though we've only got three items on the stack, we've created and destroyed 13 objects.  By making only a single small change to the Node constructor, we can reduce that to 10:

Node(const T &t, Node* link) :item{t}, next{link} {}

More savings could be realized by not using pop in the destructor since pop creates a copy of T to return:

template<class T>
Stack<T>::~Stack() {
    if (first) {
        for (Node<T> *n = first->next; n; n = first->next) {
            delete first;
            first = n;
        }
        delete first;
    }
}

Consider using smart pointers

Using something like a std::unique_ptr would free you (pun intended) from having to manage the mechanics of new and delete explicitly.