Object formatter using reflection I am making an object formatter for use when debugging.

Formatted class:

package com.myname.somepackage;

import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;

// Allows a variable to be displayed when using Formatter.format
@Retention(RetentionPolicy.RUNTIME)
public @interface Formatted {

}

Formatter class:

package com.myname.somepackage;

import java.lang.reflect.Field;

public final class Formatter {
    private Formatter() {}

    // Returns a string containing the object's information, for debugging
    // Format: ClassName[var1=somevalue, var2=somevalue]
    // The object's variables must have the Formatted annotation to be displayed here
    public static String format(Object object) {
        String className = object.getClass().getSimpleName();
        Field[] fields = object.getClass().getDeclaredFields();
        String string = className + "[";
        for (Field field : fields) {
            field.setAccessible(true);
            Formatted annotation = field.getAnnotation(Formatted.class);
            if (annotation != null) {
                String varName = field.getName();
                try {
                    String value = field.get(object).toString();
                    string += varName + "=" + value + ", ";
                } catch (IllegalAccessException e) {
                    e.printStackTrace();
                    string += varName + "=" + "{Unavailable}, ";
                }
            }
        }
        // remove last ", "
        if (string.endsWith(", "))
            string = string.substring(0, string.length() - 2);
        string += "]";
        return string;
    }
}

A class for testing this:

package com.myname.somepackage.math.geom.r2;

import com.myname.somepackage.Formatted;
import com.myname.somepackage.Formatter;

public final class Point2d {
    @Formatted
    private final double x, y;

    public Point2d(double x, double y) {
        this.x = x;
        this.y = y;
    }

    public double getX() {
        return this.x;
    }
    public Point2d setX(double x) {
        return new Point2d(x, this.y);
    }
    public double getY() {
        return this.y;
    }
    public Point2d setY(double y) {
        return new Point2d(this.x, y);
    }

    @Override
    public String toString() {
        return Formatter.format(this);
    }
}

The code to test it:

Point2d point = new Point2d(4, 2);
System.out.println(point);

The console then outputs "Point2d[x=4.0, y=2.0]".

How does my code look? I understand reflection is considered bad, but this is just my lazy way of quickly debugging. Thanks

It's pretty nice IMO but can be improved.

If your project uses apache commons (this library is often included), you should consider using the FieldUtils class to get the fields : https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/reflect/FieldUtils.html

Notably, the getFieldsWithAnnotation method would reduce your code complexity by a bit.

That's up to you though ;)

This part :

try {
    String value = field.get(object).toString();
    string += varName + "=" + value + ", ";
} catch (IllegalAccessException e) {
    e.printStackTrace();
    string += varName + "=" + "{Unavailable}, ";
}

may fail if your field is null, you should exploit the String + operator to avoid it like this :

string += varName + "=" + field.get(object) + ", ";

string is really a bad name for your variable, maybe rename it as res or something ?

I'm no big fan of the printStackTrace, you should consider using the various logging utilities proposed by java : https://docs.oracle.com/javase/9/docs/api/java/util/logging/Logger.html or slf4j.

I think those 4 modifications will already make the code neater but we can do a bigger refactoring :
instead of using a String that we concatenate bit by bit and then remove the final comma, you should consider using a Stream over the fields array and generating the result with the Collectors#joining method.

In the end, you'd have the following method :

private static final String SEPARATOR = ", ";

public static String format(Object object) {
    final String className = object.getClass().getSimpleName();
    final String prefix = className + "[";
    String res = Arrays.stream(FieldUtils.getFieldsWithAnnotation(object.getClass(), Formatted.class))
        .map(field -> {
        String varName = field.getName();
        try {
            return varName + "=" + field.get(object);
        } catch (IllegalAccessException e) {
            log.severe(e.toString());
            return varName + "=" + "{Unavailable}";
        }
    }).collect(joining(SEPARATOR));
    return prefix + res + "]";
}


"Can a list of meetings all be scheduled in a conference room?" I was given this question at an interview for a position I just got rejected by, so looking to improve and see what better answers there are!

Input: List of meetings

Output: Can conference room fit all the meetings

It was an open ended problem so I was kinda flustered. My first stab at it was probably too much in terms of set up (he was probably just looking for a solution, not so much for a grasp of object oriented principles) and I think I totally diverged from the Output, but still going to paste my answer anyway:

class Meeting
  attr_accessor :start_time
  attr_accessor :end_time

  def does_not_overlap?(start_time, end_time)
    self.start_time < start_time && self.end_time < end_time ||
    self.start_time > start_time && self.end_time > end_time ||
  end
end

class ConferenceRoom
  attr_accessor :meetings

  def initialize
    self.meetings = []
  end

  def bulk_schedule(list_of_meetings)
    currently_scheduled_meetings = self.meetings
    list_of_meetings.each do |meeting|
      self.meetings.each do |scheduled_meeting|
        if currently_scheduled_meetings.does_not_overlap?(meeting)
          schedule(meeting)
        else
          raise "Conference room can't fit all meetings"
        end
      end
    end
  end

  def schedule(meeting)
    self.meetings << meeting
  end
end

My brute force solution was this, which was just to loop through the array of meetings and see if any of them were scheduled at a certain time range when scheduling new meetings. This runs in \$O(n^2)\$ since I have to loop through an array for each element in another array. When he asked how I would optimize it, I said I would use a hash with the time range as keys and loop through those instead. However, thinking back on this I don't think that would improve things at all.

My friend said to sort the array and do binary search on it. However, I don't get how I would do that given the meetings have 2 properties we have to keep in mind (start time and end time), not just one. Can someone shed some light?

Like knut suggested, a simple solution would be to sort the meetings by start time and for each check if it conflicts with the next.

E.g. given a list of meetings:

conflict = meetings.sort_by(&:start_time).each_cons(2).any? do |a, b|
  a.end_time > b.start_time
end

No need to model a conference room, or raise exceptions; conflict will indicate if the meetings overlap or not.

If the question involved a list of pre-scheduled meetings, and having to check if a new meeting would fit, it'd be a different story. You could use something like this, but the question doesn't call for it.

As for the rest of your code, a few things stood out to me:

does_not_overlap? is a bit odd

it's a negative assertion, rather than overlaps?
why not pass in a Meeting instance instead of start and end times?
and it's got a syntax error (hanging ||)

ConferenceRoom should use the attr_accessor you created or at least @meetings – not self.meetings.
You raise which, yes, is a sort of output, but it's a bit much. All the question needs is a boolean answer.


Longest common prefix (Leetcode) Link here I'm currently learning c++ coming from a python background, so I'll include a solution in python and in c++ for the problem statement below, I'm including both for convenience, if you don't know c++, feel free to review python and vice versa.

Write a function to find the longest common prefix string amongst an array of strings. If there is no common prefix, return an empty string "".

Example 1:
Input: words = ['flower', 'flow', 'flight']
Output: 'fl'
Example 2:
Input: strs = ['dog', 'racecar', 'car']
Output: ''
longest_common_prefix.py
def get_longest(words):
    if not words:
        return ''
    common = words[0]
    for word in words:
        while not word.startswith(common):
            common = common[:-1]
    return common

if __name__ == '__main__':
    print(f"Longest prefix: \n{get_longest(['flower', 'flow', 'fly'])}")

Leetcode stats:

Runtime: 32 ms, faster than 76.56% of Python3 online submissions for
Longest Common Prefix.

Memory Usage: 14 MB, less than 100.00% of Python3 online submissions
for Longest Common Prefix.

longest_common_prefix.h
#ifndef LEETCODE_LONGEST_COMMON_PREFIX_H
#define LEETCODE_LONGEST_COMMON_PREFIX_H

#include <string_view>
#include <vector>

std::string_view get_common_prefix(const std::vector<std::string_view>& words);

#endif //LEETCODE_LONGEST_COMMON_PREFIX_H

longest_common_prefix.cpp
#include <iostream>
#include <string_view>
#include <vector>

std::string_view get_common_prefix(const std::vector<std::string_view> &words) {
    if (words.empty())
        return "";
    std::string_view common = words[0];
    for (auto word: words) {
        while (word.find(common, 0) != 0) {
            common = common.substr(0, common.size() - 1);
        }
    }
    return common;
}

int main() {
    std::vector<std::string_view> xxx{"flow", "flower", "fly"};
    std::cout << "Longest prefix:\n" << get_common_prefix(xxx);
}

Leetcode stats:

Runtime: 0 ms, faster than 100.00% of C++ online submissions for
Longest Common Prefix.
Memory Usage: 9.9 MB, less than 7.29% of C++
online submissions for Longest Common Prefix.

I'm only going to review the C++ code here, as all I could suggest for the Python code also applies to the C++, so is included in this review.
Firstly, the interface is quite limiting - the inputs need to be converted to vector of string-view objects, which is inconvenient if I have a linked-list of strings, or an input stream yielding QStrings.  I recommend changing to accept a pair of iterators, or in sufficiently modern C++, a std::ranges::range object.
This test is inefficient:
word.find(common, 0) != 0

If we don't find common at position 0, find() will continue searching the rest of the string (the Python code is better here).  We need an implementation of starts_with() (which is in C++20's std::string) - or better, we could use std::mismatch() to directly find how much of the strings are common, eliminating the loop where we repeatedly remove a single character.
Here's my attempt at that, also with a simple optimisation to return early when the common string becomes empty:
#include <algorithm>
#include <iterator>
#include <string_view>
#include <vector>

namespace
{
    template<typename String>
    String common_prefix(const String& a, const String& b)
    {
        using std::begin;
        using std::end;
        auto end_iter = std::mismatch(begin(a), end(a), begin(b), end(b));
        if (end_iter.first == end(a)) { return a; }
        if (end_iter.second == end(b)) { return b; }
        return String(begin(a), end_iter.first - begin(a));
    }
}

template<typename Iter, typename IterEnd = Iter>
std::string_view get_common_prefix(Iter first, IterEnd last)
{
    if (first==last) { return ""; }
    std::string_view common = *first;
    for (auto it = first;  it != last;  ++it) {
        common = common_prefix(common, *it);
        if (common.empty()) { return common; }
    }
    return common;
}

template<typename Container>
std::string_view get_common_prefix(const Container& words)
{
    using std::begin;
    using std::end;
    return get_common_prefix(begin(words), end(words));
}


Synchronizing async tasks Yes, I know there are plenty of libraries to get this done (Q, step, async), but only for learning I'm wondering if the following code is ok and usable or have major drawbacks and it is only usable at a small programs.

Usage:

var g = new Manager();

g.then (
    function() {
        console.log("hello");
        setTimeout( function () { g.done("hello done") }, 2000 );
    }
);

g.start();

g.then (
    function() {
        console.log("two");
        g.done ("two done");
    }
);

g.then (
    function(resp) {
        console.log("I got: " + resp);
        setTimeout( function () { g.done("happy ending") }, 2000 );
    }
);

g.then (
    function(resp) {
        console.log("last msg: " + resp);
        g.done();
    }
);

Class:

//...............................
// class Manager
//...............................
function Manager () {
    this.functionList = [];
    this.allDone = false;
} // ()

//...............................
//...............................
Manager.prototype.then = function (fun) {
    this.functionList.push (fun);
    if (this.allDone) {
        this.allDone = false;
        this.done(this.previousResult);
        console.log ( "then(): continuing");
    }

} // ()

//...............................
//...............................
Manager.prototype.done = function (prevRes) {
    this.previousResult = prevRes;
    if ( this.where <= this.functionList.length-2 ) {
        this.where++;
        this.functionList [this.where](this.previousResult);
    } else {
        this.allDone = true;
        console.log ( "done(): stopping because allDone: where=" + this.where + " and length = " + this.functionList.length);
    }

} // ()

//...............................
//...............................
Manager.prototype.start = function () {
    this.allDone = false;
    this.where=-1;
    this.done();
} // ()

Some points:

g.then(function() { … g.done ("two done");}) I (personally) don't like that signature, I'm more used to callbacks that get passed as parameters (g.then(function(done) { … done ("two done");})). They tend make the chained functions better reusable - in your case it might not matter since you're using dedicated function expressions anyway which are bound to g. And your signature simplifies argument handling in the Manager :-)
this.allDone = false; seems to be a misnomer for me. The way you're using it, it should rather be called startedAndNotRunning. Technically if you're initialising the empty list they are all done already :-)
This is highly confusing:

if ( this.where <= this.functionList.length-2 ) {
    this.where++;
    …

What happens to the last two functions? Nothing special. It better is written

this.where++;
if (this.where < this.functionList.length) {
    …

Then the iterator variable (where) will become the length of the array after the "loop", which is the common behaviour. Yet, you cannot use done any more to [re]start the chain - you'd need to decrement where before. Therefore I'd recommend to restructure the methods to

function Manager () {
    …
    this.where = 0;
}
Manager.prototype.start = function () {
    // you might reset allDone and where
    this.next();
};
Manager.prototype.done = function (prevRes) {
    this.previousResult = prevRes;
    this.where++;
    this.next();
};
Manager.prototype.next = function() {
    if (this.where < this.functionList.length)
        this.functionList[this.where](this.previousResult);
    else {
        this.allDone = true;
        console.log ( "done(): stopping at where=" + this.where + " with length = " + this.functionList.length);
    }
};
Manager.prototype.then = function (fun) {
    this.functionList.push (fun);
    if (this.allDone) {
        this.allDone = false;
        this.start();
        console.log ( "then(): continuing");
    }
};


Listing cities in an itinerary I have a has_many through relationship between my cities and travels that looks like this:

class Travel < ActiveRecord::Base
  has_many :cities, through: :destinations
end
class City < ActiveRecord::Base
  has_many :travels, through: :destinations
end

And on my Travel controller I have a method to join all its cities names:

  def city_names
    self.cities.map { |e| e.name }.join(', ')
  end

I installed the bullet gem and it tells me that this is a n+1 query and suggests that I add the includes method on the query, but as far as I can tell this method will only work if I call it directly on the Travel class.

You can use includes on any activerecord relation.

Judging by your message about having N+1 queries I suspect you are calling city_names somewhere in a loop. then the correct placement of the includes is the place where you select the elements to loop past.

I'm guessing it is for a view or something so i write some example code for .html.erb.

In controller:

@travels = Travels.all.includes(destinations: :cities)

In view:

<% @travels.each do |travel| %>
  ... Some code displaying travel info
  <%= travel.city_names %>
  ... More code displaying travel info
<% end %>

And a suggestion in line with what Mark Thomas wrote as a comment. You can refactor you city_names method for more readable code, either according to his suggestion or like this:

def city_names
  self.cities.map(&:name).join(', ')
end


Python Unit Coversion Code Optimization in terms of Space and Time Complexity Ok I am making unit converter and I have attached temperature conversion code below. The problem with this code I think is that I have written function for each and every single unit to convert to another unit.

So in code below there is one function to covert Celsius into Fahrenheit, Kelvin, Renkien and Newton. Then another function to convert Fahrenheit to Celsius,Kelvin, Renkien and Newton.

Now I do understand that every unit is related to one another. But I just can't think of a way if this conversion can be done using single function such that first you choose your source and then destination and using that without using five different function I can just finish in one.

Another way I thought of this was I can may be use dictionaries rather then using so many if else conditions but I would like know which one is more optimized in terms of both time and space complexity.

class TempConv:

def __init__(self,source,dest,val):
    self.frm=source
    self.to=dest
    self.x=val
    self.ans=0

def setFrm(self,arg):
    self.frm=arg

def setTo(self,arg):
    self.to=arg

def setX(self,arg):
    self.x=arg

def calculate(self):
    if self.x is "celc":
        self.ans=self.celcToAny()
    elif self.x is "farh":
        self.ans=self.farhToAny()
    elif self.x is "kel":
        self.ans=self.kelToAny()
    elif self.x is "renk":
        self.ans=self.renkToAny()
    elif self.x is "newt":
        self.ans=self.newtToAny()
    else:
        self.ans=self.x

#Celcius to Any
def celcToAny(self):
    if self.to is "farh":
        return (float(self.x)*1.8)+32
    elif self.to is "kel":
        return float(self.x)+273.15
    elif self.to is "renk":
        return (flaot(self.x)+273.15)*1.8
    elif self.to is "newt":
        return float(self.x)*0.33
    else:
        return self.x

#Fahrenheit to Any
def fahrToAny(self):
    if self.to is "celc":
        return (float(self.x)-32)*0.555555556
    elif self.to is "kel":
        return (float(self.x)+459.67)*0.555555556
    elif self.to is "renk":
        return float(self.x)+459.67
    elif self.to is "newt":
        return (float(self.x)-32)*0.183333333
    else:
        return self.x

#Kelvin to Any
def kelToAny(self):
    if self.to is "farh":
        return (float(self.x)*1.8)-459.67;
    elif self.to is "celc":
        return float(self.x)-273.15
    elif self.to is "renk":
        return float(self.x)*1.8
    elif self.to is "newt":
        return (float(self.x)-273.15)*0.33
    else:
        return self.x

#Rankine to Any
def renkToAny(self):
    if self.to is "farh":
        return float(self.x)-459.67;
    elif self.to is "celc":
        return (float(self.x)-491.67)*0.555555556
    elif self.to is "kel":
        return float(self.x)*0.555555556
    elif self.to is "newt":
        return (float(self.x)-491.67)0.183333333
    else:
        return self.x

#Newton to Any
def newtToAny(self):
    if self.to is "farh":
        return (float(self.x)*5.454545455)+32
    elif self.to is "kel":
        return (float(self.x)*3.030303030)+273.15
    elif self.to is "renk":
        return (flaot(self.x)*5.454545455)+491.67
    elif self.to is "celc":
        return float(self.x)*3.030303030
    else:
        return self.x

There is not much need for a class for what you are after. Storing all the conversion factors in a dictionary, you could simply do something like:

def convert_temp(val, from_, to_):
    if from_[0] == to_[0]:
        return val
    off1, mult, off2 = convert_temp.data[from_[0]][to_[0]]
    return (val + off1) * mult + off2
convert_temp.data = {'C' : {'F' : (0, 1.8, 32)},
                     'F' : {'C' : (-32, 0.555555556, 0)}}

And now:

>>> convert_temp(50, 'C', 'F')
122.0
>>> convert_temp(122, 'F', 'C')
50.000000039999996

You would of course have a larger dictionary with all possible conversions. You could get fancy and store only half the conversions:

def convert_temp2(val, from_, to_):
    if from_[0] == to_[0]:
        return val
    try:
        off1, mult, off2 = convert_temp2.data[from_[0]][to_[0]]
    except KeyError:
        off2, mult, off1 = convert_temp2.data[to_[0]][from_[0]]
        off1 = -off1
        off2 = -off2
        mult = 1 / mult
    return (val + off1) * mult + off2
convert_temp2.data = {'C' : {'F' : (0, 1.8, 32)}}

>>> convert_temp2(50, 'C', 'F')
122.0
>>> convert_temp2(122, 'F', 'C')
50.0


Sierpinski triangle html canvas Implementation I wrote my Sierpinski Triangle fractal animation using HTML canvas with JavaScript: JsFiddle
What do you think about it? How I can optimize it, if you see a need for that?
Code:
requestAnimationFrame(update);

const canvas = document.getElementById("c");
const context = canvas.getContext('2d');

var iterations = 0; //how many iterations to perform in current frame, count of fractal func invokes grows exponentially and can be calculated from sum: E 3^k, k = 0 to iterations
const maxIterations = 8; //used when animating, it's better to set 'gradientAnimation' to false, when setting 'maxIterations' to bigger values
const center = { x: canvas.width * 0.5, y: canvas.height * 0.5 };
const halfTriangleSide = 250;

//color animation
const gradientAnimation = true;
var t = 0;
var speed = 0.1;

//division animation
var divideAnim = true;
var divide = true; //loop flag
var mod = 0.0; //color modificator

function drawTriangle(A, B, C)
{
    context.beginPath();
    context.moveTo(A.x, A.y);
    context.lineTo(B.x, B.y);
    context.lineTo(C.x, C.y);
    context.lineTo(A.x, A.y);

    if(gradientAnimation)
    {
        context.fillStyle =
        "rgb(" +
            ((A.x + A.y) * 0.19 * mod) + "," +
            ((B.x + B.y) * 0.15 * mod) + "," +
            ((C.x + C.y) * 0.22 * mod) +
        ")";
    }

    context.fill();
}

function fractal(center, halfSideLen, i)
{
    i++;
    var quaterSide = halfSideLen * 0.5;

    //calc new triangle coords
    var A = {
        x: center.x - quaterSide,
        y: center.y
    };

    var B = {
        x: center.x + quaterSide,
        y: center.y
    };

    var C = {
        x: center.x,
        y: center.y + halfSideLen
    };

    drawTriangle(A, B, C);
    if(i < iterations)
    {
        fractal({x: A.x, y: A.y + quaterSide}, quaterSide, i);
        fractal({x: B.x, y: B.y + quaterSide}, quaterSide, i);
        fractal({x: C.x, y: C.y - (halfSideLen + quaterSide)}, quaterSide, i);
    }
}

function update()
{
    context.clearRect(0, 0, canvas.width, canvas.height);

    //init coords
    var A = {
        x: center.x - halfTriangleSide,
        y: center.y + halfTriangleSide
    };

    var B = {
        x: center.x + halfTriangleSide,
        y: center.y + halfTriangleSide
    };

    var C = {
        x: center.x,
        y: center.y - halfTriangleSide
    };

    context.fillStyle = "#F00";
    drawTriangle(A, B, C);
    context.fillStyle = "#000";

    if(iterations != 0) fractal(center, halfTriangleSide, 0);

    t += speed;
    if(t > Math.PI * 2)
    {
        t = 0;

        if(divideAnim)
        {
            if(iterations == maxIterations) divide = false;
            else if(iterations == 0) divide = true;

            if(divide) iterations++;
            else iterations--;
        }
    }

    if(gradientAnimation)
    {
        mod = 1.25 - Math.sin(t) * 0.5;
    }

    requestAnimationFrame(update);
}

Good things:

const is used for variables that shouldn't be re-assigned
requestAnimationFrame() is used for animating
constants and global variables are declared at the top of the script.
only three functions are used

Suggestions

add a space after control structures like if for readability: this is recommended in many style guides (e.g. AirBnB, Google)
Instead of

if(gradientAnimation)

Add a space:
if (gradientAnimation)

Also recommended by some popular style guides: avoid var unless a global variable is needed. Default to using const to avoid accidental re-assignment (which seems to be a topic of this discussion on your other JS post). If re-assignment is needed then use let.

Use equality operators that don’t coerce types when not needed:
Instead of:

if(iterations == maxIterations)

Use ===
 if (iterations === maxIterations)

A common convention in many languages (including C-based and others) is to have constants that truly never change named using ALL_CAPS - e.g. MAX_ITERATIONS instead of maxIterations, HALF_TRIANGLE_SIDE instead of halfTriangleSide, etc.

the lone i++ at the start of fractal() can be moved down to conditional after the call to drawTriangle() and converted to a pre-increment:
    if (++i < iterations)

Most of the (idiomatic) JS code and JS Style guides I have seen have opening braces on the same line as the function signature or control structure instead of a new line. Perhaps having them on a new line is common in another style guide (e.g. C++, C#, etc.).


Generic BinaryReader/Writer Read/Write methods I have an extension that implements all the BinaryReader/BinaryWriter in the Stream class. I want to create generic methods for them. Is there a nicer way of doing this?

public static void Write<T>(this Stream stream, T num)
{
    if (num is byte)
    {
        stream.Write((byte)(object)num);
    }
    else if (num is Int16)
    {
        stream.Write((Int16)(object)num);
    }
    else if (num is Int32)
    {
        stream.Write((Int32)(object)num);
    }
}

public static T Read<T>(this Stream stream)
{
    object ret = null;

    if (typeof(T) == typeof(byte))
        ret = stream.ReadInt8();
    else if (typeof(T) == typeof(Int16))
        ret = stream.ReadInt16();
    else if (typeof(T) == typeof(Int32))
        ret = stream.ReadInt32();

    if (ret == null)
        throw new ArgumentException("Unable to read type - " + typeof(T));
    return (T)ret;
}

Now using this for the Read method.

static Dictionary<Type, Func<Stream, object>> ReadFuncs = new Dictionary<Type, Func<Stream, object>>()
{
    {typeof(bool), s => s.ReadBoolean()},
    {typeof(byte), s => s.ReadInt8()},
    {typeof(Int16), s => s.ReadInt16()},
    {typeof(Int32), s => s.ReadInt32()},
    {typeof(Int64), s => s.ReadInt64()},
    {typeof(string), s => s.ReadString()},
};
public static T Read<T>(this Stream stream)
{
    if (ReadFuncs.ContainsKey(typeof(T)))
        return (T)ReadFuncs[typeof(T)](stream);

    throw new NotImplementedException();
}

I'm guessing you defined more write methods than this? Otherwise the Write() would call itself recursively infinitely. You probably have overloads for every supported type, making the generic write method superfluous. This would also explain the weird casting you are doing. I'm guessing an overloaded function with a specific type has precedence over a generic method, which is why it could work. This however, doesn't make it a clean solution.

Your Read() method seems to be more useful. Instead of throwing a ArgumentException I would prefer throwing a NotSupportedException.

As a sidenote, are you sure you want to make these extension methods of Stream and not some more specific stream? Stream can be any type of stream.

In what way is this an improvement over using BinaryWriter/BinaryReader? For the purposes I gather from your code I would only create a Read<T> extension method for BinaryReader.

If you feel like experimenting a bit, there might be a more performant/cleaner solution, although you'll have to benchmark to be sure. You could create a Dictionary<Type, Func<object>> to hold a matching 'reader' for every type. Initializing the delegates to store in this dictionary is the tough part, on which information can be found in this article. Most likely this is not worth the effort, although it still is a very interesting article. :) Be sure to also check out the same solution implemented by using Expression Trees in the comments of the article. For a similar implementation example I did, check out this question I posted on Code Review.


Generating binary sequences without repetition I am trying to generate sequences containing only 0's and 1's. I have written the following code, and it works.
import numpy as np

batch = 1000
dim = 32

while 1:
    is_same = False
    seq = np.random.randint(0, 2, [batch, dim])
    for i in range(batch):
        for j in range(i + 1, batch):
            if np.array_equal(seq[i], seq[j]):
                is_same = True
    if is_same:
        continue
    else:
        break

My batch variable is in the thousands. This loop above takes about 30 seconds to complete. This is a data generation part of another for loop that runs for about 500 iterations and is therefore extremely slow. Is there a faster way to generate this list of sequences without repetition? Thanks.
The desired result is a collection of batch_size number of sequences each of length dim containing only 0s and 1s such that no two sequences in the collection are the same.

Generating all sequences and then checking if they are unique can be quite expensive, as you noticed. Consider this alternative approach:

Generate one sequence
Convert it to a tuple and add it to a set
If the set is of size batch_size return it, else go to step 1

This approach can be implemented like this:
def unique_01_sequences(dim, batch_size):
    sequences = set()
    while len(sequences) != batch_size:
        sequences.add(tuple(np.random.randint(0, 2, dim)))
    return sequences

Running the two solutions for dim=32 and batch_size=1000:
Original: 2.296s
Improved: 0.017s

Note: the result of the function I suggested is a set of tuples, but it can be converted to the format you prefer.
A few other suggestions and considerations:

Functions: consider to encapsulate the code in a function, it is easier to reuse and test.
Exit condition: this part:
if is_same:
    continue
else:
    break

Can be simplified to:
if not is_same:
    break

binary vs integer sequence: probably you know it, but the result is a "sequence" of integers, not a binary sequence as the title says.
Collisions: the suggested approach can become very slow for some configurations of dim and batch_size. For example, if the input is dim=10 and batch_size=1024 the result contains all configurations of 10 "bits", which are the binary representations of the numbers from 0 to 1023. During the generation, as the size of the set sequences grows close to 1024, the number of collisions increases, slowing down the function. In these cases, generating all configurations (as numbers) and shuffling them would be more efficient.
Edge case: for dim=10 and batch_size=1025 the function never ends. Consider to validate the input.


Quadratic equation solver in C++ // ConsoleApplication1.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include <cstdlib>
#include <iostream>
#include <cmath>

using namespace std;

int main() {
    system("cls");
    system("color 5");

    float a;
    float b;
    float c;
    float Sector1Arithmetic;
    float Sector2Arithmetic;
    float Sector3Arithmetic;
    float Sector4Arithmetic;
    float Sector5Arithmetic;
    float X1;
    float X2;

    cout << endl;
    cout << "            Quadratic Equation Solver";
    cout << endl;
    cout << endl;
    cout << "       Enter the value of the a co-efficient ---> ";
    cin >> a;
    cout << endl;
    cout << endl;
    cout << "       Enter the value of the b co-efficient ---> ";
    cin >> b;
    cout << endl;
    cout << endl;
    cout << "       Enter the value of the c co-efficient ---> ";
    cin >> c;
    cout << endl;

    Sector1Arithmetic = b * -1;
    Sector2Arithmetic = b * b - 4 * a * c;
    Sector3Arithmetic = sqrt(Sector2Arithmetic);
    Sector4Arithmetic = Sector1Arithmetic + Sector3Arithmetic;
    Sector5Arithmetic = Sector1Arithmetic - Sector3Arithmetic;
    X1 = Sector4Arithmetic / 2 * a;
    X2 = Sector5Arithmetic / 2 * a;

    cout << "  For the equation " << a << "x^2 + " << b << "x + " << c <<     endl;
    cout << endl;
    cout << "  X=" << X1 << " OR " << "X=" << X2 << endl;

    if ((-4 * a * c) >= (b * b) || (a == 0) || (b == 0) || (c == 0)) {
    cout << endl;
    cout << "An error might have occurred due to impractical math calculations" << endl;
    cout << "OR due to invalid values inputted such as 0" << endl;
    cout << endl;
    }

    cout << endl;
    system("pause");
    main();
    return 0;

}

As you may remember I also posted a quadratic equation solver in Python as well. The main reason I post this simple program is to get critical feedback and perhaps look for alternative ways of coding such simple program applying it to my future projects.

You have committed a grave error here with operator precedence, which should have easily been caught with simple testing:

X1 = Sector4Arithmetic / 2 * a;

The variable names SectornArithmetic are bizarre.  Most of those intermediate results don't need to be assigned to a variable.  The most complex intermediate result, b * b - 4 * a * c, has a standard name: "discriminant", which you should use.

You can use if (discriminant < 0) to detect that there are no real roots.  The other tests — (b == 0) and (c == 0) — are superfluous and harmful, as there is no justification for prohibiting those coefficients from being 0.

As mentioned in the previous Python review, system("pause") is inappropriate.  Just call std::getline() and discard the result.

Calling main() recursively is worse than a goto, as you are deepening the call stack for no reason.  Write a proper loop instead.  Furthermore, the return 0; statement is unreachable and should be eliminated.


Bash Script - File Comment out & Notate I have been writing my own Bash Scripts for a little while now. I am getting a better comprehension of it and I would like to start refining my code to be more elegant. I have recently developed this code to take a PHP file and comment out the code and add new code in.

If you could let me know how I could do better I would greatly appreciate it.

Bash Script:

## Set included files ## START ##

source /scripts/_inc/siteList.sh ## Description: Loads $siteList Array

## Set included files ## END ##

## Set Variables ## START ##

## Files in Use ## START ##
useFile='deferred.php'
tempFile='/scripts/_inc/deferredTextTemplate.php'
## Files in Use ## END ##

## File Contents ## START ##
newTop=$(head -n 15 $tempFile)
newBottom=$(tail -n 4 $tempFile)
## File Contents ## END ##

## Set Variables ## END ##

## Set Functions ## START ##

changeFile () {

    ## Info About Action to be intered into File.
    workInfo="\n// Edited: $(date)\n// Directory: $2"
    logTag="$(date): [$3 @ $2]"

    if [ -f "$1" ] ## Description: Does the file exist?
    then
        if grep -q F8kR8ikBiF "$1"; ## Description: Does the file contain this marker?
        then
            printf "%s Marker exists no changes necessary.\n" "$logTag" ## Description: Marker Exists!
        else
            ## Description: Print to Files that exist without Marker.
            orgFileCont=$(tail -n +2 $1 | sed -r 's/\/\*+ (.*) \*+\//\/\/ \1/')
            printf "%s\n%s\n%s\n%s\n" "$newTop" "$workInfo" "$orgFileCont" "$newBottom" > $1
            printf "%s File edited.\n" "$logTag"
        fi
    else
        printf "%s File not found.\n" "$logTag" ## Description: File does not exist.
    fi

}

## Set Functions ## END ##

## Set actions ## START ##

## Loop though all direcotries listed in $siteList array and perform action
for siteDirectory in "${siteList[@]}"
do
   fileLocation="/site/$siteDirectory/$useFile"
   changeFile "$fileLocation" "$siteDirectory" "$useFile"
done

## Set actions ## END ##

##EOF

Here is the Template File that is called:

<?php

/* Marker: F8kR8ikBiF */

/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
* File Altered By Automated SH script via Cron: overwriteDeferred.sh
* script created by xxxxxxxxxxxxxxxxxxxx 10-24-14 in order to disable
* the built in cron functioanlity of Xxxxxxxx. Cron functionality will
* now be triggered by a System Cron Script: manCronTrigger.sh to run
* once per minute.
* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */

{"moreDeferred":false}

/* Oringinal content * START *

* Oringinal content * END */

// EOF

No need to use quotes here, though it's not a problem:

useFile='deferred.php'
tempFile='/scripts/_inc/deferredTextTemplate.php'

On the other hand, it might be a good idea to double-quote $tempFile here:

newTop=$(head -n 15 $tempFile)
newBottom=$(tail -n 4 $tempFile)

It's hard to guess the meaning of the parameters $2 and $3 here:

changeFile () {

    ## Info About Action to be intered into File.
    workInfo="\n// Edited: $(date)\n// Directory: $2"
    logTag="$(date): [$3 @ $2]"

One way to improve that is to document them in a comment.
An even better way would be to reassign them to local variables with descriptive names.

The marker F8kR8ikBiF is buried deep within the script:

    if grep -q F8kR8ikBiF "$1"; ## Description: Does the file contain this marker?

It would be better to put it in a variable with a good name,
defined near the top of the script.

You were careful to double-quote most variables correctly, but not always:

orgFileCont=$(tail -n +2 $1 | sed -r 's/\/\*+ (.*) \*+\//\/\/ \1/')

You should double-quote the $1 there.

Also, the \/ make the expression hard to read. It would be better to use a different separator, for example ?:

... | sed -r 's?/\*+ (.*) \*+/?// \1?'

When you have an if-else block like this:

if [ -f "$1" ]
then
    # ... many many lines
    # ... many many lines
    # ... many many lines
else
    printf "%s File not found.\n" "$logTag"
fi

When there are too many lines in the middle,
it can be more readable to invert the if-else branches:

if [ ! -f "$1" ]
then
    printf "%s File not found.\n" "$logTag"
else
    # ... many many lines
    # ... many many lines
    # ... many many lines
fi


Finding the first two numbers that sum to a given number Task aim: Given a list of integers and a single sum value, return the first two values (parse from the left please) in order of
  appearance that add up to form the sum.

I've been trying to make my implemented function below run faster (less than 7000ms) and taking into account records upwards of 10,000,000 items.

While it works for basic tests such as these:

sum_pairs( [1, 4, 8, 7, 3, 15], 8 ) == [1, 7]
sum_pairs( [1, -2, 3, 0, -6, 1], -6 ) == [0, -6]
sum_pairs( [20, -13, 40], 7 ) == undefined
sum_pairs( [1, 2, 3, 4, 1, 0], 2 ) == [1, 1]
sum_pairs( [10, 5, 2, 3, 7, 5], 10 ) == [3, 7]
sum_pairs( [4, -2, 3, 3, 4], 8 ) == [4, 4]
sum_pairs( [0, 2, 0], 0 ) == [0, 0]
sum_pairs( [5, 9, 13, -3], 10 ) == [13, -3]

it breaks with some other unseen tests (which presumably have some tricky 'edge cases' and thus, breaks my implementation which I suspect isn't an optimal solution.

It's been weeks now and I really would appreciate some guidance or signpost to some helpful resource to set me on the right track.

var sum_pairs = function (ints, s) {
    //your code here
    var i = 0,
    j = 0,
    len = ints.length,
    min = undefined,
    count = 0;

    for (i; i < len; i++) {
        j = i + 1;
        while (j < len) {
           if (s - ints[i] === ints[j]) {
               if ( !min || (j < min[1]) ) {
                   min = [i, j];

                   // Check if worth continuing
                   close = (i+1===j);
                   if ( close ) break;
                }
            }
            j++;
        }
    }

    if (min !== undefined){
        min[0] = parseInt(ints[min[0]]);
        min[1] = parseInt(ints[min[1]]);
    }
    return min
}

Some issues with the code:

close isn't defined anywhere and will shoot up to the global scope.
Your variables are terribly named. Use names that make sense.
Suggesting a var per variable so they become portable. You can easily copy and paste without having to remove the comma and re-add the var.
I am not sure why you would need parseInt for the end part of your logic when ints appears to be an array of numbers.
Prefer function declaration (function foo(){}) over function expression (var foo = function(){}). Although variables and functions have the same namespace, function declarations get hoisted. They will always be available regardless if they appear later in the code.
JS uses camelCase convention.

For performance:

I suggest you keep a hash of first values so that whenever they come up again, like [ 1, 2, 3, 1, 6, 7], you can skip the second time it occurs, saving you a full run through the array.

Here's my take on it:

function sumPairs(numbers, sum){
  let first = null;
  let second = null;
  let firstHistory = {};

  for(let i = 0; i < numbers.length; i++){
    first = numbers[i]

    // Skip if we've already seen this number as the first number
    if(firstHistory[first]) continue;

    // Remember that we've seen this number before
    firstHistory[first] = true;

    for(let j = 0; j < numbers.length; j++){
      second = numbers[j];

      // Skip if we're on the same index
      if(i === j) continue;

      // Return once we've found it. Forget the rest.
      if(first + second === sum) return [first, second];
    }
  }

  // The only time the function ever reaches this point is when it never
  // returned (it never found the values). Functions return an implicit
  // undefined, which satisfies your "not found" case.
}

It doesn't cover one test case though. It sees [10, 5, 2, 3, 7, 5] as [5, 5] instead of [3, 7]. However, the definition of "earlier" in the condition is not clear. [5, 5] is found earlier than [3, 7] from an execution perspective.

And it might not be perfect, but the idea is there.


Wrapping an Exception with context-management, using the with statement Before I try to make this work, I wondered if anyone had tried this, whether it was a good idea or not, etc.

I find myself doing this a lot:

if some_result is None:
    try:
        raise ResultException
    except ResultException:
        logger.exception(error_msg)
        raise ResultException(error_msg)

I would rather try this:

if some_result is None:
    with ResultException:
        logger.exception(error_msg)

Where the __exit__() statement would always raise the exception in the context.

Would this be clearer? Is this possible?

I only ask because I imagine someone else has tried this, or at least attempted it. But I don't seem to find any common patterns for accomplishing this after doing a couple of Google searches.

UPDATE

Here's some more information on this particular issue.

I have a method that goes to a database and determines what the user's id is via their email. The problem is that if the user isn't found, it'll return None. This is okay for most calls, but when it's incorporated in higher-level methods, this can happen:

def change_some_setting(setting, email):
    user_id = user_id_by_email(email) # None
    _change_some_setting(setting, id)

Now when _change_some_setting attempts to change the setting for a user who's id is None, we get a very vague ProgrammingException from the database library.

This helps prevent that. Is there a better way?

This:

if some_result is None:
    with ResultException:
        logger.exception(error_msg)

would seem to be equivalent (for most cases) to:

if some_result is None:
    logger.exception(error_msg)
    raise ResultException

And so using a context manager as you describe it would not seem to be helpful. That's probably why you can't find any instances of it. But this would not actually work because when you call logger.exception, you need to be in an exception handler.

You could do:

def raise_logged(exception):
    try:
         raise exception
    except:
         logging.exception(str(exception))
         raise exception

Then use it like

 if some_result is None:
       raise_logged( ResultException("not so good") )

However, throwing the exception just to catch it and log it is a bit yucky. Do you really need to log the exception as this point? Usually, we log exceptions when they are caught.


First password generator in Python This is my first project using Python. I made a simple password generator that checks user input. How can I improve it?
import random
def password_generator():
    password = []
    letters = ["a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", "o", "p", "q", "r", "s", "t", "u",
               "v", "w", "x", "y", "z"]
    password_length = 0
    password_numbers = []
    password_letters = []
    # Input the length of the password
    while True:
        password_length_input = input("Choose the length of your password with numbers between 6 and 15:\n")
        if not password_length_input.isnumeric():
            print(f"{password_length_input} is not a number, try again:")
            continue
        else:
            password_length = int(password_length_input)
            print(f"Password length: {password_length}")
        if 6 <= password_length <= 15:
            break
        else:
            print("The password must be between 6 and 15 characters, try again:")
            continue
    # Input the amount of numbers in password
    while True:
        password_numbers_input = \
            input(f"Choose the amount of numbers you want in your password, max {password_length}\n")
        if not password_numbers_input.isnumeric():
            print(f"{password_numbers_input} is not a number try again")
            continue
        elif int(password_numbers_input) > password_length:
            password_numbers = 0
            print(f"The value is too high, choose maximum {password_length} numbers")
            continue
        else:
            password_numbers = int(password_numbers_input)
            print(f"Password numbers: {password_numbers}")
            for number in range(0,password_numbers):
                password.append(random.randrange(0,9))
            break
    # Check for numbers and letters in password
    while True:
        if password_numbers == password_length:
            print(f"The password will be only {password_numbers} numbers, no letters.")
            break
        else:
            password_letters = password_length - password_numbers
            print(f"""Your password will be {password_length} characters with {password_numbers} numbers and {password_letters} letters.""")
            for letter in range(0,password_letters):
                password.append(random.choice(letters))
            break
    random.shuffle(password)
    password_string = ''.join([str(item) for item in password])
    print(f"Your password is:\n{password_string}")

password_generator()

Usage example:
Choose the length of your password with numbers between 6 and 15:
Password length: 8
Choose the amount of numbers you want in your password, max 8
Password numbers: 2
Your password will be 8 characters with 2 numbers and 6 letters.
Your password is:
pzc11bmf

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

This method of writing the alphabet is very error prone. I would import string and use string.ascii_lowercase in place of letters. If you want to generate your own range of letters for whatever reason, I would write
letters = [chr(n) for n in range(ord('a'), ord('z') + 1)]

since there's then no danger of omitting or duplicating a letter.

password_length = 0
password_numbers = []
password_letters = []

These default values are never used. The defaults for password_numbers and password_letters don't make sense since those variables hold numbers. I would delete all three lines.

if not password_length_input.isnumeric():
    print(f"{password_length_input} is not a number, try again:")
    continue
else:
    password_length = int(password_length_input)
    print(f"Password length: {password_length}")

I would instead write
try:
    password_length = int(password_length_input)
except ValueError:
    print(f"{password_length_input} is not a number, try again:")
    continue
print(f"Password length: {password_length}")

while True:
    if password_numbers == password_length:
        ...
        break
    else:
        ...
        break

There is no sense in having a while loop here since you always break out of it on the first iteration.

range(0,password_numbers)

You can just write range(password_numbers).

password.append(random.randrange(0,9))

This will append a digit from 0 to 8 inclusive, never 9. If you want all ten digits you should write random.randrange(10). Or, perhaps better, use random.choice(string.digits).

password_string = ''.join([str(item) for item in password])

If you use string.digits then every element of password will be a character so you can simplify this to password_string = ''.join(password).


Transposing a PHP associative array The purpose of this code is to change each column to a row, and assign keys to each value for the new array according to the old array. I would like to know if there're other ways to optimize it.

<?php

$arr = [
    'name' => ['a', 'b', 'c'],
    'age'  => [ 2 ,  1 ,  3 ]
];

$result = [];
$keys   = array_keys($arr);
foreach($arr[$keys[0]] as $index => $temp) {
    $data = [];
    foreach($keys as $i => $key) {
        $data[$key] = $arr[$key][$index];
    }
    $result[] = $data;
}

print_r($result);

Which gives:

$result = [
    ['name' => 'a', 'age' => 2],
    ['name' => 'b', 'age' => 1],
    ['name' => 'c', 'age' => 3],
];

There isn't really any great performance improvement to be made here in my opinion.  You original structure is going to require nested loop approach to generate your desired structure regardless.

My only suggestion would be building an array of objects in your result rather than an array of associative arrays, as I think what you have shown in your example is more meaningful as objects (i.e. items that have a set of properties) vs. as an associative array (which I typically like to use more for hashmap type of use cases).


Mini-git class performance issue I want to build Git class where i can store integers. FilesSet is where I store current state of integers. Commits' key is basically commit number so I can checkout pretty fast. Hashes is used for quick search of hashes that I need in commit part.

Update is for updating current state of integers.

In Commit I want to save a copy of current state and do it fast. First of all I don't want to compare arrays of integers, so I calculate hash of current state. Then if Hashes already has that hash as a key, I add to Commits Pair of commitNumber and  integer array that corresponds to that hash so I won't have to store another copy of array. And in Hashes of this key I add commit Number so I can see which commits have that hash. This one is a bottleneck for my solution.

In Checkout I basically get int that I need by arguments. I think this method is already really fast.
I need this class to be fast but I can't really provide test cases as they are hidden at my college testing system. My current solution is too slow with int Commit() .
Here it is :

using System.Collections.Generic;
using System.Linq;
using System;

namespace GitTask
{
    public class Git
    {
        int[] FilesSet { get; set; }
        Dictionary<int, int[]> Commits { get; set; }
        SortedList<int, List<int>> Hashes { get; set; }

        public Git(int filesCount)
        {
            FilesSet = new int[filesCount];
            Commits = new Dictionary<int, int[]>();
            Hashes = new SortedList<int, List<int>>();
        }

        public void Update(int fileNumber, int value)
        {
            FilesSet[fileNumber] = value;
        }

        public int Commit()
        {
            var commitNumber = Commits.Count;
            var tempHash = GetMyHashCode(FilesSet);

            if (Hashes.TryGetValue(tempHash, out List<int> key))
            {
                Commits.Add(commitNumber, Commits[key[0]]);
                Hashes[tempHash].Add(commitNumber);
                return commitNumber;
            }
            Commits.Add(Commits.Count, FilesSet.Clone() as int[]);
            Hashes.Add(tempHash, new List<int>(new int[] { commitNumber }));
            return commitNumber;
        }

        public int Checkout(int commitNumber, int fileNumber)
        {
            if (commitNumber >= Commits.Count)
                throw new ArgumentException();
            else
            {
                var commit = Commits[commitNumber];
                return commit[fileNumber];
            }
        }

        private int GetMyHashCode(int[] array)
        {
            int result = 0;
            for (int i = 0; i < array.Length; ++i)
            {
                result = unchecked(result * 31 + array[i]);
            }
            return result;
        }
    }
}

I expect you to come up with tips on speeding it up or creating new kind of storing files. I also tried to measure performance of methods and it seems like calculating hash of big array is a problem and adding to SortedList<int,List<int>> is a problem as well.

Hash code collisions

There's a problem with Commit: matching hash codes do not mean that two objects are equal ('pigeonhole principle'). For example, both [1, 0] and [0, 31] have hash code 31. You can only be sure that objects are not equal when their hash codes don't match - the inverse is not true.

Also, are you sure that reusing arrays is worth the effort? If filesCount is small, then the savings are small as well, and if filesCount is large, then the likelyhood that a specific state has occurred before is probably very small.

Multiple smaller arrays

One alternative is to divide the state into multiple smaller arrays. This limits the impact of individual changes: you only need to copy a 'sub-array' (and create a new 'array-of-arrays'), instead of copying the full state. This also increases the likelyhood that a particular sub-array has occurred before, although I do not think that is something you need to be concerned about.

In Git terms, this is similar to using multiple files instead of a single large file. Committing changes to a single file only creates a new blob for the modified file and a new tree object that references the new blob (and older blobs for unmodified files).

Change-sets

Another alternative is to store a change-set for each value, using a binary search to find the value for a particular commit number. This can save a lot of space, but it'll make checkouts slower. Whether that trade-off is worth it depends on the intended use of this class.

Other notes

Instead of Clone() as int[], you can use Linq's ToArray() method.
I'd put the last 4 lines of Commit inside an else statement, to make it clearer that these are two mutually exclusive paths.
On the other hand, I would not use an else statement in Checkout - the if check is an 'early-out' guard clause, not a full-blown separate path.
In Checkout, an ArgumentOutOfRangeException might be better suited. You may also want to guard against negative values. And why not check for invalid file numbers as well?


Simple calculation program in assembly MASM I am a beginner in assembly. This program calculates:

(Z+Z)+(X-Y)

DATA_SEG    SEGMENT
      X     DW 2
      Y     DW 3
      Z     DW 5
      RES1  DW ?
      RES2      DW ?
 DATA_SEG        ENDS
 CODE_SEG        SEGMENT

           ASSUME CS:CODE_SEG, DS:DATA_SEG

;**********************************************
 MAIN    PROC    FAR          ;MAIN PROGRAM STARTS HERE.

        MOV     AX,DATA_SEG
            MOV     DS,AX      ;INITIALIZING THE DS REGISTER.

            ; ***FIRST WE CALCULATE (Z+Z) AND SAVE IN RES1****
            MOV     AX,Z     ;
            add     AX,Z     ;  AX = Z+Z
            MOV     RES1,AX  ;  HOLD TEMPORARILY THE RESULT OF (Z-Z) IN RES1

        ; ***SECOND WE CALCULATE  (Y-X) AND SAVE IN RES2***
        MOV     AX,Y     ;  NOW STORE VAR Y IN AX REGISTER
        SUB     AX,X     ;  PERFORM  AX = Y-X
            MOV     RES2,AX  ;  HOLD TEMPORARILY THE RESULT OF (Y-X) IN RES2

            ; ***NOW ADD BOTH RES1 + RES2***

            MOV     AX,RES1  ; STORE  RESULT OF (Z+Z) IN AX REGISTER
            ADD     AX,RES2  ;  ADD RES1 + RES2

        ;****NOW FOR PRINTING WE MOVE THE VALUE IN DX REGISTER****

        MOV     DX,AX    ; tO PRINT WE FIRST MOVE DATA IN DX REGISTER
        ADD     DX,30H ;CONVERT TO ACSII CODE
            MOV     AH,2
            INT     21H
        MOV     AH,04CH
            INT     21H
MAIN    ENDP
CODE_SEG        ENDS
END     MAIN

I have some suggestions that may help you improve your code.

Fix formatting

There seem to be varying numbers of leading spaces on each line, impeding easy reading.  Typically labels and directives start in column 1 and instructions start in column 9 (one tabstop) and are all aligned there.  Many, including me, additionally align comments as well.

Make comments more useful

Comments like this are useful because they explain why we're doing something.

MOV     DX,AX    ; tO PRINT WE FIRST MOVE DATA IN DX REGISTER

Although it would be easier to read if it wasn't in ALL CAPS.  Other comments are not useful because they explain something obvious from the instruction:

 MAIN    PROC    FAR          ;MAIN PROGRAM STARTS HERE.
    ;  other code
    MOV     DS,AX      ;INITIALIZING THE DS REGISTER.

Eliminate "magic numbers"

There are a few numbers in the code, such as 30H and 2 that have a specific meaning in their particular context.  By using named constants such as ASCII_ZERO or PRINT_CHAR, the program becomes easier to read and maintain.

Think carefully about the problem

As mentioned in a comment, if you're really calculating (Z-Z)+(X-Y) then you can simply ignore Z because (Z-Z) is always going to be zero.  Next, the description says X-Y but the code says Y-X.  It's usually essential to clearly understand the problem at hand before beginning to write the code.

Prefer registers to memory

Register use is typically much faster than memory use.  For that reason, I'd recommend eliminating memory use that isn't needed.  In particular, RES1 and RES2 could easily be eliminated by either chaining operations or storing temporary values in a register.  By "chaining operations" I mean that (Z-Z)+(Y-X) is equivalent to Z-Z+Y-X (or simply Y-X) so one could simply evaluate the expression from left to right.

Use the appropriate OS service

The INT 21h, AH=02h is the "Write character to standard output" service, so it's only going to print a single character.  That's fine if you're only ever going to have a single digit non-negative result, but that doesn't seem assured here.  Better would be to create a function that converts the numeric result in AX to a string in memory and then use a function such as INT 21h, AH=40h, "Write to file or device" for output.

Set all appropriate registers for use by the OS services

The program exits with a call to the DOS "Terminate with error code" function (INT 21h, AH=4Ch) but fails to actually set the error code in AL.


Generating Ulam numbers with OpenMP and single-thread versions I'm making a program that takes an integer n and generates the first n Ulam numbers. I followed this guide about OpenMP.

This is the core function, single thread version:

bool isulam (int n, int size) {
    int count = 0;
    for (int i = 0; i < size; i++)
        for (int j = 0; j < size; j++) {
            if (i != j && ulam[i]+ulam[j] == n) count++;
            if (count > 2) return false;
        }
    return count;
}

And this is my attempt at optimizing it with OpenMP:

bool isulam (int n, int size) {
    int count = 0;
    bool toomany = false;
    for (int i = 0; i < size; i++)
        #pragma omp parallel for reduction (|:toomany)
        for (int j = 0; j < size; j++) {
            if (i != j && ulam[i]+ulam[j] == n) count++;
            if (count > 2) toomany = true;
        }
    if (count > 2) return false;
    return count;
}

I'm compiling with g++ -Ofast -fopenmp. The output is correct, but the OpenMP version is much slower:

$ time ompulam <<< 1000 > /dev/null

real  0m22.211s
user  0m39.697s
sys   0m3.202s
$ time ulam <<< 1000 > /dev/null

real  0m7.073s
user  0m7.017s
sys   0m0.008s

What's happening? My CPU is an AMD E1 2500 (2 cores, 1400Mhz) which may not be the best, but I was hoping for a much different result. Is OpenMP only worth on 4+ cores?

FWIW with a regular #pragma omp parallel for (thus without the toomany), the code is running in 18.583s.

My CPU is an AMD E1 2500

That CPU is a dual core CPU, so with perfect multithreading that introduces exactly zero overhead, which at best could double the performance. As the comments already pointed out, the overhead is more than zero. Other than that, your CPU is a Kabini low power CPU with 15W TDP, so it's not unlikely that thermal throttling lowers the clock speed when running multithreaded - try to look at the actual clock speed (not % use of CPU) when running the test again.

With that out of the way, the problem in your code is that you introduce absolutely massive overhead. In addition to the overhead due to having threads, the variable count is shared between threads, which forces the threads to wait for each other all the time. Also, your code will always run the entire loop, while the single-threaded version aborts early. A much faster version is the following, although that one still does not abort early (disclaimer: not tested, as I don't have OpenMP installed).

bool isulam (int n, int size) {
    int count = 0;
    bool toomany = false;
    for (int i = 0; i < size; i++)
        #pragma omp parallel for reduction (+:count)
        for (int j = 0; j < size; j++) {
            if (i != j && ulam[i]+ulam[j] == n) count++;
        }
    return count <= 2;
}


Call third parties in parallel I am trying to speed up some reporting on all employees but I am having difficulty determining where the "proper" place to parallelize my tasks. In order, this is what my code is currently doing.

Ask for all developers
Create a Developer object for each employee
REST call to Jira to get JSON of all tickets for employee
REST call to Github using info from Jira json to get pull request information
Hydrating JiraTicket objects with Jira and Github jsons
Returning developers

I have the below code to make the requests to Github and Jira. The getJiraTicketsAsync takes the json from Jira, the json contains info on all tickets. The getPullRequestURLsFromTicketId method makes another call to Jira as you cannot get development information from the JQL call to get the tickets json. My question is: am I parallelizing in the proper place? Should I move it up to a higher level and just call createDeveloper() in parallel?

private Set<JiraTicket> getJiraTicketsAsync(JsonArray ticketsArray)
{
    List<Callable<JiraTicket>> tasks = createJiraTicketTasks(ticketsArray);
    Set<JiraTicket> tickets = new HashSet<>(tasks.size());
    try
    {
        ExecutorService service = Executors.newCachedThreadPool();
        List<Future<JiraTicket>> futures = service.invokeAll(tasks);
        for(Future<JiraTicket> future: futures){
            try
            {
                tickets.add(future.get());
            } catch (ExecutionException e)
            {
                logger.error("There was an error getting a jira ticket", e);
            }
        }
        service.shutdown();
    } catch (InterruptedException e)
    {
        logger.error("Interrupted hydrating jira ticket", e);
    }
    return tickets;
}

private List<Callable<JiraTicket>> createJiraTicketTasks(JsonArray ticketsArray)
{
    List<Callable<JiraTicket>> tasks = new ArrayList<>(ticketsArray.size());
    for (int n = 0; n < ticketsArray.size(); n++)
    {
        int index = n;
        tasks.add(()->{
            JsonObject ticketJson = ticketsArray.get(index).getAsJsonObject();
            JsonArray pullRequestsJsonArray = getPullRequestURLsFromTicketId(ticketJson.get("id").getAsString());
            ticketJson.add("pullRequestUrls", pullRequestsJsonArray);
            return ticketHydrator.hydrateJiraTicket(ticketJson);
        });
    }
    return tasks;
}

The first consideration is that your method:

private Set<JiraTicket> getJiraTicketsAsync(JsonArray ticketsArray)

Is not asynchronous at all.

As it returns a Set of JiraTicket when you call it you wait until the collection is completed.

So the method name is not correct, and could drive to missunderanding.

On your code:

ExecutorService service = Executors.newCachedThreadPool();
List<Future<JiraTicket>> futures = service.invokeAll(tasks);
for(Future<JiraTicket> future: futures){
    try
    {
        tickets.add(future.get());
    } catch (ExecutionException e)
    {
        logger.error("There was an error getting a jira ticket", e);
    }
}
service.shutdown();

The problem here is the for loop where you consume the Future object.
Of course you parallelize the requests on multiple threads, so you will probably gain a little bit of efficiency, but your code is still blocking.

The way to write concurrent code is not trivial, you have to break up your flow and rething it starting on dependencies.

You need JiraTicket in order to get infos from Github. In that way you should think your task like so:

Get the user JiraTicket
Get the infos from Github
Go to the next JiraTicket

At the end of this concurrent tasks you could do the sequencing tasks like create a Developer object and so on.

In a comment I suggest you to use a queue instead of use any sort of collection.
The reason is that a collection need to be synchronized and for that reason will stuck you on blocking code.

If you need to speed up and use advantage of concurrency, you should avoid blocking code.

A queue decouple the producer part, where you call the Jira and Github servers, from the consumer part, where you create the Developer object and complete the report.

The advantage in your case depends on the magnitude of the number of object you're dealing with, and the time your flow is using now.

There are many libraries that could help you to achieve the asyncronous part, such JMS, Reactive Streams or design pattern like using Publisher / Subscriber pattern.


Deserialize an email header into key-value pairs I've created a function that will deserialize an email header into a list of key-value pairs. I've run numerous tests using MS Office Outlook 2010 and MS Office 14.0 Object Library, all of which were successful.

However, I'd still like to know if this function could be optimized/improved.

Note

I didn't use the Dictionary(Of TKey, TValue) class because a key can appear multiple times.
The reason as to why I wrote this RegEx pattern is because a value can also contain a new line and/or :.

Public Shared Function Deserialize(header As String) As List(Of KeyValuePair(Of String, String))

    Const pattern As String = "(\n|^)[a-z|A-Z|-]*:\s"

    If (header Is Nothing) Then Throw New ArgumentNullException("header")

    Dim result As New List(Of KeyValuePair(Of String, String))
    Dim matches As MatchCollection = Regex.Matches(header, pattern)

    If ((Not matches Is Nothing) AndAlso (matches.Count > 0)) Then

        Dim endIndex As Integer = header.Length
        Dim startIndex As Integer = Nothing
        Dim item As Match = Nothing
        Dim key As String = Nothing
        Dim value As String = Nothing

        For index As Integer = (matches.Count - 1) To 0 Step -1
            item = matches.Item(index)
            startIndex = (item.Index + item.Length)
            key = item.Value.Trim().Replace(":", "")
            value = header.Substring(startIndex, (endIndex - startIndex))
            result.Insert(0, New KeyValuePair(Of String, String)(key, value))
            endIndex = item.Index
        Next

    End If

    Return result

End Function

There are a couple of things that I find odd about this code. This is the first thing that jumps out at me.

Dim startIndex As Integer = Nothing
Dim item As Match = Nothing
Dim key As String = Nothing

I see no purpose in initializing variable to Nothing. Simply declaring them would be cleaner in my opinion.

Dim startIndex As Integer
Dim item as Match
Dim key As String

I also find your For loop a bit strange. Is there a reason you're stepping backwards? If there is, I don't see it and you should comment on why in the code.

For index As Integer = (matches.Count - 1) To 0 Step -1

Personally, I would rewrite this to loop forward like this.

For index As Integer = 0 To matches.Count - 1
    '...
Next index

You could also use a comment here.

Const pattern As String = "(\n|^)[a-z|A-Z|-]*:\s"

Regular Expression patterns are rarely obvious. Tell Mr. Maintainer in English what you're matching on.


GLSL Shader that displays a Rotating Rainbow Ring I'm learning GLSL shaders on Book of Shaders and I decided to mess around with one of the examples. I wanted to see, whats a more efficient way to calculate to mask out the color?

#ifdef GL_ES
precision mediump float;
#endif

#define TWO_PI 6.28318530718

uniform vec2 u_resolution;
uniform float u_time;

//  Function from Iñigo Quiles
//  https://www.shadertoy.com/view/MsS3Wc
vec3 hsb2rgb( in vec3 c ){
    vec3 rgb = clamp(abs(mod(c.x*6.0+vec3(0.0,4.0,2.0),6.0)-3.0)-1.0,0.0,1.0);
    rgb = rgb*rgb*(3.0-2.0*rgb);
    return c.z * mix( vec3(1.0), rgb, c.y);
}

void main(){
    vec2 st = gl_FragCoord.xy/u_resolution;
    vec3 color = vec3(0.0);

    // Use polar coordinates instead of cartesian
    vec2 toCenter = vec2(0.5)-st;

    float angle = atan(toCenter.y,toCenter.x);
    float radius = length(toCenter)*2.0;

    angle = angle + u_time;

    float outsideMask = 1.0 - step(distance(st, vec2(0.5)), 0.3);
    float insideMask = 1.0 - step(0.2, distance(st, vec2(0.5)));
    float visibleArea = 1.0 - insideMask - outsideMask;

    // Map the angle (-PI to PI) to the Hue (from 0 to 1)
    // and the Saturation to the radius
    color = hsb2rgb(vec3((angle/TWO_PI)+0.5,radius, 1.0));

    gl_FragColor = vec4(color, visibleArea);
}

Here is an image of the end result.

The outsideMask, insideMask and visibleArea variables in particular all have that 1.0 minus something, which seems weird to me. Any suggestions would be appreciated!

Distance

In both your step(...) functions, you are computing distance(st, vec2(0.5)).  If you computed this once, and stored this in a local variable, you'd be able to reuse this computed value, which should save time in your shader.

But ... wait a second ...

vec2 toCenter = vec2(0.5)-st;
float radius = length(toCenter)*2.0;

You've already computed this distance (length, actually), and stored it (doubled) in the local variable radius.  We can use this value directly, by doubling your step thresholds:

float outsideMask = 1.0 - step(radius, 0.6);
float insideMask = 1.0 - step(0.4, radius);
float visibleArea = 1.0 - insideMask - outsideMask;

Or don't multiply the computed radius by 2.0 for the radius variable, and multiply it by 2 when you use it to compute the colour:

float radius = length(toCenter);

float outsideMask = 1.0 - step(radius, 0.3);
float insideMask = 1.0 - step(0.2, radius);
float visibleArea = 1.0 - insideMask - outsideMask;

color = hsb2rgb(vec3((angle/TWO_PI)+0.5, radius*2.0, 1.0));

visibleArea

outsideMask = 1.0 - step(radius, 0.3)
insideMask = 1.0 - step(0.2, radius)
visibleArea = 1.0 - insideMask - outsideMask

Substituting insideMask into (3):

visibleArea = 1.0 - (1.0 - step(0.2, radius)) - outsideMask
visibleArea = 1.0 - 1.0 + step(0.2, radius) - outsideMask
visibleArea = step(0.2, radius) - outsideMask

From step(A,B) = 1 - step(B,A), we can reworking (1) as:

outsideMask = step(0.3, radius)

And substituting into (6):

visibleArea = step(0.2, radius) - step(0.3, radius)


RFC: Lightweight C++ 2D array template with runtime dinemsions Any comments, critiques or questions about this? I'm using this as the base for a few rectangular board based toy programs like Conway's life and snake. Usage includes configuration of board size at runtime, hence the type doesn't carry size info, only the instances.

#include <utility>

template<typename type>
class Array2D {
   int y;
   type *data;
public:
   // We can't do a copy since we don't know
   // what size to allocate the array.
   Array2D(const Array2D&) = delete;
   Array2D& operator=(const Array2D&) = delete;

   Array2D(Array2D&& move) noexcept :
      y(std::exchange(move.y, 0)),
      data(std::exchange(move.data, nullptr)) {}

   Array2D& operator=(Array2D&& move) noexcept {
      std::swap(y, move.y);
      std::swap(data, move.data);
   }

   Array2D( int _x, int _y ) : y(_y) {
      data = new type[_x*y];
   }

   type const *operator[](int i) const {
      return data + (i * y);
   }

   type *operator[](int i) {
      return data + (i * y);
   }

   ~Array2D() {
      delete[] data;
   }
};

Overall
It's fine as far as it goes.
I don't see anything wrong with the implementation.
Note: Since you use none checked version of operator[] you should have the ability to validate that your inputs are correct. So your array needs the ability to return the dimensions of the array in both the x and y dimensions so the code can validate the inputs when required.
Next Step
Currently this is not a container.
There are a few more requirements to make this a standards compliant container. If you want to go this direction.
see: https://en.cppreference.com/w/cpp/named_req/Container
Some improvements:
User defined Types (and by extension template types) should always have an initial capitol letter. This is for humans so we can easily distiguish types from objects:
template<typename type>

I don't like this as now type looks like an object to me when I read it in the code.
Put the destructor next to the constructors.
When code reviewing I want to make sure that your destructor releases the resources correctly as I read. Not scroll back when I find the destructor at the bottom. Also they logically group together.
Why is none copy?
I don't see the logic in that.
Add a swap method (noexcept) and swap free standing function. That comes in handy when interacting with the standard library functions.
It would be nice to simply loop over an array:
Array2D<int>   data = get2DArray();

for(auto const& val: data) {
    std::cout << val << " ";
}

The ability to extract rows to loop over would also be nice (this is an alternative to the above).
for(auto const& row: data) {
    for(auto const& val: row) {
        std::cout << val << " ";
    }
    std::cout << "\n";
}

You have the none checked operator[] implemented fine. But sometimes it is nice to have checked versions so add the method at() that does the same thing but validates its input first.
This is very C like type const *object The * placed next to the object is a standard C convention but in C++ the convention is the opposite and (by tradition) you put the * next to the type rather than the objet (as it is part of the type information).


Counting number of eights I was doing a few basic Java recursion problems to refresh my memory with recursion (honestly never had to use it in a long, LONG, time), along with using it as a useful way to prepare for any possible interviews for entry level positions. I came upon this problem on codingbat.com:

  Given a non-negative int n, compute recursively (no loops) the count
  of the occurrences of 8 as a digit, except that an 8 with another 8
  immediately to its left counts double, so 8818 yields 4. Note that mod
  (%) by 10 yields the rightmost digit (126 % 10 is 6), while divide (/)
  by 10 removes the rightmost digit (126 / 10 is 12).

  count8(8) → 1
  count8(818) → 2
  count8(8818) → 4

My solution, which was all correct for all the test cases they had is as follows:

public int count8(int n) {
  if(n == 0) {
    return 0;
  } else {

    int count = (n % 10 == 8)? 1: 0;

    //This is testing for adjacent 8s.
    if(count == 1) {
      int tempN = n;
      tempN = tempN / 10;
      count = (tempN % 10 == 8) ? 2 : 1;
    }

    n = n/10;
    return count + count8(n);
  }
}

I was wondering if anyone could critique this solution, and let me know if this is the best approach, or if there is a better way. I am not a big fan of the if(count == 1) that I created, but that was the only way I could think of to test the presence of an 8 to the left of the current one.

Another approach is to let the recursive method know if the previous digit was an eight.

public int count8(int n) {
  return count8(n,false)
}

public int count8(int n, boolean lastDigitWasMatch) {
  if(n == 0) {
    return 0;
  } else {

    boolean match = (n % 10 == 8);
    int count = match?(lastDigitWasMatch?2:1):0;
    return count+count8(n/10,match);
  }
}


Transforming XML with null checks vs variables I'm working on an Spring Integration application that transforms a XML (converted by Jaxb) into a custom Object by checking various information given in the XML.

The problem is that I can't trust the data that has been sent to me and I have to do null checks everywhere. Can't decide if storing the result of a null check is better or is there another solution?

Solution 1: Do the null checks everywhere, and when we call another method, do the same null checks when needed. Which I dislike very much.

public void mapSpecificField(XmlObject xmlObject) {

    if (xmlObject != null) {
        if (xmlObject.getName() != null) {
            if (xmlObject.getName().equals("TYPE_A")) {
                if (xmlObject.getIdentifiers() != null
                        && !xmlObject.getIdentifiers().isEmpty()
                        && xmlObject.getIdentifiers().get(0) != null) {
                    String mainId = xmlObject.getIdentifiers().get(0).toLowerCase();

                    if (xmlObject.getXmlSubObjects() != null) {
                        for (XmlSubObject subObject : xmlObject.getXmlSubObjects()) {
                            if (subObject != null && subObject.getLocation() != null) {
                                mapOtherStuff(subObject);
                            }
                        }
                    }
                }
            } else {
                if (xmlObject.getIdentifiers() != null
                        && !xmlObject.getIdentifiers().isEmpty()
                        && xmlObject.getIdentifiers().get(0) != null) {
                    String mainId = xmlObject.getIdentifiers().get(0).toUpperCase();
                }
            }
        }
    }
}

Solution 2: Do the null checks in an inner class and store the result in those variables. Which I like but not sure if memory wise it's a smart thing to do...

The inner helper class:

static class Helper {
    String type = null;
    String mainId = null;

    public Helper(XmlObject xmlObject) {
        if (xmlObject != null) {
            // Common
            if (xmlObject.getName() != null) {
                type = xmlObject.getName();
                // Specific mapping
                if (xmlObject.getIdentifiers() != null && !xmlObject.getIdentifiers().isEmpty() && xmlObject.getIdentifiers().get(0) != null) {
                    if (type.equals("TYPE_A")) {
                        mainId = xmlObject.getIdentifiers().get(0).toLowerCase();
                    } else {
                        mainId = xmlObject.getIdentifiers().get(0).toUpperCase();
                    }
                }
            }
        }
    }

And the previous class using the helper:

public void mapSpecificField(XmlObject xmlObject) {
    Helper helper = new Helper(xmlObject);

    String mainId = helper.mainId;

    if ("TYPE_A".equals(helper.type)) {
        if (xmlObject.getXmlSubObjects() != null) {
            for (XmlSubObject subObject : xmlObject.getXmlSubObjects()) {
                if (subObject != null && subObject.getLocation() != null) {
                    mapOtherStuff(subObject);
                }
            }
        }
    }
}

Or if anyone has a Solution 3, I'd be up for it as I really need some opinions...

Please do note that my original class has about 8 different types & lots of fields to map and I'm splitting the mapping into few methods that sometimes need the same variables. I can't share it here as it's pretty long but the classes above show exactly what it looks like.

I'd definitely go for the second version. If you are not in a very specific limited environment, the additional memory won't hurt. Note that you basically just copy the references to the concrete content, not the content itself, so the whole memory-signature-thing essentially revolves around a few 4-byte pointers.

As for the code itself: one way of coping with these long if a != null && a.getB() != null && a.getB().getC() != null chains is, to use Optional and its map function. In the end it's all matters of taste, but you might e.g. do:

// inner if from last code sample
Optional.ofNullable(subObject)
    .map(XmlSubObject::getLocation)
    .ifPresent(this::mapOtherStuff);

I like this style very much, but seriously: matters of taste.

One more thing: in similar situations I found it really helpful to include static utility functions which map from null to empty collection/list and vice versa:

public static <T> List<T> emptyListIfNull(List<T> l) {
  if(l == null)
     return new ArrayList<T>(); // or Collections.emptyList() if immutable is OK
  return l;
}

public static <T> List<T> nullIfEmpty(List<T> l) {
   if(l != null && !l.isEmpty())
       return l;
   return null;
}

That way, you can shorten expressions like

xmlObject.getIdentifiers() != null && !xmlObject.getIdentifiers().isEmpty()

to

nullIfEmpty(xmlObject.getIdentifiers()) != null

... which helps a lot in readability.

Edit: one more afterthought. As bowzerfood mentioned nulls and equals in his answer, I'd like to point out additionally, that today you can use Objects.equals(a, b) instead of a.equals(b) which handles nulls correctly and without resorting to third-party libraries.


Singly Linked List Implementation (With Nodes) This code was recommended to me by my dear friend Jon after I wrote an implementation using arrays. With his help, I managed to create a Singly linked List similar to std::vector using Nodes.

#include <iostream>

template <class T>
class LinkedList
{
private:
    size_t _size;
public:

    class Node
    {
    public:
        Node* next;
        T value;
        Node(T val) : value(val), next(NULL) {}
    };

    Node* head;

    LinkedList() : _size(0), head(NULL) {}

    ~LinkedList() {
        std::cout << "Linked List Deleted!" << std::endl;
    }

    void push(T val) {
        Node* node = new Node(val);
        node->next = head;
        head = node;
        ++_size;
    }
    size_t size() {
        return _size;
    }
    bool empty() const {
        return head == NULL;
    }
    T pop() {
        Node* n = head;
        if (n == NULL) return NULL;
        head = n->next;
        _size--;
        return n->value;

    }
};

int main() {
    LinkedList<int> list;
    std::cout << "Empty: " << list.empty() << std::endl;
    list.push(5);
    list.push(6);
    list.push(7);
    list.push(8);
    list.push(9);
    list.push(10);
    std::cout << "Empty: " << list.empty() << std::endl;

    std::cout << "Size: " << list.size() << std::endl;
    list.pop();

    std::cout << "Size: " << list.size() << std::endl;
    LinkedList<int>::Node* n = list.head;
    while (n != NULL) {
        std::cout << n->value << std::endl;
        n = n->next;
    }
}

I recommend you compile with more warnings enabled.  That would alert you to these problems:

g++ -std=c++11 -fPIC -g -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds  -Weffc++       208038.cpp    -o 208038
208038.cpp: In instantiation of ‘class LinkedList<int>’:
208038.cpp:50:21:   required from here
208038.cpp:4:7: warning: ‘class LinkedList<int>’ has pointer data members [-Weffc++]
 class LinkedList
       ^~~~~~~~~~
208038.cpp:4:7: warning:   but does not override ‘LinkedList<int>(const LinkedList<int>&)’ [-Weffc++]
208038.cpp:4:7: warning:   or ‘operator=(const LinkedList<int>&)’ [-Weffc++]
208038.cpp: In instantiation of ‘T LinkedList<T>::pop() [with T = int]’:
208038.cpp:61:14:   required from here
208038.cpp:40:31: warning: converting to non-pointer type ‘int’ from NULL [-Wconversion-null]
         if (n == NULL) return NULL;
                               ^~~~
208038.cpp: In instantiation of ‘LinkedList<T>::Node::Node(T) [with T = int]’:
208038.cpp:27:22:   required from ‘void LinkedList<T>::push(T) [with T = int]’
208038.cpp:52:16:   required from here
208038.cpp:14:11: warning: ‘LinkedList<int>::Node::value’ will be initialized after [-Wreorder]
         T value;
           ^~~~~
208038.cpp:13:15: warning:   ‘LinkedList<int>::Node* LinkedList<int>::Node::next’ [-Wreorder]
         Node* next;
               ^~~~
208038.cpp:15:9: warning:   when initialized here [-Wreorder]
         Node(T val) : value(val), next(NULL) {}
         ^~~~

The lack of consideration of copy/move behaviour is particularly concerning, given that we have a pointer which we own.  The destructor is also faulty, failing to release the allocated resource:

valgrind -q --leak-check=full ./208038
Empty: 1
Empty: 0
Size: 6
Size: 5
9
8
7
6
5
Linked List Deleted!
==18193== 96 (16 direct, 80 indirect) bytes in 1 blocks are definitely lost in loss record 6 of 6
==18193==    at 0x4835E2F: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18193==    by 0x1094BD: LinkedList<int>::push(int) (208038.cpp:27)
==18193==    by 0x109276: main (208038.cpp:57)
==18193==

Remember the guideline - whenever you write new or new[], make sure you know where the corresponding delete or delete[] is!  (Don't forget that you need a delete in the pop() method, too).

There's no need for Node to be a class:

struct Node
{
    T value;
    Node* next;
};

void push(T val) {
    head = new Node{std::move(val), head};
    ++_size;
}

I think it would be better if it didn't need to be public, either - there's nothing to stop broken code messing up the guts of the list.  We'd like a way to iterate through the values of the list without having to understand its innards, and without being able to accidentally change it (potentially breaking the size invariant).

Finally, std::size_t is written incorrectly throughout, and is missing its header (<iostream> is not one of the headers specified to define this type).


Strict types decorator (works only with Python 3.5) I wrote a decorator which makes Python 3.5 raise exceptions if the arguments that are passed to a type-hinted function are of the wrong type.

from typing import get_type_hints

def strict_types(f):
    def type_checker(*args, **kwargs):
        hints = get_type_hints(f)

        all_args = kwargs.copy()
        all_args.update(dict(zip(f.__code__.co_varnames, args)))

        for key in all_args:
            if key in hints:
                if type(all_args[key]) != hints[key]:
                    raise Exception('Type of {} is {} and not {}'.format(key, type(all_args[key]), hints[key]))

        res = f(*args, **kwargs)

        if type(res) == hints['return']:
            return res
        else:
            raise Exception('Type of result is {} and not {}'.format(type(res), hints['return']))

    return type_checker

It's used like this:

@strict_types
def concatenate_with_spam(text: str) -> str:
    return text + 'spam'

and if something of the wrong type is passed to the decorated function e.g. concatenate_with_spam(42) an exception is raised:

Exception: Type of text is <class 'int'> and not <class 'str'>

The part that I'm worried about is the part that checks the result. It calls the original function (to learn result of what type it returns), but returns a wrapped function type_checker that will be called one more time, so the runtime of function is (at least) multiplied by 2. Are there any other things I have missed? What can be improved here (except for not using this not-pythonic decorator and crucifying me for writing it)?

You should change this part a bit:

    for key in all_args:
        if key in hints:
            if type(all_args[key]) != hints[key]:
                raise Exception('Type of {} is {} and not {}'.format(key, type(all_args[key]), hints[key]))

Here, you are silently ignoring missing type hints, but if the user went as far as to use this decorator, you should raise an exception instead, like this:

    for key in all_args:
        try:
            if type(all_args[key]) != hints[key]:
                raise Exception('Type of {} is {} and not {}'.format(key, type(all_args[key]), hints[key]))
        except IndexError:
                raise TypeError('The formal parameter {} was not given a type'.format(key))

Specific Errors & Exceptions

Exception is hilariously un-informative, use TypeError to convey more information.

Order of conditionals

    if type(res) == hints['return']:
        return res
    else:
        raise Exception('Type of result is {} and not {}'.format(type(res), hints['return']))

Here, the real logic is in the else, but I prefer seeing the main logic after the if, like this:

if type(res) != hints['return']:
    raise Exception('Type of result is {} and not {}'.format(type(res), hints['return']))
return res

Now it looks more like a simple perturbation of an almost linear function, than a full-blown logic statement.

Please do not abbreviate

res -> result
func -> function

It takes 0.5 seconds more to type and a tenth of the time to read.


Python code for handling HTTP cookies We would like for a code review for our Python code. The code is written in Python 3.7 and is part of a class handling HTTP cookies.

The code:

class CookieJar(object):
    def add_cookie_header(self, request):
        w_r = WrappedRequest(request)
        self.policy._now = self.jar._now = int(time.time())

        # the cookiejar implementation iterates through all domains
        # instead we restrict to potential matches on the domain
        req_host = request.hostname
        if not req_host:
            return

        if not IPV4_RE.search(req_host):
            hosts = potential_domain_matches(req_host)
            if '.' not in req_host:
                l_req_host = "%s%s" % (req_host, '.local')
                hosts += [l_req_host]
        else:
            hosts = [req_host]

        Cookies = []
        for hosti in range(len(hosts)):
            if hosts[hosti] in self.jar._cookies:
                Cookies.extend(self.jar._cookies_for_domain(hosts[hosti], w_r))

        ats = self.jar._cookie_attrs(Cookies)
        if ats:
            if w_r.has_header("Cookie") == False:
                w_r.add_unredirected_header("Cookie", "; ".join(ats))

        self.processed = self.processed + 1
        if self.processed % self.check_expired_frequency == 0:
            # This is still quite inefficient for large number of Cookies
            self.jar.clear_expired_cookies()

The class CookieJar has other fields and methods, obviously, but they shouldn't be reviewed, so they're not provided here.
You can assume all required imports are present at the top of the file and all referenced methods and fields do exist.

How can we make our code more elegant, efficient and maintainable?

Since you are not showing a whole lot of the class itself, there is not to much fundamental to be done on the presented code.

Documentation

First, you should add documentation to your class and to its methods. Use the """...""" to write those docstrings as recommended in the official Style Guide.

Style

The Style Guide also has a section on variable naming (snake_case is recommended). You're mostly following this convention, and that's also the reason why Cookies as variable name almost immediately caught my attention while first screening the code. The widely accepted convention is to only use names with the first letter capitalized for class names.

Looping

While I was at it, the following piece of code caught my attention next.

Cookies = []
for hosti in range(len(hosts)):
    if hosts[hosti] in self.jar._cookies:
        Cookies.extend(self.jar._cookies_for_domain(hosts[hosti], w_r))

Since hosts is a list, and you're not trying to do anything fancy on the elements themselves, this can be simplified to

cookies = []
for host in hosts:
    if host in self.jar._cookies:
        cookies.extend(self.jar._cookies_for_domain(host, w_r))

which I find easier to read.

String formatting

This snippet comes a little bit earlier in the code: l_req_host = "%s%s" % (req_host, '.local'). It uses the old-old Python string formatting using %. Since you specifically tagged your question as Python 3, I strongly recommend to make use of the new f-string synthax which allows you to write that same line as l_req_host = f"{req_host}.local". Here is a nice blog post talking about all the different ways of string formatting in Python and their pros and cons.

if statements

Next up on my list would be if w_r.has_header("Cookie") == False. In general there is no need to do if sth == True or if sth == False in Python. Both can be conveniently expressed as if sth or if not sth. Usually this will also allow someone to read the expression almost as if it was text. If that doesn't work, maybe think about the name of your bool variable.

Minor tweaks

At the moment the last thing I could come up with was to change self.processed = self.processed + 1 to self.processed += + 1 which saves you a few characters of typing in the future.


Generic pass_many(vals, pass_one) to unroll and call pass_one(val) I'm trying to create a generic function that takes a data structure as input and then sends each non-empty item—e.g.: str, int—individually:

def print_this(v, delim1="", delim2=""):
    print v, delim1, delim2

def pass_many(vals, some_funct, args=None):
    if vals:
        if type(vals) == list or type(vals) == tuple:
            for v in vals:
                if v:
                    pass_many(v, some_funct, args)
        else:
            if args:
                some_funct(vals, *args)
            else:
                some_funct(vals)

>>> pass_many([['foo'],['',(5,3)]], print_this, ["newline","notnewline"])
foo newline notnewline
5 newline notnewline
3 newline notnewline

My current implementation works reasonably well, but feels hacky...

How could I generalise it to unroll all data-structures (except strings), and make it less hacky?

I'd make a few changes:

1) Take the if v check out of the inner loop into the function body (if not vals: return) - seems more consistent that way, unless you want pass_many(0,print) to print something but pass_may([0], print) not to?

2) I'm not sure there's any reason to use args=None and then check for it being None, rather than just doing args=() to start with, and the implementation's shorter that way.

3) There are definitely better ways to check whether something is a tuple/list/set/etc than using type.  Unfortunately none of them are perfect... I really wish there was a good clean way of distinguishing these things from strings and dictionaries.
Probably he most general thing I can come up with, using Abstract Base Classes from the collections module - it goes over all elements of iterable objects except for mappings (dictionaries etc - unless you want to include those?) and strings (I think the basic string type/s may be different in python3? Not sure.)

from collections import Iterable, Mapping

def pass_many(vals, some_funct, args=()):
    if not vals:
        return
    if isinstance(vals, Iterable) and not isinstance(vals, (basestring, Mapping)):
        for v in vals:
            pass_many(v, some_funct, args)
    else:
        some_funct(vals, *args)

Still, the ABCs aren't perfect - maybe using hasattr checks for whether something is an iterable or a mapping would be better. Not sure. Probably every possible way will have some downsides.

If you want to be more stringent and just accept plain lists and tuples and no other iterables, isinstance(vals, (list, tuple)) will be fine. It just won't work on things like iterators - whether that matters depends on the intended use.