Turning a Java console app into a server I'm not familiar with Java at all, so I'd like to get some feedback on this piece of code.

I made it by looking at a few Hello World-esque Java server examples, so it may have drawbacks I'm not aware of.

My task was turning an existing Java console app into a single-threaded server listening on some port. Each connection should “launch” what previously was the console app, where actual “arguments” are passed by the client. Anything that was written by console app to stdout, should go into process' stdout; but what was previously written to stderrm needs to be redirected to the client. Finally, when the processing has finished, if there was no error, the client should receive "OK" and get disconnected. (And the next client should be processed.)

Here's the original and my version side by side:

// Change: we want to run this as a server instead

// public static void main(String [] args) {
//     (new Main(args)).execute();
//     System.exit(0);
// }

public static void main(String args[]) throws Exception {
    int port = Integer.parseInt(args[0]);
    ServerSocket welcomeSocket = new ServerSocket(port);

    System.out.format("Listening on %d\n", port);
    PrintStream defaultStderr = System.err;

    while (true) {
        // Establish a connection
        Socket connectionSocket = welcomeSocket.accept();
        BufferedReader input = new BufferedReader(new InputStreamReader(connectionSocket.getInputStream()));
        DataOutputStream output = new DataOutputStream(connectionSocket.getOutputStream());

        // Hook System.err
        System.setErr(new PrintStream(output));

        try {
            // Run Batik as if we ran console app, but without the launch overhead
            (new Main(input.readLine().split(" "))).execute();
        } catch (Throwable t) {
            // Normally, we shouldn't get here (as Batik should not crash).
            // If we *do* get here, print to stderr, which we redirect to output stream anyway.
            t.printStackTrace();
        }

        // Let the client know how it went
        if (output.size() == 0) {
            output.writeBytes("OK");
        }

        output.close();

        // Restore stderr
        System.setErr(defaultStderr);
    }
}

What can I make better? I want to clarify I do not want to change any other method in the app to keep it easy to merge upstream changes.

General:

Commandline handling in any application is a PITA.... My recommendation is that you change both the client side, and server side, to put each argument on a different line so that you don't have to 'split' it on the Server. This would be relatively simple to do.

OOP:

This is a 'for future improvement' thing. It is a common pattern in Java for servers to handle different 'clients' in different threads. It is really easy if you have the basic infrastructure in place. Simply creating a class called "ClientManager" or something is a start, and having the following:

class ClientManager {
    private final Socket socket;
    ClientManager(Socket socket) {
        this.socket = socket;
    }

    public void processSocket() {

        // do all your things for the socket in here.

    }
}

With this simple class, your Java server becomes:

while (true) {
    // Establish a connection
    ClientManager client = new ClientManager(welcomeSocket.accept());
    client.processSocket();
}

This has some advantages. The first is that it becomes easier to separate Server problems from client problems, and the second is that the client.processSocket() can be easily moved off to any thread we want to use.

Right now, using a single thread is probably fine, but, the Error handling would be much better with this distinction. Consider the following:

try {
    while (true) {
        // Establish a connection
        ClientManager client = new ClientManager(welcomeSocket.accept());
        try {
            client.processSocket();
        } catch (Exception e) {
            // this is unusual to catch `Exception` but it is done intentionally!
            // Any exception in the client code is specific to **that** client.
            // Clients should be better behaved and manage their own exceptions
            // but no client is perfect.
            // Log the issue, and wait for the next customer.
            e.printStackTrace();
        }
    }
} catch (IOException serverexception) {
    // this is an exception in the **server** code, and thus the server dies:
    serverexception.printStackTrace();
}

OK, so we have a Server that is well isolated from the client code it runs.

The client handler

The client code is handled by a ClientManager class. What happens in there.... this is a copy/paste of the relevant code:

    BufferedReader input = new BufferedReader(new InputStreamReader(connectionSocket.getInputStream()));
    DataOutputStream output = new DataOutputStream(connectionSocket.getOutputStream());

    // Hook System.err
    System.setErr(new PrintStream(output));

    try {
        // Run Batik as if we ran console app, but without the launch overhead
        (new Main(input.readLine().split(" "))).execute();
    } catch (Throwable t) {
        // Normally, we shouldn't get here (as Batik should not crash).
        // If we *do* get here, print to stderr, which we redirect to output stream anyway.
        t.printStackTrace();
    }

    // Let the client know how it went
    if (output.size() == 0) {
        output.writeBytes("OK");
    }

    output.close();

    // Restore stderr
    System.setErr(defaultStderr);

Now, this client/server setup is simple. It is a call-response type only, with a 'success' indicated by 'OK'. Anything else is a problem.

The protocol can be set up that the client pushes the arguments, and then closes that stream.... and then waits for results. Closing the stream has the advantage that we can read the arguments nicely (wait for end-of-stream):

List<String> arguments = new ArrayList<>();
String line = null;
while ((line = input.readLine()) != null) {
    arguments.add(line);
}

Let's introduce the Java7 try-with-resources system. This has a number of advantages, but, in this case it makes sure that, if there's a problem, the client side of the process knows immediately instead of hanging around waiting for a socket to die (get garbage collected).

Note, never catch Throwable.... it will catch all sorts of things you don't want to, like ThreadKillError, etc.

First up, we manage the input/output streams:

PrintStream defaultStderr = System.err;
try (  BufferedReader input = new BufferedReader(
           new InputStreamReader(socket.getInputStream()));
       DataOutputStream output = new DataOutputStream(socket.getOutputStream());
       PrintStream stderr = new PrintStream(output);
) {
    // at this point, the input/output is guaranteed to close
    List<String> arguments = new ArrayList<>();
    String line = null;
    while ((line = input.readLine()) != null) {
        arguments.add(line);
    }

    // Hook System.err
    System.setErr(stderr);

    try {
        // Run Batik as if we ran console app, but without the launch overhead
        Main batik - new Main(arguments.toArray(new String[arguments.size]));
        batik.execute();
    } catch (Exception t) {
        // Normally, we shouldn't get here (as Batik should not crash).
        // If we *do* get here, print to stderr, which we redirect to output stream anyway.
        t.printStackTrace();
    }

    // Let the client know how it went
    if (output.size() == 0) {
        output.writeBytes("OK");
    }
    output.flush();
} finally {
    // note, we close the socket explicitly when we are done.
    // even if the input and output are closed, you should still close the socket.
    this.socket.close();

    // Restore stderr
    System.setErr(defaultStderr);
}

note that this handling of System.err is a problem.... you will never be able to multi-thread until you fix this.

All the best, I hope this makes sense to you.


dequeue() in Queue implememtation that uses a circular linked list I use a circular linked list to implement a queue, and it only holds one last (note that last.next links to the first, not null).

public class Queue<T> {

    private Node last;
    private int n;

    private class Node {
        T value;
        Node next;
        Node(T t, Node n) {
            value = t;
            next = n;
        }
    }

    public Queue() {
        last = null;
        n = 0;
    }

    public boolean isEmpty() {
        return n == 0;
    }

    public void enqueue(T item) {
        if (isEmpty()) {
            last = new Node(item, null);
            last.next = last;
        } else {
            Node tmp = new Node(item, last.next);
            last.next = tmp;
            last = tmp;
        }
        n++;
    }

   // other code
}

I think the correct way of dequeue() is :

public T dequeue() {
    if (isEmpty()) throw new NoSuchElementException();
    Node first = last.next;
    T t = first.value;
    if (last == first) {
        last = null;
    } else {
        last.next = first.next;
    }
    n--;
    return t;
}

This is because we should set last to null if there is one element.

However, the alternative way in following also works, why?

public T dequeue2() {
    if (isEmpty()) throw new NoSuchElementException();
    Node first = last.next;
    T t = first.value;
    last.next = first.next;
    n--;
    return t;
}

I call N times dequeue2() after N times enqueue(), and assert queue.last== null. To my surprize, the assertion passes. I think the last can never be null if I use dequeue2().
So, is the dequeue2() correct?

dequeue2

This method works because, when there's just one remaining item in the queue, the line:

Node first = last.next;

sets first to be the same as last (because, for a 1-sized queue last.next is a pointer to last).

In the other dequeue() you set last to be null when there's nothing left, but this does not make a difference, really (other than Garbage Collection) because the enqueue function checks the empty queue using n == 0 and not last == null

static inner class

Your Node class does not have any need for a back-reference to the Queue. It should be a static inner class private static class Node....... To do that, though, you need to make it generic too. Use a letter other than T to represent the generic class...

 private static class Node<U> {
    U value;
    Node<U> next;
    Node(U val, Node<U> n) {
        value = val;
        next = n;
    }
}

This saves a small amount of memory, but it is good practice. When you create the Node instances now, the code will change from:

private Node last;

to

private Node<T> last;

and you will add the <> diamond operator here:

Node<T> tmp = new Node<>(item, last.next);

Constructor:

Your constructor:

public Queue() {
    last = null;
    n = 0;
}

should just be:

public Queue() {
}

as you don't need to explicitly supply the default values.

General

Your code is neat, your variable names are not bad, and the use of generics is good.

As a learning exercise you should spend some time seeing if you can make it implement java.util.Queue interface (it's not easy - fair warning) because that will teach you a bit about what the actual Java Collections API offers.


JavaScript - Optimizing a two array.map loop I have to iterate two arrays, if the iso propery of the first array is equal to address.country of the second array condition is verified, assign address and slug of the second array (this.concessions) to the first array (this.countries).

at the end, you need to have a new this.countries array that contains theaddress and slug property (in addition to the properties he already had)

this.countries.map((element) => {
  this.concessions.map((value) => {
    if (element.iso === value.address.country) {
      element.address = value.address
      element.slug = value.slug
    }
  })
})

How can I optimize this, and for this case what is the best iterable to use, for ..of for example ?

That'd be with map:

this.countries = this.countries.map(country => {
    var concession = concessions.find(x => x.address.country === country.iso);
    country.address = concession.address;
    country.slug = concession.slug;
    return country;
});

That'd be with forEach:

this.countries.forEach(country => {
    var concession = concessions.find(x => x.address.country === country.iso);
    country.address = concession.address;
    country.slug = concession.slug;
});

I don't know if you can be sure there always be a concession for each country and how you should proceed in case there isn't.


Determine numeric month based on actual moth The following script uses a switch statement to determine the numeric month based on the actual month. Is there a more succinct way (read: elegant, or less time to write) to accomplish the same result? Is there a down-side or up-side to either approach?

jsFiddle

var month = "May";
            var m;
            switch(month) {
                case "January":
                    m = 0;
                    break;
                case "February":
                    m = 1;
                    break;
                case "March":
                    m = 2;
                    break;
                case "April":
                    m = 3;
                    break;
                case "May":
                    m = 4;
                    break;
                case "June":
                    m = 5;
                    break;
                case "July":
                    m = 6;
                    break;
                case "August":
                    m = 7;
                    break;
                case "September":
                    m = 8;
                    break;
                case "October":
                    m = 9;
                    break;
                case "November":
                    m = 10;
                    break;
                case "December":
                    m = 11;
                    break;
                default:
                //
            }
        console.log(m);

Just use an array.

var months = ["January", "February", "March", "April", "May", "June", "July", "August", "September", "October", "November", "December"];

var m = months.indexOf('May');  // 4


Validating that all strings in an array match a condition I want to validate a list of objects for example strings. and if one of the objects fails to pass the condition return false as validation result . this is the code I use :

    public static bool AreValid(string[] strs)
    {
        foreach (string str in strs)
        {
            if (str != condition )
            {
                return false; // does this breaks the for loop ?
                // break;     // no need to this
            }
        }

        return true;
    }

is this a correct approach ?

Yes, return immediately* returns from the method, no matter where in the method you are. You don't need the break.

But there is even easier way to write this code, using the LINQ method All():

strs.All(str => str == condition)

This also returns as soon as single non-matching element is found and is more readable.

* Actually, finally blocks run before you actually return from the method, but that's not relevant here.


Semantic markup for card blocks I'm using Bootstrap. I'm doing my best to work with more semantic markup and getting away from divitis.

Several of these "card" blocks appear on a page, like this.

Original:

Card Title

Lorem ipsum dolor sit amet, consectetur adipisicing elit. Reiciendis ipsam eos, nam perspiciatis natus commodi similique totam consectetur praesentium molestiae atque exercitationem ut consequuntur, sed eveniet, magni nostrum sint fuga.

          Learn More

Rewritten:

Card Title

Lorem ipsum dolor sit amet, consectetur adipisicing elit. Sapiente esse necessitatibus neque.

          Learn More

Could I improve the HTML?

200_success' comment is right, that the context and the content of the component is important for semantics. I assume, from the linked page, that you want to display a list of short articles that lead to more detailed information.

That being said. Let's look at your rewrite:

Outer section-element.

While a section might be fine, to me this looks more like a list of short articles. So I would use an article-element here.

  The article element represents a self-contained composition in a document, page, application, or site and that is, in principle, independently distributable or reusable, e.g. in syndication. This could be a forum post, a magazine or newspaper article, a blog entry, a user-submitted comment, an interactive widget or gadget, or any other independent item of content.

From w3.org 4.4.4 The article element

Also interesting on Stackoverflow: Semantic HTML of an articles list

header-element

If you only have a heading-element and no other content for the the header of a sectioning-element, you should drop the extra header-element, as:

  These elements [h1 … h6] represent headings for their sections.

From w3.org 4.3.6 The h1, h2, h3, h4, h5, and h6 elements

Nested section-element

The paragraph is the actual content of this item. Its context is not a standalone section, as implied, when wrapping it in another section-element:

  The HTML  element represents a standalone section

From MDN
: The Generic Section element

So drop that section as well.

footer-element

Using a footer-element here is debatable (as all other points as well). From the docs point of view it's absolutely fine:

  A footer typically contains information about its section such as who wrote it, links to related documents, copyright data, and the like.

From w3.org 4.4.9 The footer element

However, personally I would drop it for simplicity.

Finally we remove everything that is not needed for styling – in regard of your linked layout – and end up with this:

Card heading

Some teaser …

    Learn more …


A simple encryption program using Python I made this encryption program in Python.

I don't have much experience (or any experience at all) about encryption (since I just gave my 9th grade finals), but I had this idea about an algorithm some time back which would enable the user to encrypt words following an algorithm where the program would follow this process for each letter of the entered word; pseudocode:

Let the variable x be the position of the alphabet in the list of alphabets sorted alphabetically
Let the variable y be the position of the alphabet in the entered word
For example, if the user enters 'abcd', the program would find x and y for a, b, c and d one by one
Then, it would find the variable z = 26-x+y and z would be the position of the alphabet in the code
    in the list of alphabets
In abcd : for a - x = 1, y = 1, so, z = 26-1+1 = 26, so  coded alphabet = 26th alphabet = z
 Similarly, all of a,b,c and d will have coded alphabets as 'z'
 So, 'abcd' will be coded as 'zzzz'

Here's the Python code:

alphabets =['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']

# Defining all sorts of functions

def position(tofind,source):
    size = len(source)
    for i in range(0,size,1):
        if (source[i] == tofind):
            p = i + 1
            break
    return p
def encrypt(a):
    output = []
    sizea = len(a)
    for i in range(0,sizea,1):
        x = i+1
        y = position(a[i],alphabets)
        z = 26-x+y
        if (z>26):
            z = z % 26
        element = alphabets[z-1]
        output.append(element)
    return output
def converttolist(text):
    size = len(text)
    l = []
    for i in range(0,size,1):
        l.append(text[i])
    return l

# The main program

print ()
print ("NOTE : Please enter all the alphabets in lowercase...")
print ()
given = str(input("Please enter the word to be coded : "))
givenlist = converttolist(given)
outputlist = encrypt(givenlist)
print ()
print ("The code for ",given," is :-")
outputlistlength = len(outputlist)
for i in range(0,outputlistlength,1):
    print (outputlist[i],end = "")

Let me know what you think about it.

I think it's a nice project. I would say that the main things for you to work on is getting further acquainted with Python's standard library and with standard practices, which is what most of my advice will be surrounding.

Minor improvements

For your alphabet, you could use ascii_lowercase from string, i.e.:

from string import ascii_lowercase
alphabet = [character for character in ascii_lowercase]

Unless I'm misreading, your function position() looks like an attempt at recreating list.index(value) (or in your case source.index(tofind)).

"Unneeded" variables can sometimes make sense if they improve readability, but your function:

def converttolist(text):
    size = len(text)
    l = []
    for i in range(0,size,1):
        l.append(text[i])
    return l

would be just as readable if written as:

def converttolist(text):
    l = []
    for i in range(0,len(text),1):
        l.append(text[i])
    return l

and while we're on that particular function, I would strongly recommend having a look at list comprehension---it's both faster and cleaner. Your function would then become:

def convert_to_list(text: str) -> list:
    return [text[i] for i in range(len(txt)]

but I should add that, for cases like this, even better is to just use in-line built-ins like str.split() or [character for character in text].

You don't need to write str(input(<whatever>)) since input already returns a string.

The function range() defaults to step-size 1, so writing range(start, end, 1) is unnecessary.

I would also recommend using a main function for your main loop. You could move all of the stuff in the bottom into a if __name__ == "__main__":, which also would allow you to load in this python script into other programs.

Naming

Remember that Readability counts. The standard in python is to use snake_case for variable names, but more importantly ensure that your names make the variables' purpose clear; avoid names like x and sizea.


Show number of pages next to Flickr threads This is a userscript that simply adds the number of pages that a thread has next to its title for Flickr interface. (FWIW it's not really done because the script is only loaded at page load, but not reloaded on subsequent AJAX calls that the user may make).

I'm far from being a seasoned JS developer, so I'm looking for guidelines to potentially bring the script to state-of-the-art JS standards.

Example of questions that I'm not sure about:

Should I use promises?
Is the workflow done well?
Is the inline CSS/HTML evil in this case?
Should I omit jQuery?
Am I making too many jQuery calls? (i.e. should I just make one query and find the next rows through siblings or something similar?)

Any pointers are appreciated! (this is also my first userscript, so feedback about the userscript itself is also appreciated)

// ==UserScript==
// @name        Flickr - Number of pages in a thread
// @namespace   flickr_
// @description This user script is simply displaying the number of pages in a thread on the main page of the group
// @include     http*://*flickr.com/groups/*
// @version     1
// @grant       none
// @run-at      document-start
// @require     http://ajax.googleapis.com/ajax/libs/jquery/1.2.6/jquery.js
// ==/UserScript==

var api_key = 'fe753c0dd6068fb6c74cb70458b1b9a0';
var POSTS_PER_PAGE = 100;

function call_flickr(method, params, callback) {
    'use strict';
    var url = "https://api.flickr.com/services/rest?";
    params.api_key = api_key;
    params.format = 'json';
    params.nojsoncallback = 1;
    params.method = method;
    $.get(url, params, function (data) {
        callback(JSON.parse(data));
    });
}

function number_of_pages(data) {
    'use strict';
    var url = '/groups/' + data.topics.path_alias + '/discuss/';
    data.topics.topic.forEach(function (topic) {
        var pages_number = Math.ceil(topic.count_replies / POSTS_PER_PAGE);
        var link = url + topic.id + '/';
        var last_link = link + 'page' + pages_number;
        var current_element = $('a[href=' + link + ']');
        if (pages_number > 1) {
            current_element.after(' ' + pages_number + '');
        }
    });
}

function threads_list(data) {
    'use strict';
    var group_id = data.group.id;
    var params = {};
    params.group_id = group_id;
    call_flickr('flickr.groups.discuss.topics.getList', params, number_of_pages);
}

function main() {
    'use strict';
    var params = {};
    params.url = document.URL;
    call_flickr('flickr.urls.lookupGroup', params, threads_list);
}

main();

First at all I not so used to usercript. But in general I think your solution is overengineered.

I can point:

Readability of code is not good.
The "standar" for JavaScript usually is camel case.

Regarding your questions:

You don't have any complicated workflow or expecting several answers, so I wouldn't use promises.
As I said before, I think your code is really hard to read for only being three functions
You could remove the HTML in line creating the elements by JavaScript, but I don't think is so much for calling it "evil"...
You could
Don't think so

Few changes I would made:

Only declare one, 'use strict'

var url = "https://api.flickr.com/services/rest?";

var POSTS_PER_PAGE = 100;

var params = {
    url: document.URL,
    api_key: 'fe753c0dd6068fb6c74cb70458b1b9a0',
    method: 'flickr.urls.lookupGroup',
    format: 'json',
    nojsoncallback: 1
};

Whit this change I don't think that you are gaining so much whit your current function call_flickr.

So I would split it in two functions each one whit the call. Like this:

'use strict';

var url = "https://api.flickr.com/services/rest?";

var POSTS_PER_PAGE = 100;

var params = {
    url: document.URL,
    api_key: 'fe753c0dd6068fb6c74cb70458b1b9a0',
    format: 'json',
    nojsoncallback: 1
};

function numberOfPages(data) {
    var url = '/groups/' + data.topics.path_alias + '/discuss/';
    data.topics.topic.forEach(function (topic) {
        var pages_number = Math.ceil(topic.count_replies / POSTS_PER_PAGE);
        var link = url + topic.id + '/';
        var last_link = link + 'page' + pages_number;
        var current_element = $('a[href=' + link + ']');
        if (pages_number > 1) {
            current_element.after(' ' + pages_number + '');
        }
    });
}

function threadsList(data) {
    params.group_id = data.group.id;
    params.method = 'flickr.groups.discuss.topics.getList';

    $.get(url, params, function (data) {
        numberOfPages(JSON.parse(data));
    });
}

function main() {

    params.method = 'flickr.urls.lookupGroup';

    $.get(url, params, function (data) {
        threadsList(JSON.parse(data));
    });

}

main();


Simplifying these methods for iterating through object and modifying string fields I have this method (consisting of three methods) that I need to use for looking through objects that are going to be serializes and logged as json objects.
a bit of intro (not necessary to read)
The initial problem was that I noticed that sometimes I would receive data in objects from external api's that would contain personal sensitive data i.e social security numbers. I would still like to log these objets though, I just want to sanitize the social sec numbers away, and I also would like to have some method that could this for object with any kind of internal structure, such that I could reuse it, and would'nt need to rewrite a new sanitization method for each log.
The problem
So now I wanted these methods that could look into objects of any type, and sanitize all the string fields, such that the strings would look the same, but for any string field with a social security number of the type 123456-7891, should be replaced with XXXXXX-XXXX.
public static void DoitNow(object obj)
{
    Queue<PropertyInfo> properties = new Queue<PropertyInfo>(obj.GetType().GetProperties());
    while (properties.Count != 0)
    {
        PropertyInfo property = properties.Dequeue();

        if (property.GetValue(obj) is string)
        {
            string stringField = property.GetValue(obj).ToString();

            String sanitizedField = sanitizeSensitiveString(stringField);
            property.SetValue(obj, sanitizedField);

        }
        else if (property.GetValue(obj) is IList)
        {
            IList y = (IList)property.GetValue(obj);

            if (y.Count != 0)
            {
                if (y[0] is string)
                {
                    for (int i = 0; i < y.Count; i++)
                        y[i] = sanitizeSensitiveString((string)y[i]);
                }
                else if (IsNested(y[0]))
                {
                    for (int i = 0; i < y.Count; i++)
                    {
                        DoitNow(y[i]);
                    }
                }
            }
        }
        else if (IsNested(property.GetValue(obj)))
        {
            var l = property.GetValue(obj);

            DoitNow(l);
        }
    }
}

public static string sanitizeSensitiveString(string sensitiveString)
{
    return Regex.Replace(sensitiveString, @"\d{6}-?\d{4}[0-9]*", " XXXXXX-XXXX ");
}

public static bool IsNested(object value)
{
    Type t = value.GetType();
    if (t.IsPrimitive || value is string)
    {
        return false;
    }
    FieldInfo[] fields = t.GetFields(BindingFlags.NonPublic |
                                     BindingFlags.Public |
                                     BindingFlags.Instance);
    if (fields.Any())
    {
        return true;
    }
    return false;
}

Now, the method DoitNow is the core method here, it takes any object as a parameter, and uses projection to look through all the properties of it. If a field is a string field, we use the method sanitizeSensitiveString to sanitize the string. If a property is itself a object (I use the method isNested to determine this) I will call the method recursively on it. If a property is a List type, I look into the list and sanitize if it contains strings, and recurse if it contains complex objects.
I would like to get some tips on my IsNested method, am I using projection right? In general am I using projection In a way that makes sense to access the fields and modify them?
Also, In order to be able to operate with many list types, I use the IList interface, however this has led me to using for-loops in a way that I find very un-elegant, can I replace some of my loops with some functional type function calls? I also use if (list.count != 0) to test if a list is empty, I just find this plain stupid.
Any other optimizations are welcomed!

As always simplicity is king. In my opinion you have done a lots of extra effort which might not be necessary.
Let's look at the problem from 10 000 feet high:

You receive an arbitrary object as input
You need to produce a string output where the SN data are masked out

What if you first serialize the data and then mask it?
public static string MaskOut(object toBeMaskedObject,
    string regexPattern = @"\d{6}-?\d{4}[0-9]*",
    string mask = " XXXXXX-XXXX ")
{
    var serializedData = JsonConvert.SerializeObject(toBeMaskedObject);
    return Regex.Replace(serializedData, regexPattern, mask);
}

And that's it.
I've used the following data structures for testing:
var simpleWithoutSN = new
{
    Desc = "123",
    Id = 1
};

var simpleWithSN = new
{
    SN = "123456-7891",
    Id = 1
};

var nestedWithSN = new
{
    Id = 1,
    Items = new []
    {
        new { SN  = "123456-7891",},
        new { SN  = "123456-7892",},
    }

};

Console.WriteLine(MaskOut(simpleWithoutSN));
Console.WriteLine(MaskOut(simpleWithSN));
Console.WriteLine(MaskOut(nestedWithSN));

The output will be:
{"Desc":"123","Id":1}
{"SN":" XXXXXX-XXXX ","Id":1}
{"Id":1,"Items":[{"SN":" XXXXXX-XXXX "},{"SN":" XXXXXX-XXXX "}]}


Memoized Prime Generator Here is my code:

def odd_numbers_from(n):
    if n % 2 == 0:
        n = n + 1
    while True:
        yield n
        n = n + 2

def takeWhile(pred, lst):

    for item in lst:
        if pred(item):
            yield item
        else:
            break

def takePrimes(iterator):

    for n in iterator:

        if not 0 in (n % k for k in takeWhile(lambda x: x**2 <= n, primes())):

            yield n

# -- memoized prime generator
def primes(earlier_primes=[2]):

    yield from earlier_primes

    for new_prime in takePrimes(odd_numbers_from(earlier_primes[-1]+1)):

        earlier_primes.append(new_prime)

        yield new_prime

if __name__ == "__main__":

    for k in primes():

        print(k)
        if k > 300:
            break

In first run, it will produce prime numbers as long as they are requested. On subsequent runs, it will just yield from previously generated primes. We can test this behaviour like this:

# -- memoized prime generator
def primes(earlier_primes=[2]):

    yield from earlier_primes

    for new_prime in takePrimes(odd_numbers_from(earlier_primes[-1]+1)):
        print("generated new prime")
        earlier_primes.append(new_prime)

        yield new_prime

if __name__ == "__main__":

    for k in primes():

        print(k)
        if k > 20:
            break

    print("=====")

    for k in primes():

        print(k)
        if k > 30:
            break

This should output:

2
generated new prime
3
generated new prime
5
generated new prime
7
generated new prime
11
generated new prime
13
generated new prime
17
generated new prime
19
generated new prime
23
=====
2
3
5
7
11
13
17
19
23
generated new prime
29
generated new prime
31

How does it look?

1. Review

There are no docstrings. What do these functions do? How do I call them? What do they return?
The function odd_numbers_from could be implemented using itertools.count, like this:

def odd_numbers_from(n):
    """Generate the odd numbers greater than or equal to n."""
    return itertools.count(n + 1 - (n % 2), 2)

But if you started with earlier_primes=[2,3] then you could avoid the special case here.
The function takeWhile is built into Python under the name itertools.takewhile.
takePrimes is only called from primes, so would make more sense to be inlined there.
A Python object that can be looped over using the for statement (like the argument to takePrimes) is properly known as an iterable, not a iterator.
Instead of not 0 in expression, write all(expression).

2. Revised code

from itertools import count, islice, takewhile

def primes(earlier_primes=[2, 3]):
    """Generate the prime numbers.

    >>> list(islice(primes(), 10))
    [2, 3, 5, 7, 11, 13, 17, 19, 23, 29]

    """
    yield from earlier_primes
    for n in count(earlier_primes[-1] + 2, 2):
        if all(n % p for p in takewhile(lambda p: p**2 <= n, primes())):
            earlier_primes.append(n)
            yield n


Regular expression matching with string slice in Go I have a slice of strings, and within each string contains multiple key=value formatted messages. I want to pull all the keys out of the strings so I can collect them to use as the header for a CSV file. I do not know all potential key fields, so I have to use regular expression matching to find them.

Here is my code.

package main

import (
    "fmt"
    "regexp"
)
func GetKeys(logs []string) []string {
        // topMatches is the final array to be returned.
        // midMatches contains no duplicates, but the data is `key=`.
        // subMatches contains all initial matches.
        // initialRegex matches for anthing that matches `key=`. this is because the matching patterns.
        // cleanRegex massages `key=` to `key`
        topMatches := []string{}
        midMatches := []string{}
        subMatches := []string{}
        initialRegex := regexp.MustCompile(`([a-zA-Z]{1,}\=)`)
        cleanRegex := regexp.MustCompile(`([a-zA-Z]{1,})`)

        // the nested loop for matches is because FindAllString
        // returns []string
        for _, i := range logs {
                matches := initialRegex.FindAllString(i, -1)
                for _, m := range matches {
                        subMatches = append(subMatches, m)
                }
        }

        // remove duplicates.
        seen := map[string]string{}
        for _, x := range subMatches {
                if _, ok := seen[x]; !ok {
                        midMatches = append(midMatches, x)
                        seen[x] = x
                }
        }
        // this is where I remove the `=` character.
        for _, y := range midMatches {
                clean := cleanRegex.FindAllString(y, 1)
                topMatches = append(topMatches, clean[0])
        }
        return topMatches
}

func main() {
    y := []string{"key=value", "msg=payload", "test=yay", "msg=payload"}
    y = GetKeys(y)
    fmt.Println(y)
}

I think my code is inefficient because I cannot determine how to properly optimise the initialRegex regular expression to match just the key in the key=value format without matching the value as well.

Can my first regular expression, initialRegex, be optimised so I do not have to do a second matching loop to remove the = character?

Playground: http://play.golang.org/p/ONMf_cympM

You're not making good use of regular expressions. A single regex can do the job:

pattern := regexp.MustCompile(`([a-zA-Z]+)=`)

The parentheses (...) are the capture the interesting part for you.

You can use result = pattern.FindAllStringSubmatch(s) to match a string against the regex pattern. The return value is a [][]string, where in each []string slice, the 1st element is the entire matched string, and the 2nd, 3rd, ... elements have the content of the capture groups. In this example we have one capture group (...), so the value of the key will be in item[1] of each []string slice.

Instead of a map[string]string map for seen, a map[string]boolean would be more efficient.

Putting it together:

func GetKeys(logs []string) []string {
    var keys []string
    pattern := regexp.MustCompile(`([a-zA-Z]+)=`)

    seen := make(map[string]bool)
    for _, log := range(logs) {
        result := pattern.FindAllStringSubmatch(log, -1)
        for _, item := range result {
            key := item[1]
            if _, ok := seen[key]; !ok {
                keys = append(keys, key)
                seen[key] = true
            }
        }
    }

    return keys
}

If the input strings are not guaranteed to be in the right format matching the pattern, then you might want to add a guard statement inside the main for loop, for example:

    if len(result) != 2 {
        continue
    }


Ternary extension method I created the following HtmlHelper extension method for my Asp.Net MVC Razor views because the ternary syntax sucks when you need to use it intermixed with markup.

Is there a better way to write this and are there any potential type safety issues with it?

public static string GetValueTernary(this HtmlHelper htmlHelper, object a, object b, object valueIfEqual, object valueIfNotEqual)
{
    return a.Equals(b) ? valueIfEqual.ToString() : valueIfNotEqual.ToString();
}

and its usage in a Razor view is like this:

...

I would change your method to use generics (so that the return value is meaningful) and not call .ToString() internally. This will enable you to use it in more scenarios.

public static T GetValueTernary<T>(this HtmlHelper html, object a, object b, T valueIfEqual, T valueIfNotEqual)
{
    return object.Equals(a, b) ? valueIfEqual : valueIfNotEqual;
}

Now you can use it like this as well and also it will enable you to use HtmlString-s as values.

...

You could also benefit (depends on your actual usage) from not using object for the compared values - by using a generic the compiler will complain if the types of the two are not compatible (for example, you are comparing string to int):

public static TReturn GetValueTernary<TValue, TReturn>(this HtmlHelper html, TValue a, TValue b, TReturn valueIfEqual, TReturn valueIfNotEqual)
{
    return object.Equals(a, b) ? valueIfEqual : valueIfNotEqual;
}


VBA - new built-in enumerations and backwards compatibility msoGraphic is an MsoShapeType enumeration that is available in Office PowerPoint 2016 and later (versions which can handle svg graphics). If you attempt to check an MsoShapeType against msoGraphic in earlier versions of Office you will get a compile error - as msoGraphic is not defined (in any module containing Option Explicit). I am handling this by putting this property in a module where Option Explicit is NOT declared - and calling it from anywhere that needs the value of the constant. The module only contains this property (and any other properties that need to handle any other constants in this way).

Public Property Get myMsoGraphic() As Long

  If msoGraphic = 0 Then
    myMsoGraphic = 28
  Else
    myMsoGraphic = msoGraphic
  End If

End Property

I could of course have just re-declared msoGraphic as a constant with value 28, but best practice seems to be that you should avoid using the actual value and use the enumerate constant instead - in case the value gets changed at some point in the future (which I guess is probably highly unlikely).

Does this seem like the best way to handle this situation?

Without Option Explicit, the msoGraphic identifier in that property scope is a Variant/Empty; there's an implicit type conversion happening when you do this:

If msoGraphic = 0 Then

Sure vbEmpty will equate to 0, or even vbNullString or "", but that's after converting to a comparable type (Integer, or String). There's a better way.

If IsEmpty(msoGraphic) Then

The IsEmpty function will only ever return True when given a Variant/Empty value - which is exactly what we're dealing with here.

  I could of course have just re-declared msoGraphic as a constant with value 28, but best practice seems to be that you should avoid using the actual value and use the enumerate constant instead

One doesn't exclude the other. If you define a public constant in an appropriately named standard module (e.g. OfficeConstants), and use it, then you are adhering to the best practice. What happens then is deliberate shadowing of the MsoShapeType.msoGraphic declaration - something Rubberduck would normally warn about, but with a descriptive @Ignore or @IgnoreModule annotation comment, the intention is clarified, and the static code analysis tool knows to ignore them - and with a link to the official documentation, you ensure the values correctly match the actual documented underlying values for each identifier:

'@IgnoreModule ShadowedDeclaration: these constants are only available in Office 2016+
Option Explicit
'https://docs.microsoft.com/en-us/office/vba/api/office.msoshapetype
Public Const msoGraphic As Long = 28
'...

What you want to avoid, is code like this, where 28 is some magic value that has no clear meaning:

If shapeType = 28 Then

  Does this seem like the best way to handle this situation?

The problem is that you can't name your property msoGraphic (well you could, but then you'd have to fully-qualify the msoGraphic constant, and then that wouldn't be compilable, even without Option Explicit), so any code (hopefully with Option Explicit specified) that means to use the name msoGraphic now needs to use myMsoGraphic instead, and that isn't ideal, because it adds to the overall cognitive load: you, future you, and eventual maintainers have to remember to avoid msoGraphic and use myMsoGraphic instead, whereas with a Public Const in a standard module that hides/shadows the constant from the referenced PowerPoint library when it exists, usage is much more seamless.

That said, while VBA is case-insensitive, a camelCase public member clashes with the naming convention of pretty much everything else in your standard libraries - enum members only have a lowercase prefix as a namespace surrogate; every type, method, member, property, procedure, function, constant, in every standard library, uses PascalCase. There's no reason to not make every single one of your own procedures use this naming convention too; also while the my prefix is ubiquitous in so many VB examples, it's not a good prefix to use in actual code.

One last thing:

  in case the value gets changed at some point in the future

It won't: MsoShapeType is defined in a shared Office library that isn't only referenced by PowerPoint projects - VBA is ridiculously backward-compatible (line numbers, GoSub/Return statements, Call, Let, DefBool keywords, to name a few should-be-extinct-but-aren't-because-backward-compatibility still-supported language features): there is no way any constant ever released into the wild is ever going to get a new value in any future release. Not going to happen.


JSON serialization helper class I wrote this class to one-line all my JSON serialization, and I'm curious of any input on it.

It's only responsible for serializing/deserializing any type to/from JSON.

The comments and code are pretty self explanatory, but if you want any extra explanation of it please feel free to comment.

Not a lot here, so comment on what you can.

/// <summary>
/// Provides methods for Serialization and Deserialization of JSON/JavaScript Object Notation documents.
/// </summary>
public class JsonSerialization
{
    /// <summary>
    /// Serializes an object to a JSON/JavaScript Object Notation string.
    /// </summary>
    /// <typeparam name="T">The type of the object to serialize.</typeparam>
    /// <param name="value">The object to serialize.</param>
    /// <param name="serializedJson">Filled with a string that is the JsonSerialized object.</param>
    /// <param name="throwErrors">If true, will throw errors. Otherwise, returns false on failures.</param>
    /// <returns>A boolean value indicating success.</returns>
    public static bool Serialize<T>(T value, ref string serializedJson, bool throwErrors = false)
    {
        if (value == null)
        {
            return false;
        }

#if DEBUG
#warning When in DEBUG Mode JavaScript Serialization Errors will be thrown regardless of throwErrors parameter.
            throwErrors = true;
#endif

        try
        {
            JavaScriptSerializer jss = new JavaScriptSerializer();
            serializedJson = jss.Serialize(value);
            return true;
        }
        catch
        {
            if (throwErrors)
                throw;

            return false;
        }
    }

    /// <summary>
    /// Deserializes a JSON/JavaScript Object Notation string to an object.
    /// </summary>
    /// <typeparam name="T">The type of the object to serialize.</typeparam>
    /// <param name="value">The JSON string representing the serialized object.</param>
    /// <param name="deserializedObject">Filled with the object that is the JsonSerialized string.</param>
    /// <param name="throwErrors">If true, will throw errors. Otherwise, returns false on failures.</param>
    /// <returns>A boolean value indicating success.</returns>
    public static bool Deserialize<T>(string value, ref T deserializedObject, bool throwErrors = false)
    {
        if (value == null)
        {
            return false;
        }

#if DEBUG
#warning When in DEBUG Mode JavaScript Deserialization Errors will be thrown regardless of throwErrors parameter.
            throwErrors = true;
#endif

        try
        {
            JavaScriptSerializer jss = new JavaScriptSerializer();
            deserializedObject = jss.Deserialize<T>(value);
            return true;
        }
        catch
        {
            if (throwErrors)
                throw;

            return false;
        }
    }
}

Style

The overall reading of these methods is pretty good and the code is well structured.

The documentation is good to read and to understand.

Your style is a little bit inconsistent. It's about the using of braces {} for single statement if's . One time at the guard clause you use braces and later on for the if(throwErrors)  you don't use them.
Speaking about throwErrors, the method won't ever throw errors but one Exception so a better name could be either shouldThrowException or throwsException.

Speaking about Serialize() method
I don't like ref parameters that much and I try to avoid them if there is a better/different way to solve this.
For the Serialize() method the usage of an ref parameter could be avoided by passing a StringBuilder instead of a string as argument.
You for sure would need to check if the passed in StringBuilder is not null but you could just pass it afterwards to this overload of the Serialize() method.


Python UDP class to control drone I'm currently building a small plane with Raspberry Pi Zero W. I wrote a small code which uses UDP to control it.

Here I'm resending packets regularly in a thread because of the joystick input (it gives a lot of values when used and time.sleep() in a pygame event loop does bad things). Am I doing it the right way?

I'm also sending three bytes only per packets (which is enough for the control), but should I use power of 2 packet size? And why?

import socket
import struct
import time
import threading

S_TYPE = "BBB"

class Server:
    def __init__(self, ip="", port=8080, timeout=-1):
        self.ip = ip
        self.port = port
        self.sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
        self.sock.bind((ip, port))
        if (timeout > 0):
            self.sock.settimeout(timeout)

    def recv(self):
        data = self.sock.recv(struct.calcsize(S_TYPE))
        res = struct.unpack("BBB", data)
        return (res)

class Client:
    def __init__(self, ip, port=8080, interval=100):
        self.ip = ip
        self.port = port
        self.data = (0, 0, 0)
        self.interval = interval
        self.sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
        self.thread = threading.Thread(target=self.send_loop)
        self.thread.daemon = True
        self.lock = threading.Lock()
        self.running = True

    def send(self, data_bytes):
        self.sock.sendto(data_bytes, (self.ip, self.port))

    def send_loop(self):
        '''Send loop is used to regulary send data in a thread'''
        while (self.running):
            self.lock.acquire()
            data_bytes = struct.pack(S_TYPE, *self.data)
            self.lock.release()
            self.send(data_bytes)
            time.sleep(self.interval / 1000.0)

    def update(self, a, b, c):
        '''Update data which is sent'''
        self.lock.acquire()
        self.data = (a, b, c)
        self.lock.release()

    def start(self):
        self.thread.start()

    def stop(self):
        # __del__ wouldn't be called with a thread running
        self.running = False

EDIT: do you see any problem not using a lock for self.running?

Some notes:

data as a name has been criticised elsewhere; I would simply say that any variable or property should be self-explanatory within context. a, b, and c should also have more descriptive names.
You don't need self.ip and self.port in the server - they are only used in the constructor. YAGNI.
You should default timeout to None and forward it directly to the socket configuration. Making your code have different timeout semantics from the socket library is probably just going to cause confusion.
Why return (res) instead of return res?


Priority checker for infix-to-postfix converter I am trying to implement an infix to postfix conversion program using stacks.  I have actually have done it successfully, but I'm uncertain about the complexity of the logic that I used for the priority checking.

int hasLowerPriority(char ch,char ch2)
{
    int num=0,num2=0;
    if(ch=='+'||ch=='-')
        num=1;
    if(ch=='*'||ch=='/')
        num=2;
    if(ch2=='+'||ch2=='-')
        num2=1;
    if(ch2=='*'||ch2=='/')
        num2=2;
    if(num<num2)
        return 1;
    else
        return 0;
}

To keep it simple I am using four basic operators only. I am looking for optimized code for the same operation. Can I reduce its complexity?

There is duplicated code that can be removed.

static int precedence(char operator) {
    switch (operator) {
      case '*': case '/':
        return 2;
      case '+': case '-':
        return 1;
      default:
        return 0;
    }
}

int hasLowerPriority(char operator, char otherOperator) {
    return precedence(operator) < precedence(otherOperator);
}

At that point, you might as well get rid of the hasLowerPriority() function altogether, since precedence(operator) < precedence(otherOperator) is clearer and more readable.

(Note that operator is a reserved keyword in C++, but not in C.  You might want to rename it to op or something.)


Changing height of an UI panel using Array content I've created following code which works just fine:

GraphBar1.GetComponent<RectTransform> ().sizeDelta = new Vector2 (50, oScript.reactionTimeArray [0] * 1000);
GraphBar2.GetComponent<RectTransform> ().sizeDelta = new Vector2 (50, oScript.reactionTimeArray [1] * 1000);
GraphBar3.GetComponent<RectTransform> ().sizeDelta = new Vector2 (50, oScript.reactionTimeArray [2] * 1000);
GraphBar4.GetComponent<RectTransform> ().sizeDelta = new Vector2 (50, oScript.reactionTimeArray [3] * 1000);
GraphBar5.GetComponent<RectTransform> ().sizeDelta = new Vector2 (50, oScript.reactionTimeArray [4] * 1000);
GraphBar6.GetComponent<RectTransform> ().sizeDelta = new Vector2 (50, oScript.reactionTimeArray [5] * 1000);
GraphBar7.GetComponent<RectTransform> ().sizeDelta = new Vector2 (50, oScript.reactionTimeArray [6] * 1000);
GraphBar8.GetComponent<RectTransform> ().sizeDelta = new Vector2 (50, oScript.reactionTimeArray [7] * 1000);
GraphBar9.GetComponent<RectTransform> ().sizeDelta = new Vector2 (50, oScript.reactionTimeArray [8] * 1000);
GraphBar10.GetComponent<RectTransform> ().sizeDelta = new Vector2 (50, oScript.reactionTimeArray [9] * 1000);

As I'm re-writing almost the same code so many times, I'm convinced this can be done using less code, even though I'm confused on how to do it with the Array properties being present.

It could be written as :

var graphBars = new[] { GraphBar1, GraphBar2, GraphBar3, GraphBar4, GraphBar5, GraphBar6, GraphBar7, GraphBar8, GraphBar9, GraphBar10 };
for (int i = 0; i < graphBars.Length; i++)
    graphBars[i].GetComponent<RectTransform>().sizeDelta =
        new Vector2(50, oScript.reactionTimeArray[i] * 1000);


Printing element combinations that make a neutral compound An array is given with the element name and their respective pH values. Print the combination of 2 elements which make a neutral compound

Here is the solution that I came up with but uses a Hashmap hence O(N) space complexity. Any idea I can reduce space complexity but maintaining O(N) time complexity

   public static void FindCombinationSuchThatPHValueIsNeutralized(PhValue[] input)
         {
             var map = new Dictionary<int, string>();
             foreach (var phValue in input)
             {
                 if (!map.ContainsKey(-phValue.Value))
                 {
                     map.Add(phValue.Value, phValue.Element);
                 }

                 else
                 {
                     Console.WriteLine(phValue.Element + "," + map[-phValue.Value]);
                     break;
                 }

             }

         }

I don't believe you can reduce the space complexity, and still maintain the \$O(n)\$ time complexity. There is a trade off here you have to compromise on. Of course, you can, if you want, run on \$O(1)\$ space complexity, but your time complexity will have to be compromised.

Now, your code has a bug.... it needs to add the element to the Dictionary even when it has a match for neutral.... Consider a data set like (fake elements and pH's):

H 0
He 1
C -1
Au 1
...

Then, I would expect there to be two matches, both C and He are 0, and Au and C are 0, but, when you matched C against He, you did not add C to the Dictionary, so it is not in the Dictionary when you process Au, and the match is missed.

Unfortunately, as you add the Au on the match, though, you will overwrite the existing He entry.... because you do not support multiple elements with the same pH.

You need your map to be a Dictionary<Int, IList<string>>, and you need to add the element to the list for the respective pH, Then, when you find a match, you need to map it against all the other Elements....

I believe this will require a full process once through the data, and then another to go through the keys of the Dictionary.

Edit: A third bug (thanks @Heslacher) is that the neutral pH is 7, so your check should be:

if (!map.ContainsKey( 7 - (phValue.Value - 7))


File duplicate finder In an attempt to clean out my picture collection I made a script to find all duplicate files

import datetime
import hashlib
import json
import typing
from collections import defaultdict
from pathlib import Path

def filesize(path: Path) -> int:
    """returns the filesize"""
    return path.stat().st_size

def hash_first(
    path: Path, hash_function=hashlib.md5, blocksize: int = 65536
) -> str:
    """returns the hash of the first block of the file"""
    with path.open("rb") as afile:
        return hash_function(afile.read(blocksize)).hexdigest()

def hash_all(path: Path, hash_function=hashlib.md5, blocksize=65536) -> str:
    """returns the hash of the whole file"""
    with path.open("rb") as afile:
        hasher = hash_function()
        buf = afile.read(blocksize)
        while buf:
            hasher.update(buf)
            buf = afile.read(blocksize)
    return hasher.hexdigest()

def compare_files(
    comparison: typing.Callable[[Path], typing.Hashable], duplicates: dict
) -> dict:
    """
    Subdivides each group along `comparison`
    discards subgroups with less than 2 items
    """
    results: defaultdict = defaultdict(list)
    for old_hash, files in duplicates.items():
        for file in files:
            results[(*old_hash, comparison(file))].append(file)

    return {
        filehash: files
        for filehash, files in results.items()
        if len(files) > 1
    }

def find_duplicates(
    rootdir: Path,
    comparisons: typing.Iterable[
        typing.Callable[[Path], typing.Hashable]
    ] = (),
):
    """
    Finds duplicate files in `rootdir` and its subdirectories

    Returns a list with subgroups of identical files

    Groups the files along the each of the comparisons in turn.
    Subgroups with less than 2 items are discarded.

    Each of the `comparisons` should be a callable that accepts a
    `pathlib.Path` as only argument, and returns a hashable value

    if `comparisons` is not defined, compares along:
        1. file size
        2. MD5 hash of the first block (65536 bytes)
        3. MD5 hash of the whole file
    """
    if not comparisons:
        comparisons = filesize, hash_first, hash_all

    duplicates = {(): filter(Path.is_file, rootdir.glob("**/*"))}
    for comparison in comparisons:
        duplicates = compare_files(comparison, duplicates)
    return [tuple(map(str, files)) for files in duplicates.values()]

if __name__ == "__main__":
    pic_dir = r"./testdata"
    results = find_duplicates(rootdir=Path(pic_dir))

    # print(json.dumps(results, indent=2))

    timestamp = datetime.datetime.now().strftime("%Y%m%d_%H%M%S")
    output_file = Path("./output") / f"{timestamp}.json"
    with output_file.open("w") as fh:
        json.dump(results, fh, indent=2)

Any remarks on this code?

The code is formatted with black (linelength 79), with mypy pylint and pylama as linters.

If I try to specify the dict as argument and return value to compare_files to typing.Dict[typing.Tuple[typing.Hashable, ...], typing.Iterable[Path]], mypy complains: Incompatible types in assignment (expression has type "Dict[Tuple[Hashable, ...], Iterable[Path]]", variable has type "Dict[Tuple[], Iterator[Path]]")

Some suggestions:

I would usually inline single line functions unless what they do is hard to understand from just reading the command. filesize might be one such candidate.
I believe that not specifying the block size can be faster, by allowing Python to use whatever block size it thinks is appropriate. This would need some actual testing though.
Your function arguments are only partially typed. You might want to use a stricter mypy configuration, such as the following, in your setup.cfg:

[mypy]
check_untyped_defs = true
disallow_untyped_defs = true
ignore_missing_imports = true
no_implicit_optional = true
warn_redundant_casts = true
warn_return_any = true
warn_unused_ignores = true

MD5 was optimized for cryptographic use, not for speed. Another Q&A explores options for fast non-cryptographic hashing.
I would avoid converting digests to strings - it might be slightly easier for debugging purposes, but data type conversions are in general very costly.
Optional parameters mean you have to test at least two things rather than one. Your code only ever calls the hash_* functions without optional parameters, so you might as well either inline them.
On a related note, static values which are used in multiple places are perfect for pulling out as constants. The hashing function and block size would be obvious candidates for this.
Mutable default parameter values are an accident waiting to happen. The default pattern for this is to use a default of None and to assign a default if foo is None.
r-strings like r"./testdata" are meant for regular expressions. Since pic_dir is not used as such it should probably be a regular string.
duplicates is at first a list of potential duplicates and later is trimmed in stages. This makes the code hard to follow.
Things like the directory/directories to include should be arguments. argparse can deal with this easily.
To make this more scriptable I would print the result on standard output. It's trivial to then redirect it to a file.


Refactor Coffee Script to fire on page load instead of change I have a Rails app where I'm trying to select a list of facilities based on what region is selected in a form.  So far, I've implemented group_collection_select to do this as well as a bit of CoffeeScript.

It works when creating a new record and selecting a region.  The behavior being to only show facilities listed for the selected region.  What does not work is when editing a record, selecting the facilities shows all of the facilities grouped by region instead of constraining the facilities to the selected region.

If I select another region and then select the original reason, the proper list of facilities show up.

I'd like to learn how to refactor the CoffeeScript to where when editing the record the function is fired both on load and on change.

Here's what my code looks like:

calls.js.coffee

jQuery ->
  facilities = $('#call_transfer_from_id').html()

  $('#call_region_id').change ->
    region = $('#call_region_id :selected').text()
    options = $(facilities).filter("optgroup[label=#{region}]").html()

    if options
      $('#call_transfer_from_id').html(options)
    else
      $('#call_transfer_from_id').empty()

region.rb

has_many :facilities

facility.rb

attr_accessible :region_id
belongs_to :region

_form.html.erb excerpt

 <%= f.label :region %>
  <%= f.collection_select(:region_id, Region.all, :id, :area, {:include_blank => true}, {:class => 'select', required: true}) %>
  <%= f.label :Transfer_From %>
  <%= f.grouped_collection_select :transfer_from_id, Region.order(:area), :active_facilities, :area, :id, :facility_name, {include_blank: true}, class: 'select' %>

If my question and examples are not clear, please let me know and I'll be happy to edit.

This is borderline Code Review material, since the code doesn't work as intended. It's not the same thing as "broken", because it does run, but what you're asking for is more or less a new feature.

(I'm also a little unsure of what you're asking, though I think I got it right. This isn't prose review, but you've got some run-on sentences in your question that are nigh impossible to read. Edit: That's been improved now)

Still, I'll give it shot regardless.

First, when you use jQuery, remember to store things you'll use more than once - don't call $(...) again and again for the same selectors.

Second, store a clone of the facilities list element. Not the HTML string. You can just have an element in memory without having to to display it on the page. No need to reduce it a string, and parsing it every time you need something.

Third, I'd use a more precise identifier for the optgroups than the region name. It works, but I'd prefer using an integer ID or something. Unfortunately, grouped_collection_select doesn't have an option to do that (you'd have to write your own version), so I'll skip that here.

I'd extract a function to do the options-setting

$ -> # on DOM ready

  # Grab the elements we'll need
  facilitySelect = $ "#call_transfer_from_id"
  regionSelect = $ "#call_region_id"

  # Make an in-memory clone of the facilities.
  # We'll use this as out "master list"
  facilitiesCopy = facilitySelect.clone()

  # A simple helper function to set the facility select's option
  # to match those in a named optgroup (again, we're cloing elments)
  setFacilityOptionsForSelectedRegion = ->
    region = regionSelect.val()
    options = facilitiesCopy.find("optgroup[label='#{region}'] option").clone()
    facilitySelect.empty().append options

  # Call the function on region change
  regionSelect.on "change", setFacilityOptionsForSelectedRegion

  # and also just call it now that the page has loaded
  setFacilityOptionsForSelectedRegion()

I think that should work.

Edit: Here's a demo


Measuring a given method's execution time I have been playing around with some improvements to some sort algorithms like Selection Sort and Merge Sort and I ended up needing some sort of measurement to check if my versions were any faster than the original algorithm form. I also had a need to implement some sort of time measurement before but never did it, so here came the chance. And so I ended up coding the following measurement method:

public static double Measure(Action action, bool print = true)
{
    Stopwatch watch = new Stopwatch();
    const int precision = 1; //estimated precision of 1 milisecond on my machine
    const int error = 1; //max error
    const int times = 10;
    double min = double.MaxValue;
    for (int i = 0; i < times; ++i)
    {
        int iterations = 0;
        watch.Restart();
        while (watch.Elapsed.Milliseconds < precision*(100-error))
        {
            action();
            ++iterations;
        }
        watch.Stop();
        min = Math.Min(min, watch.Elapsed.Milliseconds*1000.0/iterations);
    }

    if(print)
        Debug.WriteLine("The action takes {0:N4} nanos to complete", min);

    return min;
}

Is this a well conducted measurement algorithm? Any suggestions or improvements that I could apply?

The thing you're timing is:

while (watch.Elapsed.Milliseconds < precision*(100-error))
{
    action();
    ++iterations;
}

The problem is that if action takes a very short time, then most of what you're timing is the time it takes to call the watch.Elapsed.Milliseconds property.

A more accurate timer would be something like:

int iterations = 1000;
watch.Restart();
while (iterations--)
{
    action();
}
watch.Stop();

You would then need to do something to ensure you pick a suitable number of iterations (e.g. try again with 10 times as many iterations if the measured time is too short to be accurate).


jQuery-based flow diagram I have created a proof of concept jQuery based flow diagram based upon kayen's answer to this post.

This diagram shows the flow diagram that I am seeking to create in jQuery:

I also wanted to make the UI as clean as possible, so I have set it that only the current question is displayed as the users works their way through the various questions.

The user can also go back to the last question at any time before they are given an answer.

The code will be used on a website.

As my code is long, I have set it out in full in my jsFiddle. The basic structure is:

HTML:

<!—question 2-->
<fieldset id="question2" style="display:none;">
    <legend>question 2</legend>

                <input type="radio" name="question2" value="Yes" id="rdYes" />Yes

                <input type="radio" name="question2" value="No" id="rdNo" />No

            <input type="button" value="Back to last question" id="back2-1" />

</fieldset>

jQuery:

//Test for question 2
jQuery("input[name=question2]").change(function () {

    if ($(this).val() == "Yes") {
        jQuery("#question2A").show();
        jQuery("#question2").hide();
    } else {
        jQuery("#question3").show();
        jQuery("#question2").hide();
    }
});

//Back button to question 1 from 2
jQuery("#back2-1").click(function () {
    jQuery("#q1").show();
    jQuery("input:radio").attr("checked", false);
    jQuery("#question2").hide();
});

It works, but as this is my first project it would really help me and others learning code, if someone could confirm:

Is there a more efficient way to code this in jQuery?
Would it be better to code it in another language, i.e. to avoid problems with older browsers?

You can simplify the javascript to be something as given below by adding some properties to the markup.

jQuery(function () {

    jQuery('input:radio[name^="question"]').change(function(){
        var $this = $(this);

        $this.closest('.question-container').hide();
        $('#' + $this.data('target')).show();
    });
});

Required changes in the markup

add a class question-container to the question container ie the element with the question id like q1, question1A, etc
For all radio boxes add an additional attribute data-target which will have the id of the question to be displayed if it was checked. ex Yes button in question question1A will have data-target="question1B" and No button will have data-target="question2-1A"

demo: Fiddle


Proper way to execute parameterized commands within a method This method allows for the execution of parameterized commands within a DAL class, and I tried to make it as reusable as possible. It works well but it feels a bit verbose to me. Is there a way to simplify this or is it good as is?

/// <summary>
/// Retrieves records from the specified data source.
/// </summary>
/// <param name="query">The command to be executed.</param>
/// <param name="dataSrc">The data source to use for the command.</param>
/// <param name="parameters">Optional parameters for a parameterized command.</param>
/// <returns>A System.Data.DataTable containing the returned records.</returns>
public static DataTable GetRecords(string query, DelConnection dataSrc, params SqlParameter[] parameters)
{
    if (query == "") throw new ArgumentException("No command provided.", "query");
    string conString = Dal.GetConnectionString(dataSrc);

    using (SqlConnection con = new SqlConnection(conString))
    using (SqlCommand cmd = new SqlCommand(query, con))
    {
        // Add parameters to command
        foreach (SqlParameter p in parameters)
        {
            cmd.Parameters.Add(p);
        }

        // Fill data table
        DataTable data = new DataTable();
        using (SqlDataAdapter adap = new SqlDataAdapter(cmd))
        {
            adap.Fill(data);
        }
        return data;
    }
}   //// End GetRecords()

Why is it static?

First off, I would remove the check for no parameters.  You've already checked that it is not null, if somebody passes in an empty list, the foreach for parameters will just be ignored.

As svick said, I would not return a null value if no results are found.  This complicates code on the calling side because you have to check for null before you work with it.  Same principle as the parameters.  If I have a foreach on an empty list, my code within the foreach would not be called.

I would change conString == "" to string.IsNullOrEmpty(constring)  I would do the same thing for query, because most people will pass an empty string before they pass a space string.  If you think space will be a problem, do both.

I would change this DataTable data = new DataTable(); to use var var data = new DataTable();  It makes the code easier to scan quickly.

If you insist on returning null for empty sets, change (data.Rows.Count > 0) to (data.Rows.Any()).  I think it makes the code much easier to read because it says exactly what you are looking for.


A simple daily horoscope I wrote a simple daily horoscope in Python using Beautiful Soup as scraper. It works fine but the code can use some sort of a... direction if you want to put it that way. I have used underscores in my variable names which is a folly. Please point out how can I correct this and also, please tell me how I can expand this. Thanks you.

import requests
from bs4 import BeautifulSoup

def horoscope(url):
    url_ = url
    page = requests.get(url_)

    soup = BeautifulSoup(page.text, "html.parser")

    horoscope_ = soup.findAll("div", {"class": "block-horoscope-text f16 l20"}, text=True)[0].text

    return horoscope_

if __name__ == '__main__':
    rinput = input("Enter your sign: ")
    sign = str.lower(rinput)
    sign = str.strip(sign)
    url = ''

    signs = ['aries', 'taurus', 'gemini', 'cancer', 'leo', 'virgo', 'libra', 'scorpio', 'sagitarius', 'capricorn',
             'aquarius', 'pisces']

    for index, sign_ in enumerate(signs):
        if sign == sign_:
            url = "https://www.horoscope.com/us/horoscopes/general/horoscope-general-daily-today.aspx?sign={0}".format(
                index + 1
            )

print (horoscope(url))

I don't see an underscore variable name that is needed.  I don't understand why you did url_ = url in horoscope().  Why not just use url directly?  The only time you would want to do a straight var1 = var2 is for when one of them will be assigned to something else, and you want to remember the original value.  In this case, neither one changes, so they are still both the original by the end of the function.  I also don't see why you said horoscope_ = ... instead of just horoscope = ...  I think it may be because the function itself is named horoscope, but why should that make a difference?  Yes, conceivably you might have recursion later that would be surprised when horoscope means something else, but ... wait, why would you have recursion?  Since you won't, it doesn't change anything inside the function to have the same name, and since it isn't a global variable, it doesn't effect anything else.  As for sign_, I'll get to that later.

I am happy to see you using if __name__ == "__main__":.  That is an excellent piece of code that many people leave out.  I do wonder one thing, though: why is the print(...) outside of it?  Since url isn't defined unless the if __name__ == "__main__": block executes, you will have an error if you try to import this module.

The way you use str.lower(...) and str.strip(...) is definitely not conventional.  You very rarely need to access instance methods by using the class method with the instance as the first argument.  Just use it like an instance method:

sign = rinput.lower()
sign = sign.strip()

I would also suggest merging them:

sign = rinput.strip().lower()

rinput seems to be a strange name for user input. I would do something more like this:

sign = input("Enter your sign: ").strip().lower()

There really isn't a reason I can see to create more than one variable.

You first define url as blank.  I'm not sure why you do that, because a blank url causes a requests.exceptions.MissingSchema error in horoscope, whereas no url at all causes a NameError.  As far as I can tell, it doesn't make a difference to the user which error is shown.  I believe the error shouldn't be shown.  You should have error checking, and ask the user again for input.  (More later)

You don't need to use a for loop to get the index.  The list class already has a very handy method for it: .index().  I would suggest that you use it:

sign = signs.index(sign) + 1
url = "https://www.horoscope.com/us/horoscopes/general/horoscope-general-daily-today.aspx?sign={0}".format(sign)

This will throw an IndexError if the user types the wrong thing, so I would do something like this:

sign = ''
while True:
    sign = input("Enter your sign: ").lower.strip()
    if sign in signs:
        break
    print("That sign is not available.  The available signs are:")
    print("\n".join(signs)

That way, you make sure that the sign is valid before you try to get an index.

The requests.get function cat take a params argument for giving variables in the url.  I would suggest that you use that instead of putting the variables right into the url.  To do that, horoscope could be defined like this:

def horoscope(sign):
    url = "https://www.horoscope.com/us/horoscopes/general/horoscope-general-daily-today.aspx"
    page = requests.get(url, params={'sign': sign})

You can then call it with horoscope(sign) (where sign is the index + 1).


Finding words of different lengths given six letters I'm trying to find all the 3, 4, 5, and 6 letter words given 6 letters.  I am finding them by comparing every combination of the 6 letters to an ArrayList of words called sortedDictionary.  I have worked on the code a good bit to get it to this point.

I tested how many six letter words are checked and got 720 which is good because 6*5*4*3*2*1=720 which means I am not checking any words twice.  I can't make it faster by getting rid of duplicate checks because I have already gotten rid of them all.

Can I still make this faster?

Note that sortedDictionary only contains about 27 hundred words.

for(int l1 = 0; l1 < 6; l1++){
    for(int l2 = 0; l2 < 6; l2++){
        if(l2 != l1)
            for(int l3 = 0; l3 < 6; l3++){
                if(l3 != l1 && l3 != l2){
                    if(sortedDictionary.contains(""+anagramCharacters[l1]+anagramCharacters[l2]+anagramCharacters[l3]))
                        anagram_words.add(""+anagramCharacters[l1]+anagramCharacters[l2]+anagramCharacters[l3]);
                    for(int l4 = 0; l4 < 6; l4++){
                        if(l4 != l1 && l4 != l2 && l4 != l3){
                            if(sortedDictionary.contains(""+anagramCharacters[l1]+anagramCharacters[l2]+anagramCharacters[l3]+anagramCharacters[l4]))
                                anagram_words.add(""+anagramCharacters[l1]+anagramCharacters[l2]+anagramCharacters[l3]+anagramCharacters[l4]);
                            for(int l5 = 0; l5 < 6; l5++){
                                if(l5 != l1 && l5 != l2 && l5 != l3 && l5 != l4){
                                   if(sortedDictionary.contains(""+anagramCharacters[l1]+anagramCharacters[l2]+anagramCharacters[l3]+anagramCharacters[l4]+anagramCharacters[l5]))
                                       anagram_words.add(""+anagramCharacters[l1]+anagramCharacters[l2]+anagramCharacters[l3]+anagramCharacters[l4]+anagramCharacters[l5]);
                                   for(int l6 = 0; l6 < 6; l6++){
                                       if(l6 != l1 && l6 != l2 && l6 != l3 && l6 != l4 && l6 != l5)
                                           if(sortedDictionary.contains(""+anagramCharacters[l1]+anagramCharacters[l2]+anagramCharacters[l3]+anagramCharacters[l4]+anagramCharacters[l5]+anagramCharacters[l6]))
                                               anagram_words.add(""+anagramCharacters[l1]+anagramCharacters[l2]+anagramCharacters[l3]+anagramCharacters[l4]+anagramCharacters[l5]+anagramCharacters[l6]);
                                    }
                                }
                            }
                        }
                    }
                }
            }
    }
}

My solution, still probably not the best written code but it reduce the loading time from about 1-2 seconds to nearly instant (no noticeable wait time; didn't actually test how long it was).

for(int i = 0; i < sortedDictionary.size(); i++){
    for(int index = 0; index < anagram.length(); index++)
        anagramCharacters[index] = anagram.charAt(index);
    forloop:
    for(int i2 = 0; i2 < sortedDictionary.get(i).length(); i2++){
        for(int i3 = 0; i3 < anagramCharacters.length; i3++){
            if(sortedDictionary.get(i).charAt(i2) == anagramCharacters[i3]){
                anagramCharacters[i3] = 0;
                break;
            }
            else if(i3 == anagramCharacters.length-1)
                break forloop;
        }
        if(i2 == sortedDictionary.get(i).length()-1)
            anagram_words.add(sortedDictionary.get(i));
    }
}

Before I get started I have to say: Thank you for coming here. Mess of Java For Loops is almost an understatement. Now, let's get working, shall we?

In the below answer I assume that sortedDictionary and anagram_words is both of type List<String>. I also assume that anagramCharacters is of type char[].

Code duplication

Start by identifying which parts of the code looks similar, and create some methods to reduce code duplication

Code like this is repeated several times:

if(sortedDictionary.contains(""+anagramCharacters[l1]+anagramCharacters[l2]+anagramCharacters[l3]))
     anagram_words.add(""+anagramCharacters[l1]+anagramCharacters[l2]+anagramCharacters[l3]);

This can be extracted to a method: addWord() (Note that the below method is not optimized for what it is doing, it is mainly so that you are able to understand what it does better).

private void addWord(char... chars) {
    String string = "";
    for (char ch : chars) string += ch; // Create a strings of the chars
    if (sortedDictionary.contains(string))
        anagram_words.add(string);
}

So instead of repeatedly writing

if (sortedDictionary.contains(....)) anagram_words.add(...)

we simply call a method:

addWord(anagramCharacters[l1], anagramCharacters[l2], anagramCharacters[l3]);

Variables with index

Whenever you find yourself using variable names that contains numbered values, such as l1, l2, l3, l4, STOP and think "Would an array be useful here?". Creating an int[] letters is far more flexible than using the multiple variables l1, l2, l3

If all the l-variables are instead inside the letters array, some things can be simplified further at the moment:

This:

if(l4 != l1 && l4 != l2 && l4 != l3)

would become

if(letters[3] != letters[0] && letters[3] != letters[1] && letters[3] != letters[2])

This:

if(letters[3] != letters[0] && letters[3] != letters[1] && letters[3] != letters[2])

can become a method

private boolean lastLetterDifferent(int index) {
    for (int i = 0; i < index; i++) {
        if (letters[i] == letters[index]) return false;
    }
    return true;
}

The previous call we made to addWord only needs to contain the number of indexes to check instead of each and every char.

private void addWord(int numberOfChars) {
    String string = "";
    for (int i = 0; i < numberOfChars; i++) string += anagramCharacters[letters[i]]; // Create a strings of the chars
    if (sortedDictionary.contains(string))
        anagram_words.add(string);
}

So now that's done, what remains now?

After doing the above refactoring, here is what's left:

for(letters[0] = 0; letters[0] < 6; letters[0]++){
    for(letters[1] = 0; letters[1] < 6; letters[1]++) {
        if(lastLetterDifferent(1)) {
            for(letters[2] = 0; letters[2] < 6; letters[2]++) {
                if(letters[2] != letters[0] && letters[2] != letters[1]) {
                    addWord(3);
                    for(letters[3] = 0; letters[3] < 6; letters[3]++) {
                        if (lastLetterDifferent(3)) {
                            addWord(4);
                            for(letters[4] = 0; letters[4] < 6; letters[4]++){
                                if (lastLetterDifferent(4)) {
                                    addWord(5);
                                    for(letters[5] = 0; letters[5] < 6; letters[5]++){
                                        if (lastLetterDifferent(5)) {
                                            addWord(6);
                                        }
                                    }
                                }
                            }
                        }
                    }
                }
            }
        }
    }
}

That looks a lot cleaner, but how flexible is this? What if we want to use an 8-letter word, or a 12-letter word? NO, you should NOT continue this pattern and add more for-loops! So what other alternative is there? Recursive methods, my friend!

To do this, we need a couple of helper methods. You mentioned the number 720. It just so happens that 720 equals 6 factorial. So let's create a factorial method.

public static int fact(int x) {
    if (x < 0) throw new IllegalArgumentException("x cannot be negative: " + x);
    if (x <= 1) return 1;
    return x * fact(x - 1);
}

This is a recursive because it calls itself on some occasions. More precisely, when x >= 2. Recursive methods are fun, so let's create another one!

public static <E> List<E> permutationI(List<E> values, int iteration) {
    if (values.size() == 0)
        return new ArrayList<E>();
    List<E> list = new ArrayList<E>(values.size());
    int divisor = fact(values.size() - 1);
    int pos = iteration / divisor; // Pick an index depending on the iteration value
    list.add(values.remove((int)pos)); // Remove from one list and add to another
    list.addAll(permutationI(values, iteration % divisor)); // Call method recursively to get all the elements
    return list;
}

Every time this method is called, it picks an index in the values list depending on the value of iteration. Then it gets the element for that index from the values list, removes that element from the values list and adds it to the list called list (which will be the methods result. Then this method calls itself to add all the remaining elements for the current iteration.

Putting it together:

We will need one more utility method to convert from a List of characters to a String (again, this method is not fully optimized. There are plenty of faster ways to do this).

public String charListToString(List<Character> list) {
    String result = "";
    for (Character ch : list) {
        result += ch;
    }
    return result;
}

And now, the final touch to use all this recursive-ness:

List<Character> indexes = new ArrayList<Character>();
for (char ch : anagramCharacters)
    indexes.add(ch); // Build a list of the characters in our original word
int totalPermutations = fact(indexes.size());
for (int i = 0; i < totalPermutations; i++) {
    // Since permutationI modifies the list we send, we copy the list
    List<Character> newList = permutationI(new ArrayList<Character>(indexes), i);
    addWord(charListToString(newList)); // Call our method to add the word to the result list
}


Stack implementation using nodes Purpose

Implement a Stack with push, pop, and peek methods.

The push method should add an element to the "top" of the Stack.
The pop method should remove the "top" element from the Stack.
The peek method should return the "top" element of the Stack.

Approach

I decided to create a Node class to represent the elements in a Stack. The Node class would have a data field
and a previous field that would reference another Node. By having this previous field one can "chain" Nodes together.

For my Stack class, I decided to set the head (i.e. the Node at the "top" of the Stack) value to null on initialization.
I also decided to throw Exceptions when calling peek or pop for an empty Stack (i.e. when the head value is null).

I could see an argument to not throw Exceptions but rather return null for peek or nothing for pop (since it's void).
However, null is a valid value for an Object, and since a Stack is made up of Objects, I think returning
null when calling peek for an empty Stack could be confusing.

public class Node<T> {
  T data;
  Node<T> previous;

  public Node(T data, Node<T> previous) {
    this.data = data;
    this.previous = previous;
  }

  public T getData() {
    return data;
  }

  public Node<T> getPrevious() {
    return previous;
  }
}

public class Stack<T> {
  Node<T> head = null;

  public Stack() {
  }

  public T peek() {
    if (this.head == null) {
      throw new RuntimeException("Unable to peek element for an empty stack");
    }

    return this.head.getData();
  }

  public void push(T data) {
    this.head = new Node<>(data, this.head);
  }

  public void pop() {
    if (this.head == null) {
      throw new RuntimeException("Unable to remove element from an empty stack");
    }

    this.head = this.head.getPrevious();
  }

Don't throw RuntimeException: they are not specific, and hence difficult to catch.  (Otherwise, you might as well drop the null checks and let it fail naturally with NullPointerException, which is also a kind of RuntimeException.) You should make your own exception subclass, or use java.util.NoSuchElementException.

It would also make sense to support an isEmpty() method, to give the user a fair chance to avoid the exception.

Two spaces per level of indentation is too narrow and unconventional for my taste, and could also encourage excessive nesting.  Four or eight spaces would be acceptable.


Python __new__ and __init__ usage I'm trying to write a Python module to handle matrices. (I know about numpy, this is just for fun)

So far I have written a few classes, Matrix, Dim, and Vec. Matrix and Vec are both subclasses of Dim. When creating a matrix, one would first start out with a list of lists and they would create a matrix like:

startingList = [[1,2,3],[4,5,6],[7,8,9]]
myMatrix = matrix.Matrix(startingList)

This should create a Matrix. The created Matrix should contain multiple Dims all of the same length. Each of these Dims should contain multiple Dims all of the same length, etc. The last Dim, the one that contains numbers, should contain only numbers and should be a Vec instead of a Dim.

So far this works as I want it to. Here is what I have:

from numbers import Number

test2DMat = [[1,2,3],[4,5,6],[7,8,9]]
test3DMat = [[[1,2,3],[4,5,6],[7,8,9]],[[2,3,4],[5,6,7],[8,9,0]],[[9,8,7],[6,5,4],[3,2,1]]]

class Dim(list):
    def __new__(cls,inDim):
        # Make sure inDim is iterable
        iter(inDim)

        # If every item in inDim is a number create a Vec
        if all(isinstance(item,Number) for item in inDim):
            #return Vec(inDim)
            return Vec.__new__(cls,inDim)

        # Make sure every item in inDim is iterable
        try:
            for item in inDim: iter(item)
        except TypeError:
            raise TypeError('All items in a Dim must be iterable')

        # Make sure every item in inDim has the same length
        # or that there are zero items in the list
        if len(set(len(item) for item in inDim)) > 1:
            raise ValueError('All lists in a Dim must be the same length')

        # Actually create the Dim because it passed all the tests
        return list.__new__(cls,inDim)

    def __init__(self,inDim):
        inDim = map(Dim,inDim)
        list.__init__(self,inDim)

class Vec(Dim):
    def __new__(cls,inDim):
        if cls.__name__ not in [Vec.__name__,Dim.__name__]:
            newMat = list.__new__(Vec,inDim)
            newMat.__init__(inDim)
            return newMat
        return list.__new__(Vec,inDim)

    def __init__(self,inDim):
        list.__init__(self,inDim)

class Matrix(Dim):
    def __new__(cls,inMat):
        return Dim.__new__(cls,inMat)

    def __init__(self,inMat):
        super(Matrix,self).__init__(inMat)

This works, but as I have little experience overriding __new__() I'm fairly certain that this could be better. How can I improve this? Answers need not be specific to use of __new__ and __init__, but that is what I care most about.

First, the thing that was most obvious in your code was that you never used a space after a comma.

Second, you have the Matrix class where you override __new__ and __init__, however it would be work just fine if you'd use class Matrix(Dim): pass.

That's where I started to wonder why you define a Dim class and than inherit from it for Matrix and Vec, instead of code Matrix and subclass Vec from it. Any reason?

I would move the tests from Dim.__new__, except for the numbers test, to __init__. The iter test is not needed, you'll get an error from the list constructor or the for loop anyway.

One issue I see is that Matrix('abc') will result in endless recursion, because strings are iterable. Those are the places where I regret the absence of chr type in python :)

In Vec.__new__, I would let others customize the way they inherit from, why do you check if the class is a Vec or a Dim?

Last tip, when inheriting types like list or str, you have to override all the operator overloading methods if you plan on doing things like Vec((0, 1, 2)) * Vec((2, 3, 4))

Also:

[Vec.__name__,Dim.__name__] use tuples here, memory management

test2DMat = [[1,2,3],[4,5,6],[7,8,9]] I would break lines here


Processes and semaphores in C I have this question in my exam and I want to know if my answer is correct.

Consider the following program written in C language to implement a reader:

// to simplify the exercise, are not checked the return values of calls to the operating system

 int main(int argc, char* argv[]) {
     sem_t *sem_wrt;
     int fd, readcount;
     fd=shm_open("/readcount",O_RDWR|O_CREAT,S_IRUSR|S_IWUSR);
     ftruncate(fd, sizeof(int));
     readcount=mmap(0,sizeof(int),PROT_READ|PROT_WRITE,MAP_SHARED,fd,0);
     sem_wrt=sem_open("/write",O_CREAT ,0xFFFFFFFF,1);
     while(1) {
        readcount++;
        if (readcount==1) sem_wait (wrt);
        my_read();
        (readcount)--;
        if (readcount==0) sem_post (wrt);
     }
 }

After some time of using this program, it was found that the program only works properly when it is executed only by a process simultaneously.

Change the code of the program in order to provide a solution to this problem.

int main(int argc, char* argv[]) {
     sem_t *sem_wrt;
     int fd, readcount;
     fd=shm_open("/readcount",O_RDWR|O_CREAT,S_IRUSR|S_IWUSR);
     ftruncate(fd, sizeof(int));
     readcount=mmap(0,sizeof(int),PROT_READ|PROT_WRITE,MAP_SHARED,fd,0);
     sem_wrt=sem_open("/write",O_CREAT ,0xFFFFFFFF,1);
     //sem_mutex=sem_open("/mutex",O_CREAT ,0xFFFFFFFF,1);
     while(1) {
        //sem_wait(mutex);
        readcount++;
        if (readcount==1) sem_wait (wrt);
        //sem_post(mutex);
        my_read();
        //sem_wait(mutex);
        (readcount)--;
        if (readcount==0) sem_post (wrt);
        sem_post(mutex);
     }
 }

int fd, readcount;
// ...
readcount=mmap(0,sizeof(int),PROT_READ|PROT_WRITE,MAP_SHARED,fd,0);

If this is the code you're using, it probably gave you a warning which you ignored.  mmap returns void *.  You should change the declaration of readcount to int *.  Also, the size of the mapping should be a multiple of the page size.  I'd be sure to re-read the manual for mmap, carefully read any compiler warnings, and make sure your code checks for errors.

At any rate, if int *readcount; refers to shared memory, the following will not work:

++*readcount;

The problem is this line is conceptually broken into a few steps:

Fetch *readcount into register
Increment register
Store back into *readcount.

If multiple processes perform steps 1/2/3 in lockstep, and the initial value is 0, they will both store 1 back.

You could use a GCC extension for compare and swap to make an increment safe:

int fetch;
do
{
   fetch = *readcount;
} while (!__sync_bool_compare_and_swap(readcount, fetch, fetch+1));

However I'm not sure if this will work right if you do it on memory mapped from a file by mmap.  Keeping this kind of atomicity in a page fault handler seems complex and I'm not sure if the standard guarantees that it should work.