Data marshaling wrapper for a TCP server After a lot of research, I created this wrapper which I think fits very well to my needs. My project is a TCP server and I will be using this wrapper every time the server receives a packet. Basically, the wrapper will work marshaling raw byte[] into higher-level representation of the data as marshalable structs.

What do you think about this wrapper? Any hint on how I can improve it? I'm very concerned about performance.

public static class MyMarshal
{
    /// <summary>
    /// Marshals a raw buffer to a given marshalable struct.
    /// </summary>
    public static unsafe T GetStructure<T>(byte[] buffer) where T : struct
    {
        fixed (byte* bufferPin = buffer)
        {
            return (T)Marshal.PtrToStructure(new IntPtr(bufferPin), typeof(T));
        }
    }

    /// <summary>
    /// Marshals a given T instance into a raw buffer.
    /// </summary>
    public static unsafe byte[] GetBytes<T>(T obj) where T : struct
    {
        byte[] rawBuffer = new byte[Marshal.SizeOf(obj)];

        fixed (byte* rawBufferPin = rawBuffer)
        {
            Marshal.StructureToPtr<T>(obj, new IntPtr(rawBufferPin), false);
        }

        return rawBuffer;
    }

    /// <summary>
    /// Crates a zero-initialized marshaled instance of T
    /// </summary>
    public static unsafe T CreateEmpty<T>() where T : struct
    {
        int typeSize = Marshal.SizeOf(typeof(T));

        byte* rawBuffer = stackalloc byte[typeSize];

        for (int i = 0; i < typeSize; i++)
            rawBuffer[i] = 0;

        T zeroInited = (T)Marshal.PtrToStructure(new IntPtr(rawBuffer), typeof(T));

        return zeroInited;
    }
}

Summing up, the wrapper have 3 methods:

GetStructure (used right after I receive any valid packet to get the respective structure)
GetBytes (used when I want to send a packet to a client, converts a given struct value to a raw buffer to be send to the client)
CreateEmpty (used to create a zero-initialized instance of a given struct. This method is used when I have complex structures with non-blittable arrays, etc. The goal of this method is to have a generic zero-initializer method even to non-blittable structs - those with non-blittable arrays and/or strings).

I want to raise another questions too:

What do you think about this mixed use of unsafe code and the Marshal class? Is there any problem on using that way?
In the method MyMarshal.CreateEmpty, what do you think about the stackalloc usage? Some structures that will be initialized with this method will be a little big (usually something between 100 and 1024 bytes). Is there any downside or danger of allocating this length in the stack several times per seconds? I'm expecting to process something around 5k packet per second.

Also, he TCP server will work asynchronously using the async/await pattern, so I don't expect to deal with shared stated and other common problems to a multi-threaded application.

The wrapper seems pretty good to me.  The only change you might consider is that CreateWrapper really only needs to run once for each type; since these are structs it can return copies of the same empty object each time.  Something like this should work:

static class EmptyHolder<T> where T : struct
{
    public static T EmptyInstance;
    // assign EmptyInstance in static constructor
}

and then:

public static unsafe T CreateEmpty<T>() where T : struct
{
    return EmptyHolder<T>.EmptyInstance;
}

Of course there's a tradeoff of an uncollectable memory for each type you call this with.  And with anything performance-related, you'll need to measure to see if it delivers any benefit.  The original code should be pretty fast so maybe it's not significant.

There shouldn't be any danger in no matter how often you call stackalloc, by definition it's not possible for any two such calls to affect the same stack at the same time.  And there's no danger of any sort of recursion that would have more than 1 of these allocations on the same stack since the block is freed at the end of this function.


Searching for a better implementation to EnumBase In a former question on SO I described a problem with a construct called 'EnumBase'.

As I sad there, I am not involved when basic implementation happened. So I'm not sure why the thinks are a they are. I only know that the code was written in times of .Net 2 and 3.5. The derived classes are useed as DataSources in ASP.Net and WinForm GUIs and also for Switch-Statements.

Here the shorted main code of a typical derivates class:

public class Elements : EnumBase<int, Elements>
{
    public static readonly Elements Element1 = Create("Number 0", 0);
    public static readonly Elements Element2 = Create("Number 1", 1);

    // This empty constuctor is the solution of the previous question
    static Elements() {}

    private static Elements Create(string text, int value)
    {
        return new Elements() { text = text, value = value };
    }

    public static String GetElement(int id)
    {
        return BaseItemList.Single(v => v.Value == id).Text;
    }
}

And the EnumBase inself (the parameter dosentMatter in the empty constructor is no joke):

[Serializable()]
public class EnumBase<T, E> :  IEqualityComparer<E>
        where E : EnumBase<T, E>
{
    private static readonly List<E> list = new List<E>();
    protected string text;
    protected T value;

    protected static IList<E> BaseItemList
    {
        get
        {
            return list.Distinct(new EnumBase<T, E>(false)).ToList();
        }
    }

    protected EnumBase()
    {
        list.Add(this as E);
    }

    /// <summary>
    /// Constructor for distinct to avoid empty elements in the list
    /// </summary>
    private EnumBase(bool dosentMatter) {}

    public string Text
    {
        get { return text; }
    }

    public T Value
    {
        get { return value; }
    }

    #region IEqualityComparer<E> Member

    // ...

    #endregion
}

The switch statemaents are also not really nice (i'm not a friend of long switch an if-else-constructs, but bring polymorphism in the code is still a very very long way):

Switch
  Case Elements.Element1.value
  ...
  Case Elements.Element2.value
  ...

As you see, you have to use .value every time, which is a nice source for bugs if you forgot it :-/

All in all living with EnumBase leaves every time a strange feeling. There is a lot of code which is to write in every derived class again and again. And a lot of things can go wrong if you create or use an EnumBase derived-class. One the other hand it works but this can be 'the' argument.

From my point of view it would be nice if I can use more types form .Net-Framework instead of own programmed types. So the Question is, is there maybe something new in .net 4.0 (or even exists in .Net before 4.0) which make this implementation (in parts) obsolete?

I would be grateful for any hint.

So, extending my comment...

Enums are designed to represent a fixed number of states. This is their purpose. If that is what you need then nothing prevents you from using standart enum. If you want to specify items source as a collection of enum values - use IEnumerable<MyStandartEnum>, for example:

//collection of all enum values
var allVlues = Enum.GetValues(typeof(MyStandartEnum)).Cast<MyStandartEnum>();
//collection of elements which satisfy SomeCondition() (extension method)
var someValues = Enum.GetValues(typeof(MyStandartEnum)).Cast<MyStandartEnum>().Where(x => x.SomeCondition());

If you need some complicated logic added to enum, if you need to extend it, etc., that only means that enum is not the right abstraction in your case. Instead of trying to emulate enum's behaviour in some wierd abstract class, you should build a proper class to represent your data and then use generic collections (or implement your own).


An Express route, which renders a page, and makes use of socket.io This is the code for a basic endpoint in my express app routes folder. This firstly renders a page, and then on a continuous loop, renders information about some random "Breaking Bad" character.

The idea is to have the page load, and then every two seconds, the page updates automatically with the new character info. Then two seconds later, a new character replaces the one on the screen, and so on.

To do this, I'm making use of a convenient Breaking Bad REST-ful API

const express = require("express");
const app = require("../app");
const router = express.Router();
const axios = require("axios");

const socket = require("../socketioAPI");

router.get("/", function(req, res, next) {
    runCharacterUpdateInterval();
    res.render("index", { title: "Breaking Bad Page" });
});

const runCharacterUpdateInterval = () => {
    const updateTime = 2000;
    setInterval(() => {
        updatePageWithRandomCharacter();
    }, updateTime);
};

const updatePageWithRandomCharacter = () => {
    createCharacter().then(profile => {
        socket.push("breaking bad", profile);
    });
};

const createCharacter = async () => {
    const characterId = getRandomCharacterId();
    const bio = await queryAPI("characters/" + characterId);
    const { quote } = await queryAPI("quote/random?author=" + encode(bio.name));
    const { deathCount } = await queryAPI("death-count?name=" + encode(bio.name));
    return await { bio, quote, deathCount };
};

const encode = name => {
    return name.replace(/ /g, "+");
};

const getRandomCharacterId = () => {
    const totalChar = 54;
    const initalChar = 1;
    return Math.floor(Math.random() * Math.floor(totalChar)) + initalChar;
};

const queryAPI = async endpoint => {
    const api = await axios.get(`https://www.breakingbadapi.com/api/${endpoint}`);
    if (await api.data[0]) return await api.data[0];
    else return { name: [], quote: [], deathCount: [] };
};

module.exports = router;

In the past, my code has been reviewed as "spaghetti-like", and not clean. I've been trying to fix this, and so any pointers on the style, readability, or anything code related would be greatly appreciated.

Linting

The code reads well.

There is one convention you missed: use \s to indicate whitespace.

const encode = name => {
    return name.replace(/ /g, "+");
};

const encode = name => {
    return name.replace(/\s/g, "+");
};

And the else below is redundant.

if (await api.data[0]) return await api.data[0];
else return { name: [], quote: [], deathCount: [] };

if (await api.data[0]) return await api.data[0];
return { name: [], quote: [], deathCount: [] };


Simple cube that moves around the container with keys I made a basic cube that can move around of container and jump too. It's basic but I want to know if I can use better methods, better formatting, or shorten the code.

This my HTML:

and this is my Javascript code with a little jQuery:

var velocity = 2;
var acceleration = 0.002;
var gravity = 2;
var key_left = false;
var key_right = false;
var key_up = false;
window.addEventListener('keydown', handleKeyDown)
window.addEventListener('keyup', handleKeyUp)

function handleKeyDown(event) {
if (event.keyCode == 65) {
    key_left = true;
} else if (event.keyCode == 68) {
    key_right = true;
} else if (event.keyCode == 87) {
    key_up = true;
}
};

function handleKeyUp(event) {
if (event.keyCode == 65) {
    key_left = false;
    velocity = 2;
    acceleration = 0.002;
} else if (event.keyCode == 68) {
    key_right = false;
    velocity = 2;
    acceleration = 0.002;
} else if (event.keyCode == 87) {
    key_up = false;
}
};

setInterval(function () {
var move = parseFloat($(".box").css("marginRight"));
var moveLimt = (parseFloat($(".cube").css("width")) - 100)

if (key_left === true && move < moveLimt) {
    $(".box").css("marginRight", function () {
        move += (velocity += acceleration);
        return move.toString() + "px";
    });
} else if (key_right === true && move > 0) {
    $(".box").css("marginRight", function () {
        move -= (velocity += acceleration);
        return move.toString() + "px";
    });
}

var jump = parseFloat($(".cube").css("height"));

if (key_up === true) {
    $(".cube").css("height", function () {
        jump += (gravity);
        if (jump < 402) {
            return jump.toString() + "px";
        }
    });
} else if (key_up === false) {
    $(".cube").css("height", function () {
        jump -= (gravity);
        if (jump > 98) {
            return jump.toString() + "px";
        }
    });
}
});

It looks not bad at all. However, it could look more professional, and to my not very picky eye there seem to be errors.

HTML

There are no html-head-body tags, but every HTML document should have them. Nothing bad will happen, as modern browsers are really smart and will render your code correctly. These tags can be omitted by the HTML standards (e.g. WHATWG), but it is sort of a good style to have them always included. It will help whoever reads your code understand, that it is a document, not a part or template to be included somewhere else (if you didn't mean exactly this, of course).
The third DIV element is not closed, this is an error that will cause your HTML render not correctly. Use traditional

 or one-tag self-closing syntax:

Javascript

At the first glance, it is nice, but it doesn't look like Javascript. To look like Javascript, it should follow, or at least visually be close to one of coding conventions. Just a couple of things to be mentioned, that come to my mind:

No header. A header really helps you (some time later), or another person, to understand what it is, what it does, who and when wrote it, what language it is written in, and many other things you may want to let the future generations know. This is just a /**/-comment block on the top of the script.
Var-group. There are two main trends with defining the variables in JS - in the header / top of the script / block / function, or right before the place they are used. I personally prefer the first approach as more traditional, and besides that you assign initial values in your script, which makes it logic to do it as you did it. Var block can be grouped under the same var expression for better readability and more laconic and easier to transfer code. E.g.

var velocity = 2,
    acceleration = 0.002,
    //...
    key_up = false;

Separate the next logical paragraph (add listeners) with a blank line, don't keep it in the same paragraph with the "var" paragraph;
Function body is not indented well. The indentation should start with the first line of the function body. The rest is fine. E.g.

function example() {
    //first line indented
    //block opens: {
        //block indented
    //}
}
Good style is to put a block comment descriptor in front of every function to describe it.
Use strict comparison, instead of type-independent (e.g. a === b instead of a == b). This will help you avoid some issues later.
setInterval is missing the second argument - interval.
Defining function for a callback argument in the function call expression is not really a good practice, as it decreases readability and mantainability of your code and may lead to a "Callback hell", so I would recommend first define a named function and then specify it is an argument for the setInterval method. E.g.

function example() {
    //function body
}
setInterval(example, 1000);

And, in case if there are things that you don't know and which I mentined, but didn't reference here, you know how to search the web, right? =) Reading about coding conventions and a couple of articles about the good JS coding style may defenetely help.


Vector-based Null Cipher Continuing with my series of cipher programs, I hastily wrote a null cipher program, and I was wondering how it could be improved:

#include <iostream>
#include <vector>
#include <sstream>
#include <iterator>
#include <algorithm>

std::string getString();
std::vector<std::string> tokenize(const std::string & str);
std::vector<int> getPattern();

int main(){
    std::string input;
    std::vector<std::string> words;
    std::vector<int> pattern;
    do{
        if(words.size() != pattern.size()){
            std::cout << "\nError: Number of words must be equal to number of integers in pattern.";
        }
        input = getString();
        words = tokenize(input);
        pattern = getPattern();
    } while(words.size() != pattern.size());

    for(int i = 0; i < words.size(); ++i){
        if(words[i].length() > pattern[i]){
            std::string str = words[i];
            std::cout << str[pattern[i]];
        }
    }

    std::cout << '\n';
    return 0;
}

std::string getString(){
    std::cout << "Seperate words with spaces.\nInput string: ";
    std::string input;
    std::getline(std::cin, input);
    return input;
}

std::vector<std::string> tokenize(const std::string & str){
    std::stringstream ss(str);
    std::istream_iterator<std::string> begin(ss);
    std::istream_iterator<std::string> end;
    std::vector<std::string> words(begin, end);
    return words;
}

std::vector<int> getPattern(){
    std::string pattern;
    std::vector<int> intTokenizedPattern;
    bool validPattern;
    do{
        std::cout << "Seperate positive integers with spaces: ";
        std::getline(std::cin, pattern);
        std::vector<std::string> tokenizedPattern = tokenize(pattern);
        try{
            std::transform(tokenizedPattern.begin(), tokenizedPattern.end(),
                std::back_inserter(intTokenizedPattern),
                [](const std::string & str){ return (std::stoi(str) > 0 ? std::stoi(str) - 1 : throw "Input less than one");});
            validPattern = true;
        } catch(...){
            tokenizedPattern.clear();
            intTokenizedPattern.clear();
            validPattern = false;
        }
    } while(!validPattern);

    return intTokenizedPattern;
}

Here is an example input/output:

Seperate words with spaces.
Input string: Susan says Gail lies. Matt lets Susan feel jovial. Elated angry?
Seperate positive integers with spaces: 1 2 3 1 2 3 1 2 3 1 2
SailatSevEn

In the output above, it is deciphered to say "sail at seven" (null ciphers are not used in the modern age of cryptography).

In my program, I verify that the number of elements in the words and pattern vectors is equal. I also verify that the elements in the pattern vector are all actual integers (which is done in the try-catch loop).

I found it interesting that at this time this is the only null cipher program on Code Review, so it could also prove helpful to future viewers.

How can this code be improved?

This seems like a waste of time:

std::vector<std::string> tokenize(const std::string & str){
    std::stringstream ss(str);
    std::istream_iterator<std::string> begin(ss);
    std::istream_iterator<std::string> end;
    std::vector<std::string> words(begin, end);
    return words;
}

The result is used here:

std::vector<std::string> tokenizedPattern = tokenize(pattern);
std::transform(tokenizedPattern.begin(), tokenizedPattern.end(),
               std::back_inserter(intTokenizedPattern),
               [](const std::string & str){ return stuff;});

You use an iterator to copy it into a vector so you can get the iterator to use in a standard algorithm. Why not just use the algorithm with the original iterator.

std::stringstream streamPattern(pattern);
std::transform(std::istream_iterator<std::string>{streamPattern},
               std::istream_iterator<std::string>{},
               std::back_inserter(intTokenizedPattern),
               [](const std::string & str){ return stuff;});


Simple exchange calculator in Java - structure and tests I am trying to learn how to correctly structure my projects and write a good, clean code. I would also like to learn how to write good tests. Can you please review it and give me some tips and tricks how to make that better?

The application is calculating the exchange from the bill for lemonade. Lemonade costs 5$ and client may pay with 5$, 10$ or 20$. At the beginning there is no change available.

Project structure

My project structure is as follows:

LemonadeChange (project) with src package and 4 packages inside:

app (contains Main)
calculatorImp (contains CalculatorLongImp and CalculatorShortImp)
calculatorInterface (contains CalculatorInterface)
test (contains CalculatorTest)

Classes

Main

public class Main {

    public static void main(int[] args) {
        CalculatorInterface calc = new CalculatorLongImp();
        calc.lemonadeChange(args);
    }
}

CalculatorInterface

public interface CalculatorInterface {

    public boolean lemonadeChange(int[] bills);
}

CalculatorLongImp

public class CalculatorLongImp implements CalculatorInterface {
    private int five = 0;
    private int ten = 0;
    int acc = 0;

    public boolean lemonadeChange(int[] bills) {
        for (int bill : bills) {
            if(bill == 5) {
                five++;
            }
            else if(bill == 10) {
                five--;
                ten++;
            }
            else {
                if(ten >= 1 && five >= 1) {
                    ten--;
                    five--;
                }
                else if (five >= 3) {
                    five -= 3;
                }
                else {
                    acc++;
                }
            }
        }

        if(acc == 0) {
            return true;
        }
        else {
            return false;
        }
    }
}

CalculatorTest

public class CalculatorTest {

    @Test
    public void testAllChangeShouldBePossibleToGive() {
        int[] bills = {5,5,5,10,20};
        CalculatorInterface calc = new CalculatorLongImp();
        assertTrue(calc.lemonadeChange(bills));
        calc = new CalculatorShortImp();
        assertTrue(calc.lemonadeChange(bills));
    }

    @Test
    public void testAllChangeShouldNotBePossibleToGive() {
        int[] bills = {20};
        CalculatorInterface calc = new CalculatorLongImp();
        assertFalse(calc.lemonadeChange(bills));
        calc = new CalculatorShortImp();
        assertFalse(calc.lemonadeChange(bills));
    }

    @Test
    public void testAllChangeShouldNotBePossibleToGiveSecond() {
        int[] bills = {10, 20};
        CalculatorInterface calc = new CalculatorLongImp();
        assertFalse(calc.lemonadeChange(bills));
        calc = new CalculatorShortImp();
        assertFalse(calc.lemonadeChange(bills));
    }

    @Test
    public void testAllChangeShouldBePossibleToGiveSecond() {
        int[] bills = {5, 5, 10, 20};
        CalculatorInterface calc = new CalculatorLongImp();
        assertTrue(calc.lemonadeChange(bills));
        calc = new CalculatorShortImp();
        assertTrue(calc.lemonadeChange(bills));
    }

    @Test
    public void testAllChangeShouldNotBePossibleToGiveThird() {
        int[] bills = {10, 20, 5, 5};
        CalculatorInterface calc = new CalculatorLongImp();
        assertFalse(calc.lemonadeChange(bills));
        calc = new CalculatorShortImp();
        assertFalse(calc.lemonadeChange(bills));
    }

    @Test
    public void testAllChangeShouldBePossibleToGiveThird() {
        int[] bills = {5,5,5,5,20};
        CalculatorInterface calc = new CalculatorLongImp();
        assertTrue(calc.lemonadeChange(bills));
        calc = new CalculatorShortImp();
        assertTrue(calc.lemonadeChange(bills));
    }
}

How would you improve that? Any tips and tricks welcomed!

Please let me know if you want to see the task that it solves - that is one of the tasks on LeetCode, so I can post it if that makes review easier.

First off, the implementation of lemonadeChange is pretty complicated. I get worried whenever I see a bunch of nested if statements - these always lead to bugs. Indeed, try adding another test case where you only pass $10 bills, I think it will erroneously return true. It's generally a good idea to limit the complexity of any individual function, take a look at the concept of Cyclomatic Complexity: https://help.semmle.com/wiki/plugins/servlet/mobile?contentId=29393453#content/view/29393453

Also, if you call lemonadeChange more than once on the same instance of CalculatorLongImp then the values of five/ten/acc will persist between calls, giving you odd results. Generally, you'd say this function has "side effects", and it's often a good idea to avoid these where possible.

It's good that you've included tests. I would separate these out so each test only covers a single implementation - specific tests covering a single function on a single class make it easier to see exactly what's broken.

Finally, I find the naming a bit confusing. What does acc do? What's the difference between CalculatorLongImp and CalculatorShortImp? Imagine someone else will be reading your code tomorrow and trying to understand what it does. Naming things clearly will make it much easier for someone else (or yourself in the future!) to read and understand your code.


My method of escaping MySQL strings in C++ This is how I'm currently escaping MySQL strings in C++:

std::string escapeSQL(const char* dataIn)
{
   if (!MySQL_Database_Connection__global->host)
      int a = NULL;

   if (dataIn)
   {
      std::size_t dataInLen = strlen(dataIn);
      std::string to(((dataInLen * 2) + 1), '\0');
      mysql_real_escape_string(MySQL_Database_Connection__global, &to[0], dataIn, dataInLen);
      return (to);
   }
   else
   {
      return ("");
   }
}

char query[1024] = {'\0'};
snprintf(query, sizeof(query), "SELECT * FROM database.table WHERE column = '%s';", escapeSQL(userTypedArgument).c_str());

It seems to be doing the job, but I'm curious if I can get a review of my escapeSQL() function for any pitfalls I may have overlooked.

if (!MySQL_Database_Connection__global->host)
    int a = NULL;

What is the purpose of this if statement? It only contains one unused declaration.

if (dataIn) ... return ("");

I try to avoid adding defensive behavior like this. Returning empty string for NULL argument seems like a good idea and might even save your program from crashing. But, NULL argument indicates your program is already in invalid state and it's unlikely to function properly. Assertion or exception with clear indication what's wrong would save you a lot of debugging.

std::size_t dataInLen = strlen(dataIn);
escapeSQL(userTypedArgument).c_str()

Consider using just std::string or just C strings. Or at least minimize those awkward conversions back and forth. They can only lead to bugs like this one:

mysql_real_escape_string(MySQL_Database_Connection__global, &to[0], dataIn, dataInLen);

You are creating null-terminated C string inside std::string. std::string ignores null characters. to.length() will always return dataInLen*2+1. Any method other than c_str() will result in unexpected behavior.

char query[1024] = {'\0'};
snprintf(query, sizeof(query), "SELECT * FROM database.table WHERE column = '%s';", escapeSQL(userTypedArgument).c_str());

This is exploitable. You should check if the query exceeds 1023 characters and fail in a safe way (or just use std::string). Currently user can remove part of your query by entering a long string. For example:

"DELETE * FROM table WHERE name='long string ...' AND owner=currentUser"
                                                  ^ 1024th character


Python scanner for the first free port in a range I have a distributed application (YARN), which runs a WebApp.

This application use a default port to start (8008), before I start I need to check if port is in use.

A container may run in the same virtual machine, hence port may be in use.
(Max I have 4 containers in WebApp).

I created the following code which seem to work, but want to see if there are some clean ups/improvements suggested.

def port_in_use(port):
    sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
    result = sock.connect_ex(('127.0.0.1', port))
    if result == 0:
      return True
    else:
      return False

def start_dashboard():
    base_port = os.getenv('DASHBOARD_PORT_ENV_VAR', 8008)
    scan_ports = True
    attempts = 0
    max_attempts = 10
    while(scan_ports and attempts <= max_attempts):
      if port_in_use(base_port):
         base_port += 1
         attempts += 1
      else:
         scan_ports = False
    if attempts == max_attempts:
       raise IOError('Port in use')
    dashboard.configure(port=base_port)
    dashboard.launch()

Your code has some incorrect assumptions.

an application may listen on a specific address/port combination; 127.0.0.1:port can be available while *:port is not.
an application may bind a port without listening.  Connects will fail, but so will your own bind.
a firewall or other mechanism can interfere with connections, generating false positives in your scan.

The reliable approach is to bind the port, just as your dashboard will, and then release it.

result = sock.bind(('', port))
sock.close()

You'll need to catch the exception and this is a good opportunity to move the whole thing into a function.   That will make the start_dashboard logic cleaner and get rid of boolean loop-terminator scan_ports.    Just exit the loop by returning the answer.

def next_free_port( port=1024, max_port=65535 ):
    sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
    while port <= max_port:
        try:
            sock.bind(('', port))
            sock.close()
            return port
        except OSError:
            port += 1
    raise IOError('no free ports')

def start_dashboard():

    # pass optional second parameter "max_port" here, else scan until a free one is found
    port = next_free_port( os.getenv('DASHBOARD_PORT_ENV_VAR', 8008) )

    dashboard.configure(port=port)
    dashboard.launch()

You can use netcat to make ports in-use for testing: nc -l -p 9999 will listen on port 9999; press control-C to end it.


Organizing a Python list of lists as function of number of occurances I am having a Python list of lists, that I get as an output of python-igraph (membership to clusters). The integers represent an association of a vertex to a certain cluster at a certain subgraph. I would like to organise this list in a way that the biggest cluster in the whole (unconnected) graph is getting the lowest integer - 0, and then the smaller the cluster the higher the integer that is assigned to it. For now I do it in a very cumbersome way:

import numpy as np
def organize_in_decreasing_order(membership_list):
        conlist = np.concatenate(membership_list)
        count=np.bincount(conlist)
        ordered_list = []
        for i in range(len(count)):
            ordered_list.append(np.argmax(count))
            count[np.argmax(count)]=0
#        print ordered_list
        maxvalue = max(conlist)
        locations = []
        for number in range(maxvalue+1):
            locs = []
            for i in range(len(membership_list)):
                for j in range(len(membership_list[i])):
                    if membership_list[i][j]==number:
                        locs.append((i,j))
            locations.append(locs)
        for i,value in enumerate(ordered_list):
            for location in locations[value]:
                membership_list[location[0]][location[1]]=i
        return membership_list

You can check it over the following list:

memlist =
[[0,
  1,
  2,
  1,
  3,
  3,
  1,
  4,
  1,
  4,
  5,
  5,
  4,
  4,
  5,
  5,
  3,
  6,
  7,
  5,
  0,
  2,
  5,
  6,
  7,
  4,
  4,
  5,
  1,
  1,
  2,
  2,
  8,
  2,
  4,
  2,
  2,
  2,
  7,
  5,
  2,
  2,
  5,
  2,
  2,
  7,
  7,
  2,
  5,
  5,
  2,
  5,
  3,
  8,
  8,
  2,
  5,
  5,
  7,
  4,
  8,
  8,
  8,
  4,
  8,
  5,
  8,
  8,
  6,
  5,
  5,
  5,
  4,
  2,
  4,
  7,
  4,
  7,
  5,
  8,
  5,
  5,
  5,
  4,
  5,
  5,
  5,
  2,
  5,
  4,
  4],
 [9, 9, 9],
 [10],
 [11],
 [12]]

If you have an idea how to do it more efficiently I will be happy to hear!

Try this code:

def organize_in_decreasing_order(membership_list):
    conlist = np.concatenate(membership_list)
    count = np.bincount(conlist)
    ordered_list = np.argsort(count)[::-1]
    lookup = np.argsort(ordered_list)
    return [[lookup[i] for i in sublist] for sublist in membership_list]

Be aware that the function above returns the organized list, but - in contrast to your implementation - does not change the input argument. You should also be aware that when count has repeated elements, the lists yielded by both implementations may not coincide. The reason for the different behaviour is that ties are dealt with differently.


VBA userform loop to gather numbers from cells on an Excel sheet I am working on a userform and have everything working the way it should but it's taking a little longer than I would like. It's looping through 42 labels for each For statement and there are six of them.

In an effort to learn how to code a little more efficient, could someone please review my code and show me some faster ways to get the job done?

Private Sub Find()
Application.ScreenUpdating = False
Dim i As Integer, k As Integer
k = 1

i = 1

For i = i To 42
C1 = "C1_" & i

Me.Controls(C1) = Sheet14.Range("I" & k).Value

If Me.Controls(C1) = "0" Then
Me.Controls(C1).ForeColor = &H8000000F

End If
k = k + 1

Next

k = 1

i = 1

For i = i To 42
C2 = "C2_" & i
Me.Controls(C2) = Sheet14.Range("J" & k).Value
If Me.Controls(C2) = "0" Then
Me.Controls(C2).ForeColor = &H8000000F

End If
k = k + 1

Next
'
k = 1

i = 1

For i = i To 42
C3 = "C3_" & i
Me.Controls(C3) = Sheet14.Range("K" & k).Value
If Me.Controls(C3) = "0" Then
Me.Controls(C3).ForeColor = &H8000000F

End If
k = k + 1

Next
k = 1

i = 1

For i = i To 42
C4 = "CL4_" & i
Me.Controls(C4) = Sheet14.Range("L" & k).Value
If Me.Controls(C4) = "0" Then
Me.Controls(C4).ForeColor = &H8000000F

End If
k = k + 1

Next

k = 1

i = 1

For i = i To 42
C5 = "C5_" & i
Me.Controls(C5) = Sheet14.Range("M" & k).Value
If Me.Controls(C5) = "0" Then
Me.Controls(C5).ForeColor = &H8000000F

End If
k = k + 1

Next

k = 1

i = 1

For i = i To 42
C6 = "C6_" & i
Me.Controls(C6) = Sheet14.Range("N" & k).Value
If Me.Controls(C6) = "0" Then
Me.Controls(C6).ForeColor = &H8000000F

End If
k = k + 1

Next
Application.ScreenUpdating = True
End Sub

What is your sub name? Find? That's a built in application function. And it doesn't explain at all what you're doing.
Also your indenting and spacing is way off base. Even if allowing for extra space, your function should look like this -
Option Explicit
Private Sub PopulateLabels()
    Application.ScreenUpdating = False
    Dim i As Integer, k As Integer

    k = 1
    i = 1

    For i = i To 42
        C1 = "C1_" & i
        Me.Controls(C1) = Sheet14.Range("I" & k).Value
        If Me.Controls(C1) = "0" Then
            Me.Controls(C1).ForeColor = &H8000000F
        End If
        k = k + 1
    Next

    k = 1
    i = 1

    For i = i To 42
        C2 = "C2_" & i
        Me.Controls(C2) = Sheet14.Range("J" & k).Value
        If Me.Controls(C2) = "0" Then
            Me.Controls(C2).ForeColor = &H8000000F
        End If
        k = k + 1
    Next

    k = 1
    i = 1

    For i = i To 42
        C3 = "C3_" & i
        Me.Controls(C3) = Sheet14.Range("K" & k).Value
        If Me.Controls(C3) = "0" Then
            Me.Controls(C3).ForeColor = &H8000000F
        End If
        k = k + 1
    Next

    k = 1
    i = 1

    For i = i To 42
        C4 = "CL4_" & i
        Me.Controls(C4) = Sheet14.Range("L" & k).Value
        If Me.Controls(C4) = "0" Then
            Me.Controls(C4).ForeColor = &H8000000F
        End If
        k = k + 1
    Next

    k = 1
    i = 1

    For i = i To 42
        C5 = "C5_" & i
        Me.Controls(C5) = Sheet14.Range("M" & k).Value
        If Me.Controls(C5) = "0" Then
            Me.Controls(C5).ForeColor = &H8000000F
        End If
        k = k + 1
    Next

    k = 1
    i = 1

    For i = i To 42
        C6 = "C6_" & i
        Me.Controls(C6) = Sheet14.Range("N" & k).Value
        If Me.Controls(C6) = "0" Then
            Me.Controls(C6).ForeColor = &H8000000F
        End If
        k = k + 1
    Next

    Application.ScreenUpdating = True
End Sub

I renamed it and added indents. Now lets move on to the code.

Variables
&H8000000F is a constant for vbButtonFace. I would at least give it a constant instead of repeating it over and over
 Const FACE_COLOR as Long = vbButtonFace

Your i and k variables are telling me nothing about what they are. Give me some context. Also, you haven't declared variables
C1
C2
C3
C4
C5
C6

What are those? Also, why are they being set to different strings?
C1 = "C1_" & i
C2 = "C2_" & i
C3 = "C3_" & i
C4 = "CL4_" & i 'Where did that L come from
C5 = "C5_" & i 'Where did that L go
C6 = "C6_" & i

I can't imagine what they are, or why they need to be set 42 times. Also it looks like you iterate the k the same number of times as the i, so why do you need the k?

Refactoring
You continually set i and k to 1, then iterate i from 1 to 42, increasing k by one each time. i and k are the same and they are always the same set of numbers.
Not only that, but you're doing the same exact thing 6 times -
For i = 1 To 42
    'something is something and i
    'something is Sheet14, letter and i
    'Check if it's 0 and change color
Next i

As far as I can tell, you change the Control string and the column letter. I'm sure we can create a procedure that takes arguments for this -
Sub DoTheThing(ByVal controlNameBase As String, ByVal columnNumber As Long)
    Dim i As Long
    Dim controlName As String
    For i = 1 To 42
        controlName = controlNameBase & i
        Me.Controls(controlName) = sheet14.Cells(i, columnNumber)
        If Me.Controls(controlName) = 0 Then Me.Controls.ForeColor = vbButtonFace
    Next
End Sub

Now we have -
Option Explicit
Private Sub PopulateLabels()
    Application.ScreenUpdating = False

    Const CONTROL_STRING1 As String = "C1_"
    Const CONTROL_STRING2 As String = "C2_"
    Const CONTROL_STRING3 As String = "C3_"
    Const CONTROL_STRING4 As String = "CL4_"
    Const CONTROL_STRING5 As String = "C5_"
    Const CONTROL_STRING6 As String = "C6_"
    Const CONTROL_COLUMN1 As Long = 9
    Const CONTROL_COLUMN2 As Long = 10
    Const CONTROL_COLUMN3 As Long = 11
    Const CONTROL_COLUMN4 As Long = 12
    Const CONTROL_COLUMN5 As Long = 13
    Const CONTROL_COLUMN6 As Long = 14

    DoTheThing CONTROL_STRING1, CONTROL_COLUMN1
    DoTheThing CONTROL_STRING2, CONTROL_COLUMN2
    DoTheThing CONTROL_STRING3, CONTROL_COLUMN3
    DoTheThing CONTROL_STRING4, CONTROL_COLUMN4
    DoTheThing CONTROL_STRING5, CONTROL_COLUMN5
    DoTheThing CONTROL_STRING6, CONTROL_COLUMN6

    Application.ScreenUpdating = True
End Sub

Sub DoTheThing(ByVal controlNameBase As String, ByVal columnNumber As Long)
    Dim i As Long
    Dim controlName As String
    For i = 1 To 42
        controlName = controlNameBase & i
        Me.Controls(controlName) = sheet14.Cells(i, columnNumber)
        If Me.Controls(controlName) = 0 Then Me.Controls.ForeColor = vbButtonFace
    Next
End Sub

I bet we could refactor it some more..
Option Explicit
Private Sub PopulateLabels()
    Application.ScreenUpdating = False
    Const BASE_STRING As String = "C"
    Const ALTERNATE_STRING As String = "CL"
    Const END_STRING As String = "_"
    Dim i As Long
    Dim controlString As String
    For i = 1 To 6
        If i = 4 Then
            controlString = ALTERNATE_STRING & i & END_STRING
        Else
            controlString = BASE_STRING & i & END_STRING
        End If
        DoTheThing controlString, i + 8
    Next
    Application.ScreenUpdating = True
End Sub

Now the whole thing is 27 lines. At first it was 95.

Performance
You said your loops are taking a long time. That's because you're accessing the sheet every time you loop. Bring those values into an array instead.
Sub DoTheThing(ByVal controlNameBase As String, ByVal columnNumber As Long)
    Dim i As Long
    Dim controlName As String
    Dim lookUpValues As Variant
    lookUpValues = GetArray(columnNumber)
    For i = 1 To 42
        controlName = controlNameBase & i
        Me.Controls(controlName) = lookUpValues(i,1)
        If Me.Controls(controlName) = 0 Then Me.Controls(controlName).ForeColor = vbButtonFace
    Next
End Sub

Private Function GetArray(ByVal columnNumber As Long) As Variant
    GetArray = Sheet14.Range(Sheet14.Cells(1, columnNumber), Sheet14.Cells(42, columnNumber))
End Function

Conclusion
So now instead of looping through the sheet constantly, you just get all the values you need into an array and loop through that. It's much faster than going to the sheet. We also eliminated 5 of the 6 loops by creating a procedure. It also appears much cleaner and less unwieldy.


Invoke-WebRequest in a loop until NotFound I'm using SAP's Raylight RESTful SDK to get a list of universes from the BusinessObject Enterprise repository.

To get all of the universes, the request needs to be called repeatedly, incrementing the offset parameter until the request returns a Not Found [404] error.

Current approach:

$baseUrl = 'http://<server>:6405/biprws/raylight/v1'

$headers = @{}
$headers.Add("Accept", "application/json")
$headers.Add("Content-Type", "application/json")
$headers.Add("x-sap-logontoken", $Token)

$offset = 0
$limit = 50

try {
    do {

        $Path = "/universes?offset=$offset&limit=$limit"

        $response = Invoke-WebRequest "$baseUrl$Path" -Headers $headers
        ($response | ConvertFrom-Json).universes.universe

        $offset += $limit

    } until ( $response.StatusCode -eq [System.Net.HttpStatusCode]::NotFound )
}
catch {
    # catches 404 too
    Write-Debug "StatusCode: $($_.Exception.Response.StatusCode.Value__)"
}

While the code returns the expected results, I not sure that I like how the code will always exit via the catch.  I could remove the try/catch, but that will leave the code exposed to other errors.

Is there a way to improve this?

You appear to have some incorrect logic. The until expression will never get evaluated in the case of a 404 exception. It will get skipped over. So it's kind of pointless.

So let's replace the repeat ... until with a while ($true). That is, an infinite loop. This works the same as what you already had, but the logic is clearer. I added a comment at the top, too, to let the reader know that a 404 exception will break the loop:

while ($true) {
    # Will throw 404 exception when finished.

    $Path = "/universes?offset=$offset&limit=$limit"

    $response = Invoke-WebRequest "$baseUrl$Path" -Headers $headers
    ($response | ConvertFrom-Json).universes.universe

    $offset += $limit
}

Now, in general it's not good to use exceptions for normal program control flow. They should normally be used for exceptional cases only. But sometimes—such as in this case it would seem—we do have to use them. In such cases the usual pattern is to catch the exception and then examine it to see if it's a normal control flow exception or if it's a true exception, and respond accordingly.

So let's do that. If we catch a 404 exception, then we will just ignore it. If it's anything else, then we will rethrow it:

try {
    while ($true) {
        # Will throw 404 exception when finished.

        $Path = "/universes?offset=$offset&limit=$limit"

        $response = Invoke-WebRequest "$baseUrl$Path" -Headers $headers
        ($response | ConvertFrom-Json).universes.universe

        $offset += $limit
    }
}
catch [System.Net.WebException] {
    $statusCode = $_.Exception.Response.StatusCode.Value__

    if ($statusCode -eq 404) {
        # Normal end case. Do nothing.
    }
    else {
        throw
    }
}


KQuery SPOJ problem, using merge sort segment tree Problem : KQUERY

Summary: For each query consisting of \$(i, j, k)\$, return the number of elements greater than \$k\$ in the subsequence \$a_i, a_{i+1}, …, a_j\$.

My code has complexity of \$O(n\log^2{n})\$. I am only interested in segment tree based solution of Kquery. Please let me know why am I getting TLE using merge sort segment tree. Is it possible to do some optimization to get my code accepted or is this approach totally wrong. Here I have used merge sort segment tree which has sorted vector as its node and to query particular range it creates the sorted vector of elements for the given query.

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

const int N=30005;
int arr[N];
vector<int> seg[4*N];

void build(int low,int high,int node)
{
    if(low>high)
    return;
    if(low == high)
    {
        seg[node].push_back(arr[low]);
        return;
    }
    int mid=low+high>>1;
    build(low,mid,2*node+1);
    build(mid+1,high,2*node+2);
    merge(seg[2*node+1].begin(),seg[2*node+1].end(),seg[2*node+2].begin(),seg[2*node+2].end(),back_inserter(seg[node]));
}

vector<int> query(int low,int high,int lq,int hq,int k,int node)
{
    vector<int> ans;
    if(low>high || low>hq || high<lq)
    return ans;
    if(lq<=low && high<=hq)
    {
        return seg[node];
    }
    vector<int> left,right;
    int mid=low+high>>1;
    left = query(low,mid,lq,hq,k,2*node+1);
    right = query(mid+1,high,lq,hq,k,2*node+2);
    merge(left.begin(),left.end(),right.begin(),right.end(),back_inserter(ans));
    return ans;
}
int main(){
    int n;
    scanf("%d",&n);
    register int i;
    for(i=0;i<n;i++)
    scanf("%d",&arr[i]);
    register int q;
    scanf("%d",&q);
    int x,y,k;
    build(0,n-1,0);
    while(q--)
    {
        scanf("%d %d %d",&x,&y,&k);
        vector<int> ans=query(0,n-1,x-1,y-1,k,0);
        int point=upper_bound(ans.begin(),ans.end(),k)-ans.begin();
        printf("%d\n",ans.size()-point);
    }
    return 0;
}

Your biggest issue here is that you are building a new list for every single query. Each of these lists can have size up to \$N\$ (since the query could be over the entire universe), so building this list takes time \$O(N)\$ in the worst case (this is true even though it is built from logarithmically many smaller lists). This makes the total runtime of your algorithm \$O(N\log^2 N + QN)\$, and \$O(QN)\$ is way too large to run in time for the parameters of this problem.

Of course, the solution to this issue is that you don't really need to build the list for each query; you just need to count how many elements are larger than k. Since the list for each query is just the union of logarithmically many sorted lists in your segment tree, just count this quantity for each of those lists and sum this up. This reduces your time complexity to \$O(N\log^2 N + Q\log^2 N)\$ (there are \$O(\log N)\$ lists, and it takes \$O(\log N)\$ time to binary search each one), which should run in time.

If your code still times out after making this change, I would recommend rewriting the code for build and query iteratively instead of recursively; this can sometimes speed things up a bit (however we're no longer in the realm of asymptotic improvements).

Some other code things:

I might be wrong, but I think it's generally accepted that the register keyword in C++ doesn't really do much anymore; compilers are way better than people at figuring out which variables should be stored in registers, so you're not going to get any speedup by explicitly declaring this.
Your query method accepts k as a parameter, but you don't actually use k anywhere (short of passing it along in recursive calls). That said, given the above answer, you probably should be using k in the query method.
I guess it's not super necessary for short and quick solutions to contest problems like this, but I think your code would benefit a lot from a bit more whitespace and consistent indentation (I think it would make it a lot easier to spot bugs as well). One of the things that bugs me the most about your code is how you write for(i=0;i<n;i++) and scanf("%d", &arr[n]) on two consecutive lines at the same indentation; it makes it very unclear that the second line is part of the for loop.


Displaying grades and calculating average grade I'd love to improve this code. I know I should put a lot of stuff in another class but I have no idea how.

import java.util.Scanner;

public class UserInteraction {

    public static void main(String[] args) {

        Scanner scan = new Scanner(System.in);
        int choice = 0;
        String[] subjects = new String[10];
        int grades[] = new int[10];
        double sum = 0.0;

    do
    {
        System.out.println("1. Enter a course name and a grade");
        System.out.println("2. Display all grades");
        System.out.println("3. Calculate the average grade");
        System.out.println("4. Exit program");

        choice = scan.nextInt();

        if ( choice == 1 )
        {
            Scanner scansubjects = new Scanner(System.in);
            Scanner scangrades = new Scanner(System.in);

            System.out.println("Enter 10 subjects and their corresponding grades:");
            System.out.println();

            int i = 0;

            for( i = 0; i < 10; i++ )
            {
                System.out.println("Subject:");

                String temp = scansubjects.nextLine();
                subjects[i] = temp.toLowerCase();

                System.out.println("Grade:");

                grades[i] = scangrades.nextInt();

                if( i == ( subjects.length - 1 ) )
                {
                    System.out.println("Thank you!");
                    System.out.println();
                }
            }
        }

        if ( choice == 2 )
        {
            System.out.println("Subjects" + "\tGrades");
            System.out.println("---------------------");

            for(int p = 0; p < subjects.length; p++)
            {

                System.out.println(subjects[p] + "\t" + "\t" + grades[p]);
            }
        }

        if ( choice == 3 )
        {
              System.out.println("Total of grades: " + getSum(grades));
              System.out.println("Count of grades: " + grades.length);
              System.out.println("Average of grades: " + getAverage(grades));
              System.out.println();
        }

    } while ( choice != 4);

}

    public static double getAverage(int[] array)
    {
        int sum = 0;
        for(int i : array) sum += i;
        return ((double) sum)/array.length;
    }

    public static double getSum(int[] array)
    {
        int sum = 0;
        for (int i : array)
        {
         sum += i;
        }
        return sum;
    }

}

Let's go through refactoring your code step by step.

Get (almost) everything out of main()

main() should consist of the fewest lines to launch the program.  In this case you only need one.

    new UserInteraction().run();

Or, for clarity, you can break that into two lines.

    UserInteraction ui = new UserInteraction();
    ui.run();

You would then put everything from main() into a method called run().  The actually name of the new method is not important.  You can use start() if you like.

This step may seem almost silly at first, but it is very important.  First, all your methods and variables stop needing to be static.  For a beginner, you should avoid static as much as possible.  Second, it starts you thinking in an object oriented way (as opposed to procedurally).  Here's the first consequence of getting everything out of main():

    private double getAverage(int[] array)
    ...
    private double getSum(int[] array)

Notice that the methods are no longer static.  This is a good thing.  I also changed the accessibility to private.  Like static, a member should be private unless there is a good reason for it not to be.

Methods should do one thing well

So now we have a monolithic run() method doing all sorts of this.  Let's make run() only do one thing well.  We're shooting for something like this:

    private void run() {
        int choice = 0;

        do {
            showMenu();
            choice = scan.nextInt();

            switch (choice) {
            case 1:
                enterCourses();
                break;
            case 2:
                displayGrades();
                break;
            case 3:
                displayAverageGrade();
            }
        } while (choice != 4);
    }

Now run() only concerns itself with one thing: running the basic flow of the program.

Declare variables in the tightest scope possible

So where are all the variable declarations?  In general, you want to declare your variables "as close as you can."  Variable scope is a little tricky but for a beginner, try to declare your variables in the same block as their used.  (A block is roughly the code between braces {}).  choice needs to be declared where it is because it's used outside the switch block.

You only need one scanner object and it needs to be declared once.  It will be used in several methods, so it needs to be declared outside of all methods but inside of a class.

    public class UserInteraction {
        Scanner scan = new Scanner(System.in);

        public static void main(String[] args) { // ...

Let's leave the rest of the variable declarations and initializations for later and move code into our new methods.

    private void showMenu() {
        System.out.println("1. Enter a course name and a grade");
        System.out.println("2. Display all grades");
        System.out.println("3. Calculate the average grade");
        System.out.println("4. Exit program");
    }

    private void enterCourses() {
        System.out.println("Enter 10 subjects and their corresponding grades:");
        System.out.println();

        for (int i = 0; i < subjects.length; i++) {
            System.out.println("Subject:");
            subjects[i] = scan.nextLine();

            System.out.println("Grade:");
            grades[i] = scan.nextInt();
            scan.nextLine();

            if (i == (subjects.length - 1)) {
                System.out.println("Thank you!");
                System.out.println();
            }
        }
    }

    private void displayGrades() {
        System.out.println("Subjects" + "\tGrades");
        System.out.println("---------------------");

        for (int p = 0; p < subjects.length; p++) {

            System.out.println(subjects[p] + "\t" + "\t" + grades[p]);
        }
    }

    private void displayAverageGrade() {
        System.out.println("Total of grades: " + getSum(grades));
        System.out.println("Count of grades: " + grades.length);
        System.out.println("Average of grades: " + getAverage(grades));
        System.out.println();
    }

Now you can see that subjects and grades need to have class scope, that is, to be declared outside of all methods.  And we notice that declaring sum in run() is unnecessary because a local variable (one inside a method) will do.

Scanner weirdness

The scanner class has some oddities that I can't go into at length, but the long and the short is that you either put the line scan.nextLine() after each scan.nextInt(), or write a wrapper around scanner to hide the weirdness, or, like you did, have two scanner objects, one for strings and one for ints.  The wrapper is best, but as you can imagine, it's the hardest.  The nice thing though is if you write it correctly, you can reuse it anywhere.

Final thoughts

There is a lot more you can do, but I need to wrap this up.  Here are some things to think about.

You could put all your display into one class
What happens when you enter a non-integer into the menu or grades prompts?
There's a better way to store the subjects and grades.  What about a class that holds one subject and grade?  How does that change the program?
You have repeated code in your getAverage() and getSum() methods.  How can you get rid of that?

Okay, here's the "final" code, but not really final as there's a lot still to do:

import java.util.Scanner;

public class UserInteraction {
    private Scanner scan = new Scanner(System.in);
    private String[] subjects = new String[10];
    private int grades[] = new int[10];

    public static void main(String[] args) {
        new UserInteraction().run();
    }

    private void run() {
        int choice;

        do {
            showMenu();
            choice = scan.nextInt();
            scan.nextLine();

            switch (choice) {
            case 1:
                enterCourses();
                break;
            case 2:
                displayGrades();
                break;
            case 3:
                displayAverageGrade();
            }
        } while (choice != 4);
    }

    private void showMenu() {
        System.out.println("1. Enter a course name and a grade");
        System.out.println("2. Display all grades");
        System.out.println("3. Calculate the average grade");
        System.out.println("4. Exit program");
    }

    private void enterCourses() {
        System.out.println("Enter 10 subjects and their corresponding grades:");
        System.out.println();

        for (int i = 0; i < subjects.length; i++) {
            System.out.println("Subject:");
            subjects[i] = scan.nextLine();

            System.out.println("Grade:");
            grades[i] = scan.nextInt();
            scan.nextLine();

            if (i == (subjects.length - 1)) {
                System.out.println("Thank you!");
                System.out.println();
            }
        }
    }

    private void displayGrades() {
        System.out.println("Subjects" + "\tGrades");
        System.out.println("---------------------");

        for (int p = 0; p < subjects.length; p++) {
            System.out.println(subjects[p] + "\t" + "\t" + grades[p]);
        }
    }

    private void displayAverageGrade() {
        System.out.println("Total of grades: " + getSum(grades));
        System.out.println("Count of grades: " + grades.length);
        System.out.println("Average of grades: " + getAverage(grades));
        System.out.println();
    }

    private double getAverage(int[] array) {
        int sum = 0;
        for (int i : array) {
            sum += i;
        }
        return ((double) sum) / array.length;
    }

    private double getSum(int[] array) {
        int sum = 0;
        for (int i : array) {
            sum += i;
        }
        return sum;
    }

}


TaskScheduler that uses a dedicated thread I'm trying to implement a TaskScheduler that runs all tasks in the order they are submitted, on a single dedicated thread. Here's my implementation, using a BlockingCollection:

class SequentialScheduler : TaskScheduler, IDisposable {
    readonly BlockingCollection<Task> m_taskQueue = new BlockingCollection<Task>();
    readonly Thread m_thread;
    bool m_disposed;

    public SequentialScheduler() {
        m_thread = new Thread(Run);
        m_thread.Start();
    }

    public void Dispose() {
        m_disposed = true;
    }

    void Run() {
        while (!m_disposed) {
            var task = m_taskQueue.Take();
            Debug.Assert(TryExecuteTask(task));
        }
    }

    protected override IEnumerable<Task> GetScheduledTasks() {
        return m_taskQueue;
    }

    protected override void QueueTask(Task task) {
        m_taskQueue.Add(task);
    }

    protected override bool TryExecuteTaskInline(Task task, bool taskWasPreviouslyQueued) {
        if (Thread.CurrentThread == m_thread) {
            return TryExecuteTask(task);
        }
        return false;
    }
}

I've played with it a bit and it seems to work well. I have some lingering questions however:

As you can tell by the Debug.Assert, I'm not sure how TryExecuteTask could return false and I'm just assuming that given my current implementation, it won't. Can that actually happen and what I should I do if it does?
I'm not sure my implementation of TryExecuteTaskInline makes sense as it doesn't get called in my tests. If I understand correctly, this method should run the task synchronously if possible; hence why I'm checking if the current thread is the scheduler's dedicated thread.
Also, my implementation of Dispose doesn't wait for the current task to complete, and will cause the rest of the tasks in the queue to just wait there forever, but short of making it blocking and wait for the queue to empty, I don't see how to do that any differently. I just need a way to release the thread.

TryExecuteTaskInline is used in what is called "task inlining": basically, when you call Wait() on a Task that didn't start executing yet, it might be executed on the current thread. A simple way to test that is:

var factory = new TaskFactory(new SequentialScheduler());

factory.StartNew(
    () =>
    {
        factory.StartNew(() => { }).Wait();
    });

For more information, see Stephen Toub's article Task.Wait and “Inlining”.

But this all means that a Task might be executed outside of your Run() loop, so the call to TryExecuteTask() there might return false. Because of that, you should simply ignore the return value there (just like the official example scheduler does, in its NotifyThreadPoolOfPendingWork()).

Another option would be to remove inlined Tasks from the queue, but there is no simple way to do that for BlockingCollection.

I think that m_disposed should be volatile, otherwise, the Run() loop can be optimized into an infinite loop that checks the value of m_disposed only once, at the start.

For disposal, you might want to use the completion capability of BlockingQueue. That way, trying to schedule a new Task after the scheduler has been disposed will throw, which I think is the correct behavior.

If you do this, you can also rewrite Run() to use GetConsumingEnumerable(), and remove m_disposed altogether.


Function to validate date format I have this JavaScript function which checks if the format has 'yyyy' and replaces 'yy' with 'y':

function validateDateFormat(format) {
    if(format.indexOf("yyyy") != -1)
     return format.replace(/yy/g, 'y');
     else return format;
}

I feel like it can be simplified further. Can someone help me to refactor/optimize it?

This is already pretty simple. But I'd use the ternary operator ?: to make it a bit more compact:

function validateDateFormat(format) {
    return format.indexOf("yyyy") == -1 ? format : format.replace(/yy/g, 'y');
}

Also, perhaps you can be a bit more accurate.
There is a discrepancy between your check and the action taken:

Check if string contains "yyyy"
Replace all occurrences of "yy" with "y"

Perhaps it would be more accurate to change the action to this:

  Replace "yyyy" with "yy" (only once)

That is:

function validateDateFormat(format) {
    return format.indexOf("yyyy") == -1 ? format : format.replace(/yyyy/, 'yy');
}

Actually, and probably I should have started with this:
since the .replace method returns the original string if there is no match,
you could simplify even further:

function validateDateFormat(format) {
    return format.replace(/yyyy/, 'yy');
}


EventLogger for MVC application I have worked on one application related for booking space and required to store activity of user into table and access from admin side.
For that I have created one database entity and EventLogger class.

public partial class UserEventLog
{
    public int ID { get; set; }
    public string UserID { get; set; }
    public Enum_SourceType SourceType { get; set; }
    public Enum_EventType EventType { get; set; }
    public string EventId { get; set; }
    public string EventName { get; set; }
    public Enum_LogType LogType { get; set; }
    public string LogMessage { get; set; }
    public string LogDescription { get; set; }
    public string IPAddress { get; set; }
    public string HostName { get; set; }
    public DateTimeOffset EventTriggeredOn { get; set; }
    public virtual AspNetUser User { get; set; }
}

related enum

public enum Enum_SourceType
{
    User,
    Order
}
public enum Enum_LogType
{
    Information,
    Warning,
    Error
}
public enum Enum_EventType
{
    Create,
    Update,
    Delete,
    Duplicate,
    DownLoad,
    Preview,
    Search,
    AppliedCoupon,
    Cancel,
    LoggedIn,
    Register,
    SendMail,
    ConfirmEmail,
    Account,
    View,
    SendMessage
}

and I have created EventLogger class for log event from whole application.

  public class EventLogger
{

    public static IUserEventLogService UserEventLogService
    {
        get
        {
            return Core.ContainerManager.GetConfiguredContainer().Resolve<IUserEventLogService>();
        }
    }
    public static IUnitOfWorkAsync UnitOfWork
    {
        get
        {
            return Core.ContainerManager.GetConfiguredContainer().Resolve<IUnitOfWorkAsync>();
        }
    }
    public static async Task Log(DateTimeOffset eventTriggeredOn, Enum_EventType eventType, string eventName, Enum_LogType logType, string logMessage,string eventId = "", string logDescription = "")
    {

        var currentUserId = System.Web.HttpContext.Current.User.Identity.GetUserId();
        string ipAddress = GetLocalIPAddress();
        var userEventLog = new UserEventLog()
        {
            EventId = eventId,
            LogType = logType,
            EventName = eventName,
            EventTriggeredOn = eventTriggeredOn,
            EventType = eventType,
            LogMessage = logMessage,
            LogDescription = logDescription,
            IPAddress = ipAddress,
            UserID = currentUserId,
            ObjectState = Repository.Pattern.Infrastructure.ObjectState.Added,
            SourceType = Enum_SourceType.User
        };
        UserEventLogService.Insert(userEventLog);
       await UnitOfWork.SaveChangesAsync();
    }
    public static string GetLocalIPAddress()
    {
        string visitorIPAddress = GetVistorIp(HttpContext.Current.Request.ServerVariables);
        return visitorIPAddress;
    }
    static string GetVistorIp(NameValueCollection headers)
    {
        if (!string.IsNullOrWhiteSpace(headers["HTTP_X_FORWARDED_FOR"]))
            return headers["HTTP_X_FORWARDED_FOR"];
        if (!string.IsNullOrWhiteSpace(headers["HTTP_CF_CONNECTING_IP"]))
            return headers["HTTP_CF_CONNECTING_IP"];
        return headers["REMOTE_ADDR"];
    }
}

Please suggest me if this solution is right or require any modification or any good solution for suggestion for my implementation.

Again enums should not have any prefix. Drop all those Enum_! You may like to take a look to .NET Naming Guidelines.

Inside the class UserEventLog you do not need to repeat Log prefix for its properties. For example just Message is enough. Think about a Window object: did you ever see properties named WindowTitle, WindowSize, WindowCanMaximize and so on?

Validate values, I might assign null to LogMessage (or an invalid enum for EventType) and it will go straight to the DB engine (which might reject it or not).

Do not hide resolved services in a property like UnitOfWork and UserEventLogService. If you use those objects more than once in the same call then it's a performance hit you do not want to pay (because it has to be resolved for each call and it might event be a new object, it depends on how it's configured in your DI framework). Resolve them once in ctor or better accept them as parameters.

In addition of a single method Log() with all those parameters you may want to introduce few extension methods (or overloads):

public static async Task LogUserWarning(this EventLogger logger,
                                        DateTimeOffset eventTriggeredOn,
                                        string eventName,
                                        string message)
{
    // ...
}

Think twice before using default values for parameters. If you own this code and the calling code then it's not an issue (probably) but if your code is released as a library, to be used by someone else, then you're introducing a strong contract (simply changing default value will not update callers unless they're recompiled, not to mention that they have to check if new values are what they expect). For a real-world example of this issue you may read Adding option parameter to a library in c# without creating a breaking change
.

Using String.Empty instead of null for a string entry in a DB may be a waste but it depends on DB engine you're using (for example compare SQL Server and Oracle).

I see no reason for EventLogger to do not be sealed.

Do not use System.Web.HttpContext.Current directly. Accessing a static property makes impossible to test your code in unit testing outside an HTTP request. The same is true for GetLocalIPAddress(). If really you can't avoid it then move it to a derived class, at least you can test all the underlying logic in isolation. Better would be to introduce a service to retrieve those information:

interface IHttpRequestInspector
{
    string GetIpAddress();
    string GetAuthenticatedUserName();
}

Now you can create a default implementation using the code you already have but you can also have a MockedHttpRequestInspector class which can be used in unit testing:

sealed class MockedHttpRequestInspector : IHttpRequestInspector
{
    public MockedHttpRequestInspector(string ipAddress, authenticatedUserName)
    {
        _ipAddress = ipAddres;
        _authenticatedUserName = authenticatedUserName;
    }

    public string GetIpAddress() => _ipAddress;
    public string GetAuthenticatedUserName() => _authenticatedUserName;

    private readonly string _ipAddress;
    private readonly string _authenticatedUserName;
}

Some properties you declared in UserEventLog are not used in code you show (for example UserEventLog.User). Drop them?


Haskell tips/why doesnt this scale linearly? My friend wrote a program which compares random arrangements of die faces to find the one with the most evenly distributed faces - especially when the faces are not a mere sequence.

I translated his program into haskell because I've been looking for a reason to talk someone's ear off about how cool haskell is. However, I am not very proficient with haskell (it took me forever to write this and it has undergone a couple giant refactorings) and so I have two problems.

he has been big on optimizing his versions, and this is not very fast, and it does not scale linearly. It goes from 415 checks/s to 97 checks/s when I go from 1000 to 20000 checks. Did I mess up some tail recursion or is it some kind of larger problem?
the code that came out of this isn't actually as elegant as I had predicted. I want this to be a solid showcase of Haskell, if you have any ideas on how to simplify it I am all ears

This is the most relevant code:

-- _CENTERS :: [{ x :: Float, y :: Float, z :: Float}]
-- _VALUES :: [Num]

-- Basically just (repeat $ map rand [0.._SIDES]), but never using a seed twice
randstates from = (take _SIDES (infrand from)) : randstates newseed
    where   infrand seed = seed : infrand (shuffle seed)
            newseed      = (infrand from) !! (_SIDES + 1)

-- yates shuffle
yates _ (last:[]) = [last]
yates (rand:pass) (swap:order) = choice:yates pass rorder
        where   choice = order !! index
                index  = (randfrom rand) `mod` (length order)
                rorder = take (index) order ++ swap : drop (index + 1) order

arrangements seed = map arrange $ randstates seed
        where   arrange rands = yates rands [0.._SIDES - 2]

-- fns comparing arrangements --
arcLength i j = 1 / (1 + _WEIGHT * acos(dot3D / _VEC_LEN_SQUARED))
        where   dot3D    = apply x + apply y + apply z
                apply fn = (fn i) * (fn j)

matrix arr = map crosscmp arr
        where   crosscmp s1  = [ value s1 * (distance s1 s2) | s2  <- arr ]
                distance a b = arcLength (_CENTERS !! a) (_CENTERS !! b)
                value s      = fromInteger $ _VALUES !! s

variance arr = sum $ map perside (matrix arr)
        where   perside s = (sum s - mean) ^ 2
                mean      = (sum (concat $ matrix arr)) / (sides + 1)
                sides     = fromInteger $ toInteger _SIDES

maxDistr = maximumBy (\a b -> variance a `compare` variance b)

Main is basically just

print $ maxDistr $ take _TRIALS $ arrangements seed

Just a quick observation: as the length of values and centers increase, the list indexes (!!) are increasingly inefficient. Try values :: Vector Float, centers :: Vector Center with Data.Vector.(!) and see if performance improves.

I'm trying to see what other improvements can be made, I have a few in mind but its difficult to understand how your code works without the full program. Would you mind linking to a pastebin?


Correct MySQL DB Query and processing I ve got a table called EXP it looks like this

The table contains

REG_ID - (id of the registred users)
TRA_TYPE - (type of transportation the user chose 1-export 2-import)
PRO_ID - (product id in rande [1-16])

When a user registers he can choose transportation type EXPORT and products for EXPORT or import AND PRODUCTS for IMPORT or BOTH

Then his data (name, password) is inserted into REGISTRATION table and (id form registration tabl, type/s of transportation and selected products into EXP table)

I need to retrieve an array to process the data, I expect to get this array

Array
(
    [0] => Array
        (
            [id] => 1
            [tra_type] => 1
            [prod_import] => Array
                (
                    [0] => 2
                    [1] => 6
                    [2] => 10
                )

        )

)

I use this block of code to get it

    //get all the rows containing choosen products
    $mas = array(2,6,10);
    $implode_mas = implode(',',$mas);
    $g = $qw->query("select * from exp where pro_id in ($implode_mas) and tra_type = '1';");
    while($o = $g->fetch(PDO::FETCH_ASSOC)){
        $k[] = $o;
    }

   //extract only IDs
    for($hk=0;$hk<count($k);$hk++){
        $O[] = $k[$hk]['reg_id'];
    }

    //removing equal IDs
    $O = array_values(array_unique($O));

    //define an empty array
    $Ty = array();

    //run for loop
    for($ep=0;$ep<count($O);$ep++){

        // getting current ID
        $cId = $O[$ep];

        //add id to array
        $Ty[$ep]['id'] = $cId;

         ///add tra_type to array
        $Ty[$ep]['tra_type'] = $k[$ep]['tra_type'];

        for($hj=0;$hj<count($k);$hj++){
         // if the array IDS if equal IDs from $O array  add its pro_id to array $Ty
         if($k[$hj]['reg_id'] == $cId )
           $Ty[$ep]['prod'][]= $k[$hj]['pro_id'];
        }

    }

    print_r($Ty);

Could give me advice how to do the same in an easier way?

Change SQL Query may help you in someway, like this,

SELECT reg_id, tra_type, GROUP_CONCAT(pro_id SEPARATOR ',') AS pro_ids
FROM exp
WHERE pro_id in ($implode_mas)
AND tra_type = '1'
GROUP BY reg_id;

Result will be like this

reg_id    tra_type    pro_ids
1         1           2,6,10
4         1           10

Then you can split it to array


PHP class design with methods requiring database access I've been a procedural PHP programmer for a long time but I'm getting to learn OOP and would like some advice. I'm designing a class which currently is composed mainly of simple getters/setters, but I'm not quite sure if I'm designing my class in the best way possible.

The getters require accessing a database to pull the requested information, so I'm checking if a property has been set first before running the query which seems like a good idea to me but I'm not sure.

I think I could make use of DI by type hinting the $connect parameter and using a DAL but this is not a public facing API and we're never going to use a different database engine so I'm not sure it's worth the trouble. I'm using a proprietary database engine which has a bug preventing me using PDO and parametrised queries. I'm also not sure if I should be try/catching/throwing exceptions inside class methods.

Here is a sample of the code below, there are more methods/properties but they all follow the same basic structure. I would very much appreciate any and all commentary.

class Patient {
  // Database connector
  private $connect;
  protected $_connection;

  public $dmg_ID;
  public $LNK_ID;
  public $dailyLNK_ID;

  public function __construct($connect, $lnkid, $dlkid = NULL) {
      $this->_connection = $connect;
      unset($connect);

      // Patient requires a LNK_ID but not always a daily link ID
      $this->LNK_ID = $lnkid;

      if (!is_null($dlkid) && is_numeric($dlkid)) {
        $this->dailyLNK_ID = $dlkid;
      }
  }

  public function getDmgID() {

      if ($this->dmg_ID) return $this->dmg_ID;

      $sql = "SELECT lnk_dmgID FROM LINK WHERE lnk_ID=".$this->LNK_ID;
      try {
          $result = odbc_exec($this->_connection,$sql);
          if($result) {
              $dmgID = odbc_fetch_array($result);
          }
          else {
              throw new RuntimeException("Failed to connect.");
          }
      }
      catch (RuntimeException $e) {
          print("Exception caught: $e");
      }

      return $this->dmg_ID = $dmgID['LNK_DMGID'];
  }

  public function setDmgID() {

      if (!$this->dmg_ID) return false;

      $sql = "UPDATE LINK SET lnk_dmgID=".$this->dmg_ID." WHERE lnk_ID=".$this->LNK_ID;
      try {
          $result = odbc_exec($this->_connection,$sql);
          if(!$result) {
              throw new RuntimeException("Failed to connect.");
          }
      }
      catch (RuntimeException $e) {
          print("Exception caught: $e");
      }

      return true;
  }
...

OOP is definitely the way to go. The best advise I can offer is to read the book "Clean Code" by Uncle Bob. The code there is in Java but all principles apply equally to any other language.

I would definitely abstract the DB interface so I can use for any other database, even if you don't plan it in the next future. I have posted small data store architecture code that may help you.

From a look at your code, I am a bit confused by the naming connect and connection and the latter stores $connect in the constructor. Maybe give a more descriptive names (one of the top recommendations in the mentioned book).

Why are you unsetting $connect?
The names such as $LNK_ID are cryptic for reader.
I would reserve a separate class dealing with the database via thin interface (see my post) and let other classes only talk to that class instead of DB directly.


Data layer using extension methods I've started to use extension methods to provide data layer functionality in our project. Our project is an MVC website using Code First Entity Framework. So, I've done things like this:

namespace OurProject.Models
{
    public class Project
    {
        public int ProjectId {get; set;}
        public string ProjectName {get; set;}
        public bool IsActive {get; set;}
    }

    public static partial class ProjectExtensions
    {
        public static IQueryable<Project> GetProjects(this IDbContext db)
        {
            return db.Project.Where(p => p.IsActive);
        }
    }
}

Then for ViewModels, I can do this:

namespace OurProject.Models.ViewModels
{
    public class ProjectViewModel
    {
        public int ProjectId {get; set;}
        public string ProjectName {get; set;}
        public string OtherProperty {get; set;}
        public bool IsActive {get; set;}
    }

    public static partial class ProjectExtensions
    {
        public static IQueryable<ProjectViewModel> GetProjectsFormatted(this IDbContext db)
        {
            return db.GetProjects()
                .Select(p => new ProjectViewModel{
                    ProjectId = p.ProjectId,
                    ProjectName = p.ProjectName,
                    OtherProperty = p.ProjectName + " Whatever",
                    IsActive = p.IsActive
                });
        }
    }
}

Is there any reason I shouldn't be doing this?

One file vs. many partials
My first comment is about the ProjectExtensions class itself. I would prefer keeping it together. If for any reason you want to make some consistent change in the extensions it's very hard to do that when they're scattered all over the place.
When to use extension methods
This is the main question: should I do this? My opinion: no. At least not to that extent.
When extension methods were unleashed in c# 3.0, the whole developer community (including me) went on a razzle with them and started creating extensions for just about anything. A bit like the days when text processors with more than one font entered the market. Suddenly we saw club bulletins appear that looked like (and violated) this:

The trouble is that with extension methods it's too easy to 'add' methods to a very general type while the scope of these methods is more restricted than the scope of the type itself, even within one namespace. For example, I had extension methods on string that I decided to ditch again later, when wisdom finally took over.
This phenomenon also applies to your extension methods. They are defined on IDbContext. Now I don't know how many IDbContext classes you intend to implement, but who says they're all going to have Project?
This means that you'd have to confine the extension methods to contexts knowing about Project. This, in turn, may imply that you may as well replace the extension methods by instance methods in one specific context class.
When create a method at all?
That's another question. No matter if it's an extension method or an instance method. Is the task even worth creating a generally accessible method? I specifically frown upon this GetProjectsFormatted method. To me, this looks like something that should probably be a service method. Remember the single responsibility principle? A context shouldn't have any knowledge about any format. That's a (domain) service's responsibility. It may make the context with all its paraphernalia less applicable (or targeted) to other parts of the application or other assemblies. Or, worse, you may get tempted to enter a range of FomatXyz methods to meet other specifications.
Alternatives
Finally, there are other ways to deal with this 'soft delete' problem, i.e. the problem that records are deactivated rather than physically deleted, while the deactivated records shouldn't be displayed. Of course (and that justifies your [extension] method efforts) you don't want to add where IsActive to each and every query.
But extension or instance methods supplying a pre-filtered set aren't enough either! Any old time you apply an Include you'll be in trouble.
var projects = db.GetProjects.Include(p => p.Members);

Of course this includes all members, active or not. And (grief), Includes can't be filtered.
Fortunately, there are better solutions for this. The most viable one, in my opinion, is EntityFramework.DynamicFilters, by which you can define filters that will be applied any time a type is queried, also in Includes.
A very technical, but more complete, solution is the one proposed by the EF team's product owner himself, the code and explanation of which is available here. More complete, because it also redirects deletes to setting a delete flag instead of physical deletes.
Another, pretty elegant, solution, also redirecting deletes, is here. But it uses inheritance, so it may not play nice with other inheritance schemes.


Coin Change Kata in ImmutableJS I'm preparing for a job as a junior/intermediate JS developer. I'm comfortable with my ability to think in higher-order functions, but less confident in my syntax and style (I have more experience in Clojure, ClojureScript). This is simple but I welcome your comments.

import { Map } from 'immutable';

const coinChanger = (d, m) => { // d for denominations, m for money
  const denoms = d.sort((a,b) => b-a)
  const coinCounter = (coin, amount) => {
    const coins = Math.floor(amount / coin);
    const remainder = amount % coin;
    return Map({}).set(coin, coins).set("remainder", remainder);
  };
  const coinMap = denoms.reduce((result, coin) => {
    const amount = result.get("remainder");
    const coinCount = coinCounter(coin, amount);
    return result.merge(coinCount);
  }, Map({"remainder": m}));
  return coinMap;
};

console.log(coinChanger([1, 5], 11));
console.log(coinChanger( [25, 10, 5, 1], 192 ));
console.log(coinChanger([25, 2], 105));

It looks like a nice implementation, but I don't think that using immutable.js is terribly necessary. JS already has a Map, which, while not immutable, could work too. However, a plain object might also be enough.

The slightly ironic bit is that despite choosing to use immutable data structures, your function actually has side-effects: sort modifies the receiver in-place, meaning that the d argument is mutated as a side-effect.

Inside the function itself, i.e. for local variables, I'd be less concerned about mutability. So for instance, a vanilla-JS solution using a plain object (since Map's API of get/set gets tiresome in a hurry) could be:

const coinChanger = (denominations, amount) => {
  return denominations
    .slice() // copy array before sorting
    .sort((a, b) => b - a)
    .reduce((result, value) => {
      result.coins[value] = (result.remainder / value) | 0; // bitwise trick to floor the result
      result.remainder %= value;
      return result;
    }, { remainder: amount, coins: {} });
};

Edit/sidenote: The | 0 trick, while neat, does have some very notable limitations. See the comments for a discussion (thanks, Thriggle!). Essentially, all numbers in JS are 64-bit floats, but behind the scenes, bitwise operations are performed on 32-bit ints. So the trick causes an unsafe cast to int, performs a pointless OR'ing with zero, and casts the result back to a float. So again: It's a trick. Here, given a large enough  amount value, you will get weird results. So use with caution.

I've simplified it somewhat (removed the coinCounter function), and yes, the memo object for reduce is mutated. The effects are however contained within the reduce call, so... eh. The coin denominations are also coerced to strings when used as keys in the object, which a proper Map wouldn't do, but the coercion works both ways, so the keys can transparently be used as numbers later.

Of course, once the object is returned, only the caller holds a reference, and then it's the caller's responsibility – so that's where an immutable data structure might be better. But for the purposes of a quick alternative implementation, I'll leave things as-is.

Style-wise, your code looks fine. You're missing a semi-colon after sort (JS mostly works without semi-colons, but it looks like you're using them elsewhere, so stay consistent), and you could do with a little more whitespace here and there, but otherwise it's pretty good.

The only thing to wag a finger at, which Fanis Despoudis pointed out in a comment, is that d and m should be spelt out, rather than be explained in a comment. Especially because they're arguments, and they form the interface of the function, they should be readable. In general, all names should of course be clear and descriptive, but parameter and function names should be especially easy to parse, as that's what others will see first and make use of.


Recursive filename search I have created a little program to search a set of folders holding documents scanned.

The folder structure is as follows:

  c:\images\year\month\date\documenttype\firstpartofdocumentNo.\

  the year folder contains years from 2005 - 2015

  the month folder contains the months of the year (Obviously) same with
  date

  the documenttype folder can contain between 1 and 5 folders

  the firstpartofdocumentno. can contain between 1 and 3 folders

The code I am using at the moment is:

CompName = Environment.MachineName

TicketNo = TxtTicketNo.Text

If CompName = "Comp1" Then
    ImageDir = "C:\Images\"
Else
    ImageDir = "\\Comp2\Images\"
End If

For Each DirYear As String In Directory.GetDirectories(ImageDir)
    Dim YearInfo As New DirectoryInfo(DirYear)

    For Each DirMonth As String In Directory.GetDirectories(DirYear)
        Dim MonthInfo As New DirectoryInfo(DirMonth)

        For Each DirDate As String In Directory.GetDirectories(DirMonth)
            Dim DateInfo As New DirectoryInfo(DirDate)

            For Each DirType As String In Directory.GetDirectories(DirDate)
                Dim TypeInfo As New DirectoryInfo(DirType)

                For Each DirStart As String In Directory.GetDirectories(DirType)
                    Dim StartInfo As New DirectoryInfo(DirStart)

                    MainDirectory = ImageDir & YearInfo.Name & "\" & MonthInfo.Name & "\" & DateInfo.Name & "\" & TypeInfo.Name & "\" & StartInfo.Name & "\"
                                                                                For Each Ticket As String In Directory.GetFiles(MainDirectory, TicketNo & "*")
                        LstFiles.Items.Add(Ticket)
                    Next

                Next

             Next

        Next
    Next
   Next

I have a textbox on the form which is used to enter the last four numbers of the ticketno and then this code runs when the button is clicked.

The problem is it can take up to five minutes to search, so I was wondering if there is a way to optimize this code to speed it up a bit or does this sound about right for searching that many folders.

This might sound like a goofy answer, but if you already have Windows' file indexing service set up and running on your system, you can search the database contents of the index results using simple SQL terminology.

Here is an article that shows you how

In this way, you're not traversing the directory tree each time you search, but rather you're letting Windows Indexing Service log each new file when it is entered, and then searching the resulting database of information.


Strictly Identical arrays input The textbook problem:

  (strictly identical arrays) Two arrays list1[] and list2[] are
  strictly identical if they have the same length and list1[] is equal
  to list2[] for each[i].

  Write a function that returns true if list1 and list2 are strictly
  identical using the following header:

bool strictlyEqual(const int list1[], const int list2[], int size)

  Write a test program that prompts the user to enter two lists of
  integers and displays whether the two are strictly identical. The
  sample runs follow. Note that the first number in the input indicates
  the number of the elements in the list. This number is not part of the
  list. Assume the list size is maximum.

Any general tips to clean up this code? It just seems that I can accomplish the task w/o doing all the checks.

   #include <iostream>
   using namespace std;

   bool strictlyEqual(int const list1[], int const list2[], int size);

   bool strictlyEqual (int x1[], int x2[], int n)
   {
     int f=0; int i;
     for (i =1; i<=n; i++)
     {
               if (x1[i] != x2[i])
               {
                  // breaks loop
                    f=1;
                    break;
                }
     }

 if (f==0)
 return (true);
 else
 return(false);
 }

int main () {
cout << "enter list1: " << endl;
int list1[20], i;
cin >> list1[0];
for (i=1; i<= list1[0]; i++)
cin>> list1[i];

cout <<"enter the list2" << endl;
int list2[20];
cin >> list2[0];
for (i=1; i<= list2[0]; i++)
cin >> list2[i];

if (list1[0] == list2[0])
{
     int size = list2[0];
     bool v=strictlyEqual(list1, list2, size);
     if (v== true)
     cout << "identical" << endl;
     else
     cout << "not identical " << endl;
 }

  return 0;
 }

I see a number of things that may help you improve your code.

Fix your formatting

The code as posted has inconsistent indenting which makes it hard to read.  Choose a particular style and apply it consistently to make your code easier to read.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Validate the input

Nothing in the code currently prevents the user from entering a list size of 3000 or -98.  The main routine should validate the input size and reject numbers that are not within range.

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream.  This can be time-consuming in a program with a lot of I/O and is rarely actually needed.  It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one.  Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Simplify boolean expressions

The code currently contains this:

if (f == 0)
    return (true);
else
    return (false);

However, since the == operator already returns a bool, you can simplify this to a single line:

return f == 0;

Also note that return is not a function and does not need parentheses.  Similarly within main, we have this:

bool v = strictlyEqual(list1, list2, size);
if (v == true)
    cout << "identical" << endl;
else
    cout << "not identical " << endl;

But one could instead write this:

if (strictlyEqual(list1, list2, size)) {
    std::cout << "identical\n";
} else {
    std::cout << "not identical\n";
}

Or perhaps this:

std::out << (strictlyEqual(list1, list2, size) ?  "" : "not ")
        << "identical\n";

Read the problem statement carefully

The problem says that the size is not part of the array, but your program inserts the size as the first element of the array.  Instead, you could read the size from the user and store it in a separate variable that is passed to the strictlyEqual function.  Also note that as the current code is written, if the two lists are not of equal size, the program prints nothing.

Make sure your arguments match

The declaration of the function in the current code is this:

bool strictlyEqual(int const list1[], int const list2[], int size);

but the actual defintion has this:

bool strictlyEqual(int x1[], int x2[], int n)

First, if the function is defined before use, there's no need for a separate declaration at all.  Second, if you do have a separate declaration, make sure that everything matches, including const and the parameter names.  Strictly speaking, it's not an error to have different parameter names, but it can be confusing to readers of the code, so it's generally best to have them match. A difference in const, however, modifies the signature of the function

Think more carefully about the algorithm

Because only one size is passed to the function, it seems that we must assume that the two arrays are of equal size.  (I'd say this is a flaw in the question, but presumably out of your control.)  Given that, the function only returns true if all elements are equal, so the simple way to do that would be this:

bool strictlyEqual(const int list1[], const int list2[], int n) {
    for (int i=0; i < n; ++i) {
        if (list1[i] != list2[i]) {
            return false;
        }
    }
    return true;  // we only get here if every element was equal
}

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments:  "I didn't know that." or "That's bad advice!"  My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard.  For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

  [...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

  If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea.  We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function.  Reasons against omitting seem to boil down to "it looks weird".  If, like me, you're curious about the rationale for the change to the C standard read this question.  Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!)  In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.


Webpage with four color images I have a very simple webpage that contains 4 color images. When I hover over any of the images the original (xxxx1.jpg) is replaced by a black and white version (xxxxx2.jpg) of the same image, and then it reverts back to the original image after hover.

I'm declaring the same variable for both handlerIn and handlerOut:

var src = $(this).attr("src")

Can I declare the var just once somehow? I just can't seem to figure this out myself.

$(document).ready(function(){
    $( "img" ).hover(
        function() {
            var src = $(this).attr("src").replace("1.jpg", "2.jpg");
            $(this).attr("src", src);
        },
        function() {
            var src = $(this).attr("src").replace("2.jpg", "1.jpg");
            $(this).attr("src", src);
        }
    );
});

The first thing I would do to make the code shorter is to take advantage of the fact that many jQuery methods like .attr() accept a function as the second argument, allowing you to replace this:

var src = $(this).attr("src").replace("1.jpg", "2.jpg");
$(this).attr("src", src);

...with this:

$(this).attr("src", function(i, v) { return v.replace("1.jpg", "2.jpg"); });

(and similar in the other function.) What this does is call the function in the second argument with the current value of the attribute as the v argument, so you don't need a separate call to .attr() to find out what the current value is, and you don't need the src variable.

Then I'd change the .hover() code to pass a single function to run on mouseenter and mouseleave, and use the event object to check whether it is mouseenter or mouseleave:

$(document).ready(function(){
  $("img").hover(function(e) {
    $(this).attr("src", function(i, v) {
      return v.replace(/[12]\.jpg/, (e.type === "mouseenter" ? "2" : "1") + ".jpg");
    });
  });
});

This uses a regular expression /[12]\.jpg/ with .replace() to find a string ending with "1.jpg" or "2.jpg", and replace it with a value depending on the event.type.

Demo: https://jsfiddle.net/g5ncjjxo/