Simple Android Volley login request I've made a simple login API request which gets a token as a response. I had to use Toast a few times. Is is possible to make it with only two Toasts Successful/Failed?

public void login(View view){
    RequestQueue queue = Volley.newRequestQueue(this);
    String URL = "http://10.0.2.2:8000/api/login";
    StringRequest request = new StringRequest(Request.Method.POST, URL,
            new Response.Listener<String>()
            {
                @Override
                public void onResponse(String response) {
                    try {
                        JSONObject jsonObj = new JSONObject(response);
                        myToken =jsonObj.getString("token");
                        if (myToken != ""){
                            Toast.makeText(getApplicationContext(), "Login successfull !", Toast.LENGTH_LONG).show();
                        } else {
                            Toast.makeText(getApplicationContext(), "Ooops something is not correct !", Toast.LENGTH_LONG).show();
                        }
                    } catch (JSONException e) {
                         myToken = "";
                        Toast.makeText(getApplicationContext(), "Ooops something is not correct !", Toast.LENGTH_LONG).show();
                         e.printStackTrace();
                    }
                }
            },
            new Response.ErrorListener()
                {
                @Override
                public void onErrorResponse(VolleyError error) {
                    myToken = "";
                    Toast.makeText(getApplicationContext(), "Ooops something is not correct !", Toast.LENGTH_LONG).show();
                    NetworkResponse response = error.networkResponse;
                    String errorMsg = "";
                    if(response != null && response.data != null){
                        String errorString = new String(response.data);
                    }
                }
            }
    ) {
        @Override
        protected Map<String, String> getParams()
        {
            Map<String, String> params = new HashMap<String, String>();
            TextInputEditText mail = findViewById(R.id.textInputEditTextEmail);
            TextInputEditText pw = findViewById(R.id.textInputEditTextPassword);
            params.put("email", mail.getText().toString());
            params.put("password", pw.getText().toString());
            return params;
        }
    };
    queue.add(request);
}

You can use a variable and assume that it will fail, if you do not print the new message

   public void login(View view){
            RequestQueue queue = Volley.newRequestQueue(this);
            String URL = "http://10.0.2.2:8000/api/login";
            StringRequest request = new StringRequest(Request.Method.POST, URL,
                    new Response.Listener<String>()
                    {
                        @Override
                        public void onResponse(String response) {
                            String loginMsg = "Ooops something is not correct !";

                            try {
                                JSONObject jsonObj = new JSONObject(response);
                                myToken =jsonObj.getString("token");
                                if (myToken != ""){
                                    loginMsg = "Login successfull !"
                                }
                            } catch (JSONException e) {
                                 myToken = "";
                                 e.printStackTrace();
                            }

                            Toast.makeText(getApplicationContext(), loginMsg, Toast.LENGTH_LONG).show();

                        }
                    },
                    new Response.ErrorListener()
                        {
                        @Override
                        public void onErrorResponse(VolleyError error) {
                            myToken = "";
                            Toast.makeText(getApplicationContext(), "Ooops something is not correct !", Toast.LENGTH_LONG).show();
                            NetworkResponse response = error.networkResponse;
                            String errorMsg = "";
                            if(response != null && response.data != null){
                                String errorString = new String(response.data);
                            }
                        }
                    }
            ) {
                @Override
                protected Map<String, String> getParams()
                {
                    Map<String, String> params = new HashMap<String, String>();
                    TextInputEditText mail = findViewById(R.id.textInputEditTextEmail);
                    TextInputEditText pw = findViewById(R.id.textInputEditTextPassword);
                    params.put("email", mail.getText().toString());
                    params.put("password", pw.getText().toString());
                    return params;
                }
            };
            queue.add(request);
        }


Alphabetizing `import` Statements I did a quick google search, and it didn't show up.

I haven't noticed it posted in pylint.py, or PEP8.py.

My emacs mode doesn't warn me if my imports are listed arbitrarily...does anyone do this?

import longest.modulename.first
import medium.sizename
import short
import sys
import os

from imports import go_at_the_bottom
from length import does_matter
from still import cascading

This is typically how I do it. But after a while, the list gets big!

I think this might work better, but it's kind of ugly:

import longest.modulename.first
import medium.sizename
import os
import short
import sys

from BeautifulSoup import BeautifulSoup as BS
from BeautifulSoup import BeautifulStoneSoup as BSS #froms are sorted by import
from imports import go_at_the_bottom
from length import does_matter
from mechanize import Browser as br
from still import cascading

What's the standard for this? I like the look of the cascading imports, but I also like the utility of searching for what's being used in the script quickly.

Is there another approach to this that I didn't cover that you use?

PEP-8 says:

  Imports should be grouped in the following order:

  standard library imports
  related third party imports
  local application/library specific imports

  You should put a blank line
  between each group of imports.

I follow it and then sort modules alphabetically within each group without separating from from the other imports, like this:

import re
import os
from   pprint import pprint
import traceback

import lxml

from   readers    import SomeReader
from   util.stuff import (stuff_init, stuff_close, StdStuff,
                          OtherStuff, BlahBlah)


Change the title attribute of summary if details is open I have this JavaScript code that functions as intended, although as a beginner, I do not know of a shorter, more efficient way of writing it.

The code is a simple toggle that changes the title attribute of the <summary> HTML tag from hide to show when the user clicks on the <summary> to open the <details>.

I used loops since there are several of these in my page:

var i,l;

var detailsElem = document.getElementsByTagName('details'),
summaryElem = document.getElementsByTagName('summary'),
summElemLen = summaryElem.length;
detlElemLen = detailsElem.length;

function detailsToggle() {
  for (i = 0; i < summElemLen; i++) {
    if (detailsElem[i].open === true) {
      summaryElem[i].title = 'hide';
    }
    else {
      summaryElem[i].title = 'show';
    }
  }
}

for (i = 0; i < detlElemLen; i++) {
  detailsElem[i].ontoggle = function() {detailsToggle()};
}

Is there a less verbose way of achieving the same function?

detailsElem[i].ontoggle = function() {detailsToggle()};

It's better to use addEventListener. The problem is that doing it this way replaces anything assigned to ontoggle, potentially blowing away any handler put by other code.

var detailsElem = document.getElementsByTagName('details'),
summaryElem = document.getElementsByTagName('summary'),
summElemLen = summaryElem.length;
detlElemLen = detailsElem.length;

Personally recommending a var per variable. That way, you can easily add/remove lines without worrying about continuation. One such example is summElemLen ending with a ; and not a ,. This will cause detlElemLen to be defined on the global scope, and open to collision. This minor oversight can cause a lot of headache in the long term.

Also, using getElementsByTagName is very broad. Do you really want to take all elements? Or just a subset? If it's the latter, use a class name instead.

if (detailsElem[i].open === true) {

if statements expect a boolean. detailsElem[i].open is already a boolean. No need to do === true, it's redundant. true === true is always a true.

Lastly, one flaw in the code is that once you toggle one detail element, you toggle all summary titles regardless of whether they belong to the element you clicked.

The following is what I consider shorter, more explicit and takes advantage of the array API. We get all detail elements, loop through and attach a handler. Handler checks if a summary is present and only sets a title if there is one.

const slice = Array.prototype.slice

// querySelectorAll to select elements using a CSS selector.
// It returns an array-like structure (indexed, has length, no array API)
// To take advantage of array APIs, we use slice to convert to array.
const details = slice.call(document.querySelectorAll('.detail'))

// Use forEach to loop through each detail element found
details.forEach(detail => {

  // Add event handler when toggle is fired
  detail.addEventListener('toggle', event => {

    // Look for the summary with querySelector. It's like querySelectorAll
    // Except it returns either an element or null, instead of an array.
    const summary = detail.querySelector('summary')

    // No summary, do nothing.
    if(!summary) return;

    // Otherwise, assign title depending on detail state.
    summary.title = detail.open ? 'hide' : 'show'
  })
})


Toggle CSS class on vote I have this jQuery handled buttons. But I think I overdid it with the if else statements.

I am wondering if the code can be more efficient in some way than what I have. It's mainly voting up and down. So what is does is toggle buttons and inserts HTML.

function handleVote(commentid, voted, type){
    $.ajax({
      url: endpoint + commentid + "/update/",
      method: "PATCH",
      data: { commentid: commentid, voted: voted,  type: type},
      success: function(data){
        console.log(data)
        if(type === 'down' && voted === 'True'){
          $('.comment-item .down-vote .i-down-'+commentid+'').toggleClass('active-color');
          $('.comment-item .down-vote .i-down-'+commentid+'').toggleClass('opacity-initial');
          $('.comment-item .down-vote .i-down-'+commentid+'').css('opacity', '0.2')
        } else if(type === 'down' && voted === 'False'){
          $('.comment-item .down-vote .i-down-'+commentid+'').toggleClass('active-color');
          $('.comment-item .down-vote .i-down-'+commentid+'').toggleClass('opacity-initial');
        } else if(type === 'up' && voted === 'True'){
          $('.comment-item .up-vote .i-up-'+commentid+'').toggleClass('primary-color');
          $('.comment-item .up-vote .i-up-'+commentid+'').toggleClass('opacity-initial');
          $('.comment-item .up-vote .i-up-'+commentid+'').css('opacity', '0.2')
        } else if(type === 'up' && voted === 'False'){
          $('.comment-item .down-vote .i-up-'+commentid+'').toggleClass('primary-color');
          $('.comment-item .down-vote .i-up-'+commentid+'').toggleClass('opacity-initial');
        }

        $(".comment-vote-down .vote-count-down-" + commentid +"").html(`-${data.num_vote_down}`);
        $(".comment-vote-up .vote-count-up-" + commentid +"").html(`+${data.num_vote_up}`);
      },
      error: function(data){
        var msg = formatErrorMsg(data.responseJSON)
        $("[data-id='" + commentid + "']").closest('div .comment-wrapper').after(msg);
      }
    })
  }

Use variable for element, toggle can take multiple classes, use minimal selector.

var voteButton;
if(type === 'down'){
    voteButton = $('.i-down-' + commentid);
    voteButton.toggleClass('active-color opacity-initial');
} else {
    voteButton = $('.i-up-'+commentid);
    voteButton.toggleClass('primary-color opacity-initial');
}
if(voted === 'True'){
    voteButton.css('opacity', '0.2')
}

or you could stack functions

$('.comment-item .down-vote .i-down-'+commentid+'')
  .toggleClass('active-color opacity-initial')
  .css('opacity', '0.2')


HTTP routing framework I am new to JavaScript programming and would like to get your comments/input or corrections on this chunk of code. I have tried to emulate patterns from recognized GitHub projects but I still don't have good feel for what "good" NodeJS code looks like.

I have written a very simple HTTP routing framework that works differently than ones currently available (it's a personal project / learning experience).

GitHub

var req, meth, url;
/**
 * Fork -> Synchronous process
 * @param  {Function} callback
 */
var _sync = function(callback) {
    _assembleTask(null, workFlow[meth][url].sync, function(err, tasks) {
        if (err) return callback(true, 'Invalide Workflow : ' + tasks);
        async.series(tasks, function(err, results) {
            return callback(err, results);
        });
    });
};

/**
 * Fork -> Parallel process
 * @param  {Function} callback
 */
var _async = function(callback) {
    _assembleTask(null, workFlow[meth][url].async, function(err, tasks) {
        if (err) return callback(true, 'Invalide Workflow : ' + task);
        async.parallel(tasks, function(err, results) {
            return callback(err, results);
        });

    });
};

/**
 * Merg - Parallel process
 * @param  {Object}   data - return values of sync and async
 * @param  {Function} callback
 */
var _merg = function(data, callback) {
    _assembleTask(data, workFlow[meth][url].merg, function(err, tasks) {
        if (err) return callback(true, 'Invalide Workflow : ' + tasks);
        async.series(tasks, function(err, results) {

            return callback(err, {
                'sync': data.sync,
                'async': data.async,
                'merg': results
            });
        });
    });
};

/**
 * URL Matcher
 * @param {String}   needle - incoming url
 * @param {Object}   haystack - JSON object for
 * @param {Function} callback
 */
var _urlMatch = function(needle, haystack, callback) {
    var match = null;
    for (var i in haystack) {
        if (urlglob(i, req.url)) {
            match = i;
            break;
        }
    }
    return callback(match);
};

/**
 * Assemble tasks
 * @param {data}     data - data to be passed into instantiated object
 * @param {taskList} taskList - list of tasks to instantiate
 * @param {Function} callback
 */
var _assembleTask = function(data, taskList, callback) {
    var tasks = {};
    var err = false;
    for (var key in taskList) {
        var module = taskList[key].split('.');
        if (!procsObj[module[0]]) return callback(true, module);
        var procsInst = new procsObj[module[0]](req, data);
        if (!procsInst[module[1]]) return callback(true, module);
        tasks[module[0] + '_' + module[1]] = procsInst[module[1]];
    }
    return callback(err, tasks);
};

/**
 * Main Method
 * @param  {Object}   request - request object passed from HTTP
 * @param  {Function} callback
 */
this.run = function(request, callback) {
    req = request;
    if (!(req.method in workFlow) && !('*' in workFlow)) {
        callback(405, 'Method Not Allowed');
    } else {
        meth = (req.method in workFlow) ? req.method : '*';
        _urlMatch(req.url, workFlow[meth], function(match) {
            if (match === null) {
                callback(404, 'Not Found');
            } else {
                var a = {};
                url = match;
                typeof workFlow[meth][url].sync != 'undefined' && (a.sync = _sync);
                typeof workFlow[meth][url].async != 'undefined' && (a.async = _async);
                async.parallel(a, function(err, results) {

                    typeof workFlow[meth][url].merg != 'undefined' && _merg(results, callback);
                    typeof workFlow[meth][url].merg == 'undefined' && callback(err, results);

                });
            }
        });
    }
};
};

I don't know what workFlow and urlglob are. So there might be wways to improve it that I don't know about.

_sync, _async and _merg are so similar that you should probably combine them in some way, instead of maintaining 3 near-identical copies of the same code. The callback part at the end is different in _merg because you're constructing an object at the end, but you can rewrite it so that the object-construction bit is handled where _merg is called.

In _urlMatch, the callback function doesn't follow the convention - the first argument should be an error argument. Even though you never need to call it with an error.

In fact, there's no reason why _urlMatch needs to be asynchronous at all. I.e. it shouldn't have the callback argument, and should finish with return match; instead of return callback(match);. Then, instead of _urlMatch(req.url, workFlow[meth], function(match) { ... });, do var match = _urlMatch(req.url, workFlow[meth]); ....

Similarly, _assembleTask can be made synchronous. (It makes a list of things to do; the things are asynchronous, but it doesn't actually do them, so it's OK.) The synchronous equivalent of callbacks with errors is the try-catch mechanism.

You've got a variable called module. nodejs has a built-in global variable called module. Since you've re-declared it, you're not interacting with the original module, so it's OK. Indeed, the global module usually isn't used inside function calls. But still, overriding variable declarations is generally frowned upon.

In _assembleTask, err is an unnecessary variable.

In _sync and _async, you can replace function(err, results) { return callback(err, results); } with callback.

You don't need any of these global variables (req, meth, url). You're only using them to pass information to _sync, _async, _merg and _assembleTask, so you should use arguments instead. It might look a bit messier, but global variables make code less modular and therefore harder to work with. Perhaps you did this because you didn't know what to write instead of a.sync = _sync, etc. You can use async.apply for this. I haven't documented the extra arguments to _sync/_async/_merg because I'm not sure exactly what they are.

More generally, you can use closures for this - so instead of async.apply(_sync, request, meth, match) in my version of the code, you could have written function(callback) { _sync(request, meth, match, callback); }. The anonymous function has access to the variables in its enclosing scope: request, meth, match. I only mention this because it's a more flexible way of doing things than async.apply. In this case it's not necessary, since you're writing _sync and can arrange for these three arguments to appear first.

You can replace typeof workFlow[meth][url].merg != 'undefined' with workFlow[meth][url].merg !== undefined. In fact, you can probably just replace it with workFlow[meth][url].merg. It depends on what values workFlow[meth][url].merg can have, so I can't be sure since you haven't told us what workFlow is. If workFlow[meth][url].merg is false, null, 0 or '', then workFlow[meth][url].merg is falsy but workFlow[meth][url].merg !== undefined is true; otherwise, it's OK to just use workFlow[meth][url].merg.

I would say you can do the same with typeof workFlow[meth][url].merg != 'undefined', but there's no reason why they should both be there anyway - just use if-else.

You seem to have an extra curly bracket at the end.

I've incorporated most but not all of these changes below. I haven't tested it, partly because I'm too lazy but mostly because I don't know what the setup is with these variables you haven't explained.

/**
 * Fork -> Synchronous process
 * @param  {Function} callback
 */
var _sync = function(req, meth, url, callback) {
    try {
        var tasks = _assembleTask(req, null, workFlow[meth][url].sync);
    } catch (mod) {
        return callback(true, 'Invalide Workflow : ' + mod);
    }
    async.series(tasks, callback);
};

/**
 * Fork -> Parallel process
 * @param  {Function} callback
 */
var _async = function(req, meth, url, callback) {
    try {
        var tasks = _assembleTask(req, null, workFlow[meth][url].async);
    } catch (mod) {
        return callback(true, 'Invalide Workflow : ' + mod);
    }
    async.parallel(tasks, callback);
};

/**
 * Merg - Parallel process
 * @param  {Object}   data - return values of sync and async
 * @param  {Function} callback
 */
var _merg = function(req, meth, url, data, callback) {
    try {
        var tasks = _assembleTask(req, data, workFlow[meth][url].merg);
    } catch (mod) {
        return callback(true, 'Invalide Workflow : ' + mod);
    }
    async.series(tasks, callback);
    });
};

/**
 * URL Matcher
 * @param {String}   needle - incoming url
 * @param {Object}   haystack - JSON object for
 */
var _urlMatch = function(needle, haystack) {
    for (var i in haystack) {
        if (urlglob(i, needle)) {
            return i;
        }
    }
    return null;
};

/**
 * Assemble tasks
 * @param {data}     data - data to be passed into instantiated object
 * @param {taskList} taskList - list of tasks to instantiate
 */
var _assembleTask = function(req, data, taskList) {
    var tasks = {};
    for (var key in taskList) {
        var mod = taskList[key].split('.');
        if (!procsObj[mod[0]]) throw mod;
        var procsInst = new procsObj[mod[0]](req, data);
        if (!procsInst[mod[1]]) throw mod;
        tasks[mod[0] + '_' + mod[1]] = procsInst[mod[1]];
    }
    return tasks;
};

/**
 * Main Method
 * @param  {Object}   request - request object passed from HTTP
 * @param  {Function} callback
 */
this.run = function(request, callback) {
    if (!(request.method in workFlow) && !('*' in workFlow)) {
        callback(405, 'Method Not Allowed');
    } else {
        var meth = (request.method in workFlow) ? request.method : '*';
        var match = _urlMatch(request.url, workFlow[meth]);
        if (match === null) {
            callback(404, 'Not Found');
        } else {
            var a = {};
            workFlow[meth][match].sync !== undefined && (a.sync = async.apply(_sync, request, meth, match));
            workFlow[meth][match].async !== undefined && (a.async = async.apply(_async, request, meth, match));
            async.parallel(a, function(err, results) {

                if (workFlow[meth][match].merg !== undefined) {
                    _merg(request, meth, match, results, function(err, merg) {
                        callback(err, {
                            sync: results.sync,
                            async: results.async,
                            merg: merg
                        });
                    });
                } else {
                    callback(err, results);
                }

            });
        }
    }
};


Efficiently swapping the first and last five bits in an unsigned 16-bit integer in C I'm programming on an 8-bit Z80 embedded system, and encountered a problem. The program needs to render some pixels to the screen. The color is stored as RGB565 format as a 16-bit unsigned integer, but the system color is encoded as BGR565, so I've devised the following code to swap the first and last 5 bits in a uint16_t.

Note that flash is uint8_t *, pointed to the flash memory, and each cell is seen as uint8_t.

/* read lower 8-bit */
uint16_t color = flash[idx];

/* read higher 8 bits */
color |= ((flash[idx + 1]) << 8);

/*
 * This converts RGB565 to BGR565 by swapping the first/last five bits.
 * For the last five bits, we extract and bitshift it to the beginning,
 * and for the first five bits, we extract and bitstift it to the end.
 * Then, we clear the original first and last five bits, and apply the
 * bitshifted version on it to swap them.
 */
uint16_t tmp = (color & 0x001F) << 11;
tmp |= (color & 0xF800) >> 11;
color &= 0xFFE0 & 0x07FF; /* at least the compiler can optimize this as 0x07e0 */
color |= tmp;

If I'm programming with a standard compiler, like GCC or Clang, I would be just happy about my solution. But this embedded platform only supports relatively primitive compilers without advanced optimization techniques to remove duplicate computations.

In fact, the best code optimization to this problem is "no code", just make the stored format to be consistent with the system format. But using it as a chance of learning, I'm curious to know if there is a faster way to do the computation above in pure C, especially when we're dealing with a particularly slow machine, like a Z80.

Programming 8-bitters in C is tricky, particularly when dealing with antique ones like Z80. The things you absolutely must consider are:

Keep to 8 bit arithmetic whenever possible.
Ensure that no bitwise arithmetic is done on signed int types. This means that you need to understand the common Implicit type promotion rules of C.
32 bit arithmetic should be avoided like the plague. And let's not even mention floating point.

If you don't keep the above in mind, then the theoretic efficiency of the algorithm be damned. Thus you cannot allow a PC programmer to design it, because they don't understand how an 8-bit MCU works.

Regarding your posted code:

It suffers from using lots of 16 bit arithmetic, that the compiler cannot likely optimize well.
((flash[idx + 1]) << 8); invokes undefined behavior, if flash is an uint8_t with the MSB set. This is a bug caused by implicit integer promotion to int.
Using signed hex literals on a 8/16 bit MCU is dangerous, because up to 0x7FFF they are of type int, but above that they are of type unsigned int. Therefore, you should make a habit of always appending an u/U suffix to them.

Assuming that flash is an array of const uint8_t corresponding to something in flash memory, and assuming that the 6 middle bits should be preserved, not zero-out, then you should do something like this:

// assuming Z80 Little Endian:
uint8_t lo = flash[idx];
uint8_t hi = flash[idx+1];

uint8_t lo_shift = (lo & B_MASK) << 3;
uint8_t hi_shift = (hi & R_MASK) >> 3;

lo = (lo & 0xE0u) | hi_shift;
hi = (hi & 0x07u) | lo_shift;

uint16_t color = ((uint16_t)hi<<8) | lo;

This will allow the compiler to perform as much arithmetic as possible on 8 bit integers. There are implicit promotions to int, but such promotions are not dangerous, as we don't shift data in/out of the sign bit of a 16 bit int. And the u suffix will kill off any accidental promotions, since it guarantees uint16_t in case of 16 bit arithmetic.

To get rid of "magic numbers":

#define R_MASK 0xF8u
#define B_MASK 0x1Fu

uint8_t lo = flash[idx];
uint8_t hi = flash[idx+1];

uint8_t lo_shift = (lo & B_MASK) << 3;
uint8_t hi_shift = (hi & R_MASK) >> 3;

lo = (lo & (uint8_t)~B_MASK) | hi_shift;
hi = (hi & (uint8_t)~R_MASK) | lo_shift;

uint16_t color = (uint16_t)hi << 8 | lo;


Filter Collection by Multiple Criteria I want to the user to filter a list of receipts by various criteria. Just the regular, an empty filter should show all items, an entry in customer should show all receipts from that customer and an additional entry in date should show all entries from said customer on the given date. I have the feeling, my if else apprach is not the best way, since with 4 criteria I'm already at 16 branches, not speak of 5, 6, or 7 criteria.

What is the most elegant way to achieve this.

Model:

class customer
{

    public int ID { get; set; }

    public string Name { get; set; }

}

class receipt
{

    public int ID { get; set; }

    public string Number { get; set; }

    public DateTime Date { get; set; }

    public double Amount { get; set; }

    public customer Customer { get; set; }

}

Viewmodel

class receiptViewModel
{

    ObservableCollection<receipt> ReceiptList { get; set; }

    List<receipt> ReceiptListView { get; set; }

    private string filter;

    public string Filter
    {
        get { return filter; }
        set
        {

            filter = value;

            if (number != null && date == null && customer && null)
            {
                ReceiptListView = ReceiptList.Where(x => x.Number.Contains(number)).ToList();
            }
            else if (number != null && date != null && customer && null)
            {
                ReceiptListView = ReceiptList.Where(x => x.Number.Contains(number) && x.Date === date).ToList();
            }
            //aso aso aso
          }
    }

You can build the LINQ query in several steps by appending new where clauses

IEnumerable<receipt> query = ReceiptList;
if (customer != null) {
    query = query.Where(x => x.CustomerId == customer.ID);
}
if (number != null) {
    query = query.Where(x => x.Number.Contains(number));
}
if (date != null) {
    query = query.Where(x => x.Date == date);
}
...
ReceiptListView = query.ToList();

This reduces the complexity from O(2ⁿ) to O(n)


Dynamic object creation in Python I want to abstract away differences between zipfile and rarfile modules. In my code i want to call ZipFile or RarFile constructors depending on file format. Currently i do it like that.

def extract(dir, f, archive_type):
    '''
    archive_type should be 'rar' or 'zip'
    '''
    images = []

    if archive_type == 'zip':
        constructor = zipfile.ZipFile
    else:
        constructor = rarfile.RarFile

    with directory(dir):
        with constructor(encode(f)) as archive:
            archive.extractall()
        os.remove(f)
        images = glob_recursive('*')

    return images

Is there any more elegant way for dynamic object calling?

Classes are first-class objects in Python.

amap = {
  'zip': zipfile.ZipFile,
  'rar': rarfile.RarFile
}

 ...

with amap[archive_type](encode(f)) as archive:
   ...

or

with amap.get(archive_type, rarfile.RarFile)(encode(f)) as archive:
   ...


Refining an ASP.Net MVC VeiwModel for a Table to display worked hours I am writing an ASP.Net MVC app which has a page that will display a standard table which will contain a person's hours for the week. The basic structure of the table would be a 7 column, muti-row table. The Header of each column would display the Day of the Week with the total hours worked that week. There would then be rows for the time of the day starting at 8AM going to 5:30 PM in 15m increments. In the table for each day would be a listing of what projects where worked on that day with an indication of when it started and stopped being worked on. (Please see image below for visual aid).

I have built a ViewModel to hold the data for this table and pass it up but I am not sure if I built it in a manor that makes it efficient to work though and have it's data displayed so I would love to have some feedback on ways I might be able to improve it or even alternate ways of building such a ViewModel.

If this is off topic here please let me know where is on topic and I will delete and post there.

My ViewModel Code:

{
    public class TimesheetHoursTableVM
    {
        public int TimesheetHeaderID { get; set; }
        public DateTime WeekEndingDate { get; set; }
        public decimal TotalWeekHours { get; set; }
        public decimal SundayHours { get; set; }
        public decimal MondayHours { get; set; }
        public decimal TuesdayHours { get; set; }
        public decimal WednesdayHours { get; set; }
        public decimal ThursdayHours { get; set; }
        public decimal FridayHours { get; set; }
        public decimal SaturdayHours { get; set; }
        public IEnumerable<TimesheetDailyHoursVM> SundayTimesheet { get; set; }
        public IEnumerable<TimesheetDailyHoursVM> MondayTimesheet { get; set; }
        public IEnumerable<TimesheetDailyHoursVM> TuesdayTimesheet { get; set; }
        public IEnumerable<TimesheetDailyHoursVM> WednesdayTimesheet { get; set; }
        public IEnumerable<TimesheetDailyHoursVM> ThrusdayTimesheet { get; set; }
        public IEnumerable<TimesheetDailyHoursVM> FrodauTimesheet { get; set; }
        public IEnumerable<TimesheetDailyHoursVM> SaturdayTimesheet { get; set; }
    }

    public class TimesheetDailyHoursVM
    {
        public int TimesheetID { get; set; }
        public DateTime StartDateTime { get; set; }
        public DateTime EndDateTime { get; set; }
        public string ProjectCode { get; set; }
        public string TaskCode { get; set; }
        public string ProjectDescription { get; set; }
        public string TaskDescription { get; set; }
    }
}

Table the data goes into:

I don't think there is anything necessarilly wrong with your approach especially if it works.  However I personal might consider trying to leverage the DayOfWeek enumeration of c# and avoid the individual day timesheets.

This way you could loop over weeks more easily as well as potentially leverage Linq to do things such as calculating the TotalHours as part of the viewModel.

My viewModel for this case might then look like this:

public class TimesheetWeeklyTableVM
{
    public int TimesheetHeaderID { get; set; }
    public DateTime WeekEndingDate { get; set; }
    public decimal TotalWeekHours { get; set; }

    public List<TimesheetDailyVM> DaysOfWeek { get; set; }

    public TimesheetWeeklyTableVM()
    {
        DaysOfWeek = new List<TimesheetDailyVM>();

        foreach (DayOfWeek dayOfWeek in Enum.GetValues(typeof(DayOfWeek)))
        {
            DaysOfWeek.Add(new TimesheetDailyVM()
            {
                DayOfWeek = dayOfWeek
            });
        }
    }
}

public class TimesheetDailyVM
{
    public DayOfWeek DayOfWeek { get; set; }

    public double TotalHours
    {
        get { return TimeSpan.FromSeconds(Tasks.Sum(p => (p.StartDateTime - p.EndDateTime).TotalSeconds)).TotalHours; }
    }

    public IEnumerable<TimesheetDailyHoursVM> Tasks { get; set; }

    public TimesheetDailyVM()
    {
        Tasks = new List<TimesheetDailyHoursVM>();
    }
}

public class TimesheetDailyHoursVM
{
    public int TimesheetID { get; set; }

    public DateTime StartDateTime { get; set; }
    public DateTime EndDateTime { get; set; }
    public string ProjectCode { get; set; }
    public string TaskCode { get; set; }
    public string ProjectDescription { get; set; }
    public string TaskDescription { get; set; }
}

Note, I'm not sure if TotalHours is calculated differently but if it's just a sum of the Task times then you could include that logic into the Viewmodel itself as in.

public double TotalHours
{
    get { return TimeSpan.FromSeconds(Tasks.Sum(p => (p.StartDateTime - p.EndDateTime).TotalSeconds)).TotalHours; }
}

If you wanted to add properties for each individual day to make it easier to access say for example Monday then you could easily do this such as.

public TimesheetDailyVM Monday { get { return Day(DayOfWeek.Monday); }
public TimesheetDailyVM Tuesday { get { return Day(DayOfWeek.Tuesday); }

private TimesheetDailyVM Day(DayOfWeek day)
{
    return Days.Single(p => p.DayOfWeek == day);
}


Custom Cipher Implementation This is the cipher used in this puzzling post, if you would like to crack it before looking at the programs use to make it.

Several weeks ago, I created a custom cipher for a friend of mine to crack, and I also posted it on Puzzling. Here are the programs I used to create it - I threw them together one morning, and I didn't comment them. I decided to leave them as is - they are self-explanatory to some degree, and I would also like to have my rush-job instinct in variable naming and what not critiqued. I know it's not good per say, but since it was a personal project I didn't care. How the cipher works is explained the in answer to the puzzling question, if my code doesn't show you well enough.

I have an encoder and a decoder. They currently assume that plaintext.txt, code.txt, and decoded.txt are in the current directory. The decoder assumes that the code is formatted as the encoder would format it.

encoder.py

import random
import string

text = open("plaintext.txt").read()

#chars = "abcdefghijklmnopqrstuvwxyz*.,?!()- \n"
chars = "(stephn)a*dowjzr.b\nm,uvklc ?qig!fyx-"

def base_10_to_base_6(n):
    s = ""
    while n:
        s = str(n % 6) + s
        n //= 6

    if len(s) == 1:
        s = "0" + s

    if not s:
        return "00"

    return s

def base_10_to_base_26(n):
    s = ""
    while n:
        s = string.ascii_lowercase[n % 26] + s
        n //= 26

    if len(s) == 1:
        s = "a" + s

    if not s:
        return "aa"

    return s

print(text)

result = ""

for letter in text:
    letter = letter.lower()

    if letter not in chars:
        letter = "*"

    coded_char = base_10_to_base_6(chars.index(letter.lower()))

    print(letter + ": " + coded_char + " : " + chars[int(coded_char, 6)])

    result += coded_char

len_diff = len(result) % 3

if len_diff != 0:
    result += (3 - len_diff) * str(random.randint(0, 5))

print(str(len(result)) + " " + result)

sets_of_3 = []

index = 3

while index <= len(result):
    sets_of_3.append(result[index - 3:index])
    index += 3

print(sets_of_3)

sets_of_letters = []

for set_of_3 in sets_of_3:
    sets_of_letters.append(base_10_to_base_26(int(set_of_3)))

print(sets_of_letters)

with open("code.txt", "w") as text_file:
    print(" ".join(sets_of_letters), file=text_file, end="")

decoder.py

import random
import string

text = open("code.txt").read()

#chars = "abcdefghijklmnopqrstuvwxyz*.,?!()- \n"
chars = "(stephn)a*dowjzr.b\nm,uvklc ?qig!fyx-"

def base_10_to_base_6(n):
    s = ""
    while n:
        s = str(n % 6) + s
        n //= 6

    if len(s) == 1:
        s = "0" + s

    if not s:
        return "00"

    return s

def base_10_to_base_26(n):
    s = ""
    while n:
        s = string.ascii_lowercase[n % 26] + s
        n //= 26

    if len(s) == 1:
        s = "a" + s

    if not s:
        return "aa"

    return s

def base_26_to_base_10(n):
    result = string.ascii_lowercase.index(n[0]) * 26 + string.ascii_lowercase.index(n[1])

    result = str(result)

    if len(result) == 2:
        result = "0" + result
    elif len(result) == 1:
        result = "00" + result
    elif not result:
        result = "000"

    return result

def base_6_to_base_10(n):
    return int(n[0]) * 6 + int(n[1])

sets_of_chars = text.split(" ")

print(sets_of_chars)

triplets = []

for each in sets_of_chars:
    triplets.append(base_26_to_base_10(each))

code = "".join(triplets)

print(code)

index = 2

output = ""

while index <= len(code):
    output += chars[base_6_to_base_10(code[index - 2:index])]
    index += 2

with open("decoded.txt", "w") as text_file:
    print(output, file=text_file, end="")

You may replace the lines as

text = open("plaintext.txt").read()

with so called context manager approach (see PEP 343 -- The "with" Statement):

with open("plaintext.txt") as pt:
    text = pt.read()

which has the advantage of automatically closing the open file, even if an exception is raised on the way.

sets_of_chars = text.split(" ")

is somehow different from

sets_of_chars = text.split()

in two ways - if there are more consecutive spaces, and if it is applied to the empty string:

'abc  def'.split(' ')        # returns ['abc', '', 'def']
'abc  def'.split()           # returns ['abc', 'def']

''.split(' ')                # returns ['']
''.split()                   # returns []

Consider which one is more appropriate for your code.

chars = "(stephn)a*dowjzr.b\nm,uvklc ?qig!fyx-"

It is a constant, so by PEP 8 -- Style Guide for Python Code its name should be in uppercase letters:

CHARS = "(stephn)a*dowjzr.b\nm,uvklc ?qig!fyx-"

Both functions for converting from base 10

def base_10_to_base_6(n):
    s = ""
    while n:
        s = str(n % 6) + s
        n //= 6

    if len(s) == 1:
        s = "0" + s

    if not s:
        return "00"

    return s

def base_10_to_base_26(n):
    s = ""
    while n:
        s = string.ascii_lowercase[n % 26] + s
        n //= 26

    if len(s) == 1:
        s = "a" + s

    if not s:
        return "aa"

    return s

have very similar structure - by the DRY principle (Don't Repeat Yourself) why not to substitute them with one common function, e. g. with

def base_10_to_other(n, base):
    if base not in (6, 26):
       raise ValueError("Illegal base - allowed values are only 6 and 26")

    DIGIT_SYMBOLS = {6: "012345", 26: string.ascii_lowercase}
    zero_symbol = DIGIT_SYMBOLS[base][0]

    s = ""
    while n:
        s = DIGIT_SYMBOLS[base][n % 6] + s
        n //= base

    if len(s) == 1:
        s = zero_symbol + s

    if not s:
        s = 2 * zero_symbol

    return s

Moreover, they are the same in your both modules (encoder.py and decoder.py) - why don't use the 3rd module for all common functions and import from it?
(The same for the other pair of converting functions, of course.)


C++ EventHandler class, invoking & arguments This is my EventHandler class, which is extremely simple and can be used on lambdas and similar:

template<typename Signature, typename...Ax>
class Event
{
private:
    typedef std::function<Signature(Ax...)> Function;
    Function _func;

public:

    void CallFunc(Ax... ax)
    {
        if (_func)
        {
            (_func)(ax...);
        }
    }

    void operator+=(Function func)
    {
        _func = func;
    }

};

I'm looking to find improvements as it's basically as simple as it gets.

Example:

Event<void, int> myEvent;
myEvent += [](int arg) { printf("%i", arg); };

myEvent.CallFunc(15);

Would print out 15.

I don't like that you use operator += to set the function. += suggests that I can append a bunch of functions and they all get called when the event happens, which is not the case. myEvent.setFunction([](int){}); would be better.

It would make sense to take advantage of move semantics. The lambda that gets passed into myEvent is moved into func and then copied into _func, when 2 moves would have been enough:

void operator+=(Function func)
{
    _func = std::move(func);
}

Better yet: Make it a template and perfect forward the argument to have only 1 move:

template<class Fun>
void operator+=(Fun &&func)
{
    _func = std::forward<Fun>(func);
}

You get somewhat worse error messages but most likely better performance.

Same goes for CallFunc, it should take a variadic template argument that gets perfect forwarded into the call for efficiency.

Speaking of efficiency, the flexibility of std::function comes at a price. It is known at compile time what types func can be, so _func should be exactly the same type without type erasure and virtual function calls. You can make that work with a template <class Fun> Event<Fun> make_event(Fun &&fun) style function to get the type of the function and create an Event with the appropriate template parameters, but you lose the ability to specify the function later.

There are extra parenthesis in the call (_func)(ax...);. Maybe it is a style thing, but I would write _func(ax...); instead.

Oh, and printf is not very C++y, but it was just an example.


Calculate all anagrams of a word with Perl In order to become more familiar with Perl I have implemented a code kata from here.

The task was to write a program which prints out all anagrams for a given word. I'd appreciate your review of my solution:

get_anagrams.pl

use strict;
use warnings;

require 'anagrams.pm';

use feature 'say';

my $number_of_arguments = $#ARGV + 1;
if ($number_of_arguments != 1) {
    say 'Usage: get_anagrams.pl word';
    exit 1;
}

foreach my $word (get_permutations($ARGV[0])) {
    say $word;
}

anagrams.pm

use strict;
use warnings;

sub get_permutations
{
    my $string = shift;

    my @permutations = ();
    my @letters = split //, $string;
    my $number_of_letters = scalar(@letters);

    if ( $number_of_letters == 1 ) {
        push @permutations, $letters[0];
    }
    elsif ( $number_of_letters == 2 ) {
        push @permutations, $letters[0] . $letters[1];
        push @permutations, $letters[1] . $letters[0];
    }
    elsif ( $number_of_letters > 2 ) {
        for ( my $i = 0 ; $i < $number_of_letters ; $i++ ) {
            my $first_letter_in_word = $letters[$i];
            my $other_letters = remove_letter_by_index($string, $i);
            foreach my $sub_permutation ( get_permutations($other_letters) ) {
                push @permutations, $first_letter_in_word . $sub_permutation;
            }
        }
    }

    return @permutations;
}

sub remove_letter_by_index
{
    my ( $string, $index ) = @_;
    my $result = '';

    my @letters = split //, $string;
    my $number_of_letters = scalar(@letters);

    for ( my $i = 0 ; $i < $number_of_letters ; $i++ ) {
        if ( $i != $index ) {
            $result = $result . $letters[$i];
        }
    }
    return $result;
}

sub is_element_in_list
{
    my ( $element, @list ) = @_;

    if ( not $element ) {
        return 0;
    }

    foreach my $item (@list) {
        if ( $item eq $element ) {
            return 1;
        }
    }
    return 0;
}

return 1;

anagrams.t

use strict;
use warnings;

require "anagrams.pm";

use feature 'say';

use Test::More tests => 20;

my $sample_string = 'a';
my @retrieved_permutations = get_permutations($sample_string);
ok(is_element_in_list('a', @retrieved_permutations));
ok(scalar(@retrieved_permutations) == 1);

$sample_string = 'ab';
@retrieved_permutations = get_permutations($sample_string);
ok(is_element_in_list('ab', @retrieved_permutations));
ok(is_element_in_list('ba', @retrieved_permutations));
ok(scalar(@retrieved_permutations) == 2);

$sample_string = 'abc';
@retrieved_permutations = get_permutations($sample_string);
ok(is_element_in_list('abc', @retrieved_permutations));
ok(is_element_in_list('acb', @retrieved_permutations));
ok(is_element_in_list('bac', @retrieved_permutations));
ok(is_element_in_list('bca', @retrieved_permutations));
ok(is_element_in_list('cab', @retrieved_permutations));
ok(is_element_in_list('cba', @retrieved_permutations));
ok(scalar(@retrieved_permutations) == 6);

$sample_string = 'abcd';
@retrieved_permutations = get_permutations($sample_string);
ok(is_element_in_list('abcd', @retrieved_permutations));
ok(is_element_in_list('acbd', @retrieved_permutations));
ok(is_element_in_list('dbac', @retrieved_permutations));
ok(scalar(@retrieved_permutations) == 24);

$sample_string = 'abcdefg';
@retrieved_permutations = get_permutations($sample_string);
ok(is_element_in_list('abcdefg', @retrieved_permutations));
ok(is_element_in_list('abcedfg', @retrieved_permutations));
ok(is_element_in_list('acbdefg', @retrieved_permutations));
ok(scalar(@retrieved_permutations) == 5040);

You have written very self-documenting code! That's good. However, you seem somewhat new to Perl, so I'll walk you through some Perl-specific best practices and idioms.

Namespaces/Packages/Modules, use vs. require

You have put your implementation into a separate file anagrams.pm, which you load via require 'anagrams.pm'. When writing a Perl module, we should put it into its own namespace. There is a relationship between file paths and package names: a package Foo::Bar::Baz should reside in a file Foo/Bar/Baz.pm. It is customary for package names to use PascalCase. Lower-case names are only used for pragmas such as strict or warnings that modify how Perl itself behaves. While package names look hierarchical, they are only so by convention: Foo::Bar is not technically an “inner package” of Foo, although names are often used in this manner. Top-level package names should not be used for “real” code, since this is likely to clash with package names from CPAN (the open-source Perl module repository). The Local:: namespace can be used to avoid clashes, since no CPAN module will ever use it. However, if you want to distribute your module, you should reserve a namespace: either by publishing your code in CPAN under a specific module name, or by registering a PAUSE account name. The PAUSE account name can be used as a reserved namespace. E.g. I have some unpublished personal software under the AMON:: prefix.

Assuming you do not intend to distribute your code, using the name Local::Anagrams might be sensible.

To load a module, there are three main techniques available:

require 'path' is rarely used, since the module system is often preferable.
require Module::Name searches for Module/Name.pm in all registered module locations at runtime. This form is only used for deferred module loading, where we only want to load a module if a specific code path was taken.
use Module::Name is like require Module::Name, but at “compile time”. It is the preferred module loading mechanism, since it allows the loaded module to put symbols into the loading namespace.

The problem with namespaces is that symbol names become tediously long. For example, the get_permutations sub from Local::Anagrams would be accessed as Local::Anagrams::get_permutations. When loading our module with use, we can allow certain symbols to be exported from our namespace. Traditionally, the built-in Exporter module is used. In the package-global variable @EXPORT_OK, we list the names of all subroutines we want to make available. The useing module can then list the sub names it wants to import:

package Local::Anagrams;
use strict;
use warnings;
use Exporter 'import';
our @EXPORT_OK = qw/ get_permutations is_element_in_list /;

...

Then in get_anagrams.pl:

use Local::Anagrams 'get_permutations';

And in anagrams.t:

use Local::Anagrams qw/ get_permutations is_element_in_list /;

In larger software systems, exporting symbols doesn't really scale and may still introduce name clashes. The Sub::Exporter module allows the importing module to customize the imported subroutines, but at some point you might give up and prefer to deliver your functionality via an object-oriented rather than procedural interface.

Previously, I said that Perl searches for modules in “all registered module locations”. These can be inspected in the @INC global variable. For example, the current working directory . is usually part of @INC. But unless you install your custom modules via CPAN, they will not generally be in one of the expected locations. If we want to load a module relative to the location of a script, we have to add a new module location:

# in get_anagrams.pl:
use FindBin;  # finds the location of our script
use lib "$FindBin::Bin/../lib";

use Local::Anagrams;

This assumes a standard project directory layout such as

anagrams-project/
  bin/
    get_anagrams.pl
  lib/
    Local/
      Anagrams.pm
  t/
    anagrams.t

For tests, I don't use Find::Bin. Instead, I supply the library directory to the test runner, e.g. perl -Ilib t/anagrams.t or prove -l t.

get_anagrams.pl

All Perl scripts should use a shebang – generally #!/usr/bin/perl or #!/usr/bin/env perl. While this is only used on Unix systems or in some web servers, I'd still recommend using it even on Windows.

my $number_of_arguments = $#ARGV + 1 is a complicated way of saying my $number_of_arguments = @ARGV. Actually, the variable $number_of_arguments is useless here, and you could use @ARGV directly.

Error messages should go on STDERR, not STDOUT. This becomes really important when your program is used in a pipe, or with shell redirection. E.g. perl get_anagrams.pl > anagrams.txt would write the error to the file rather than displaying it to the user. Instead:

say STDERR "Usage: get_anagrams.pl <word>";
exit 1;

Note that after STDERR there is no comma. You could also use method syntax:

use IO::File;
...

STDERR->say("Usage: get_anagrams.pl <word>");
exit 1;

If the exact exit code is not relevant, you could just throw an exception with die, which would have the same effect, except for the exit code being 255 (= -1):

die "Usage: get_anagrams.pl <word>\n";

If you do not want to hardcode the name of the executed program, you can use the special $0 variable:

die "Usage: $0 <word>\n";

This may or may not result in a better error message.

It may not be the most readable style, but I like the statement modifier versions of Perl's control flow constructs a lot. For example, I'd write your loop as

say for get_permutations shift @ARGV;

When say or print aren't given an explicit argument, they default to the implicit $_ context variable, which is used by for/foreach loops when not given an explicit loop variable.

get_permutations():

When looping over all indices of an array, do not use a C-style for loop. Instead, use a range-base loop

for my $i (0 .. $#letters) { ... }

or just instruct Perl to iterate over all indices with keys @letters (available since v5.12.0):

for my $i (keys @letters) { ... }

Note that for and foreach are synonymous, so you have no reason to type out the longer name.

Your algorithm doesn't handle the empty string. get_permutations('') should return the single-element list ''. With it, all cases can be reduced to the two alternatives zero-length or n-length input strings; special-casing 1 and 2 is unnecessary.

sub get_permutations {
  my ($string) = @_;
  my @letters = split //, $string;

  return '' if not @letters;

  my @permutations;
  for my $i (keys @letters) {
    ...
  }
  return @permutations
}

The pattern

for my $var (@input) {
  push @output, simple_expression($var);
}

can be reduced to

push @output, map { simple_expression($_) } @input;

though some people prefer explicit loops for readability.

remove_letters_by_index

Try to avoid single-character operations whenever possible, since these are not at all efficient in Perl. We can implement this function without having to split the string. To remove d (index 3) in the string abcdef, we concatenate the string with indices 0–2 (length 3) and 4–5 (length 2):

0 1 2 3 4 5
|a|b|c|d|e|f|

substr("abcdef", 0, 3) . substr("abcdef", 3+1);

substr takes a start index and a length. Care has to be taken that the substr starting index is a legal index, which is relevant when we are trying to remove the last character.

sub remove_letters_by_index {
  my ($string, $index) = @_;
  return substr($string, 0, $index) if $index + 1 >= length $string;
  return substr($string, 0, $index) . substr($string, $index + 1);
}

We could also just ask Perl to delete a specific character in a string:

sub remove_letters_by_index {
  my ($string, $index) = @_;
  substr($string, $index, 1) = '';
  return $string;
}

With arrays, two similar solutions are possible. We can use the splice function to remove elements from a list:

sub remove_letters_by_index {
  my ($string, $index) = @_;
  my @letters = split //, $string;
  splice @letters, $index, 1;
  return join '', @letters;
}

Or we can use slices to access a range in the list. Slices use a list of indices, which can be generated by a range, and ranges use two indices as inclusive bounds. Since empty ranges do not pose any issue, we do not have to special-case the last character.

sub remove_letters_by_index {
  my ($string, $index) = @_;
  my @letters = split //, $string;
  return join '', @letters[0 .. $index - 1, $index + 1 .. $#letters];
}

is_element_in_list

This function is only ever used in the tests, so it should rather be defined in the test file. It has nothing to do with the single responsibility of the anagrams module, which is finding anagrams.

You require that the $element be a truthy value, and report any falsey values as not found. The following scalars are falsey: undef, 0, '', '0', and any object that has overloaded boolean context or logical negation. Of these values, '' and '0' may legitimately occur in a list of anagrams: get_permutations('') and get_permutations('0'). If you are trying to guard against undef values, always test for definedness. And don't just return a failure – if this is an error, throw an error!

use Carp; # exports `croak`

# `croak` is like `die`, but reports the location
# where `is_element_in_list` was called.
croak "element must be defined" if not defined $element;

The @list is unnecessarily copied. When you pass arrays or hashes to a Perl subroutine, they are flatted to a list. The code

my @array = (1, 2, 3);
some_sub(@array);
my %hash (a => 1, b => 2);
some_sub(%hash);

is effectively equivalent to

some_sub(1, 2, 3);
some_sub('a', 1, 'b', 2);

To avoid this, we can pass array references. An array reference $ref = \@array is a scalar value. To access the array under the reference, we need to dereference it as @$ref or @{ $ref } and can access elements like $ref->[0]. Then:

sub is_element_in_list {
  my ($needle, $haystack_ref) = @_;

  croak "needle must be defined" if not defined $needle;

  for my $item (@$haystack_ref) {
    return 1 if $needle eq $item;
  }

  return 0;
}

Called as:

ok(is_element_in_list('abc', \@retrieved_permutations));

While modules must return a true value, this is usually accomplished with just 1; rather than return 1;.


Handling CSRF protection I'm currently working on an implementation of a CSRF protection. What is the best practice when we detect a CSRF? Is it better to 404 the page, 403 (forbidden), 200 (OK) with an error message, something else?

Bonus : here's my code.
I'm not very proud of the preg_replace and ob_*. If you also have suggestions on how to do it better, I'll take that too.

The addCSRF method is called just before sending the output of an HTML page.
The checkCSRF method is called when the server receives a request.

<?php
// This method checks if the content contains a form and adds a csrf_token hidden field
public static function addCSRF()
{
    $content = ob_get_contents();
    if (strlen($content))
    {
        // Random csrf token
        $randomtoken = base64_encode(openssl_random_pseudo_bytes(32));
        // Add the hidden input to the content if needed
        $content = preg_replace('/(<([^>]*\s)?form(\s[^>]*)?>)/i', '\1<input type="hidden" name="csrf_token" value="'.$randomtoken.'" />', $content, -1, $count);
        // If at least one input has been added, add the csrf_token value in the $_SESSION and replace the content
        if ($count)
        {
            Session::set('csrf_token', $randomtoken);
            // Echo the new content
            ob_end_clean();
            ob_start();
            echo $content;
        }
    }
    return;
}
// This method checks if a form has been submited and if the csrf token is given and valid
public static function checkCSRF()
{
    // No form submitted
    if (!isset($_POST))
        return;
    // CSRF detected
    if (!isset($_POST['csrf_token']) || $_POST['csrf_token'] != Session::get('csrf_token'))
    {
        // 404 ? 403 ? 200 + error message ?
    }
    Session::forget('csrf_token');
}

While trying to add an easter egg on my framework, I saw this answer : Stack Overflow returning HTTP error code 418 (I'm a teapot)?.

Since it looks like there is no "correct" way to handle CSRF, I thought it could be a fun thing to do. Upon detecting a CSRF attack, my framework now sends a HTTP 418 header with a nice ASCII art of a trolly teapot.

So I mark this question as answered, because there's no real best practice (yet ?) and anything would be OK.


Loop for input section of rename script For the input section of a script, I wrote a snippet using a while loop and an if statement to check whether there are matches for the $search. Only if there are matches does it move on to the input of $replace; if there are no matches it asks for $search again.

 while true; do
     echo "Please enter token to be replaced: "
     read -e -r search
     match="$(find . -maxdepth 1 -path "*${search}*" -printf "." | wc -c)"
     if [ "${match}" -gt 0 ]; then
         echo "${match} file(s) found"
         break
     else
         echo "no matches found!"
         echo
     fi
 done

 echo "Please enter replacement: "
 read -e -r replace

# I used the following command to check if the output was right
# echo "rename 's/$search/$replace/g' ./*"

In this snippet I used advice I got from a question I asked on Stack Overflow.

This code is going to be used as the input section of a rename script. In the last line I added an echo "rename... command to check if the output works as expected.

The script is meant to process all files matching the $search criteria in the current directory. Therefore I used find . -maxdepth 1....

The script of which this snippet is part of got reviewed here earlier.

Does this code match general standards?

If you want to match by the same pattern as rename,
then it's probably best to use perl.
To be really pedantic and support filenames with embedded newlines,
you could use -print0 of find,
and then in perl split the output by \0 characters and filter with the grep function:

find . -maxdepth 1 -print0 | perl -ne "print scalar grep /$search/, split /\0/"

Putting in your script:

 while true; do
     echo "Please enter token to be replaced: "
     read -e -r search
     match=$(find . -maxdepth 1 -print0 | perl -ne "print scalar grep /$search/, split /\0/")
     if [ "${match}" -gt 0 ]; then
         echo "${match} file(s) found"
         break
     else
         echo "no matches found!"
         echo
     fi
 done


Rearrange page numbers for printing a book I didn't want my title to be too long so to clear up any confusion I'll explain better here what my code does.

It asks for the total pages of a book(PDF file in my case), and returns the page numbers in a specific order. The groups of 32 pages will each make a booklet that when all booklets are stapled together and laid one on top of the other will be a book ready for binding.

One issue with how I wrote the program is if "pages" is not a multiple of 4, then there will be extraneous pages at the end of the list (i couldn't think of a way to pop those). Another tid-bit, print_order doesn't need to be a list, it just needs to be page numbers separated by commas and I knew list items were separated by commas so having a list of ints would give me what I needed. The formula for printing a book on a single sided printer goes like this (n, f, f+1, n-1) where n is the last page, and f is the first page of each booklet/the whole book. The reason a long book needs to be broken into booklets is because if I take a stack of 100 papers and just fold the whole stack in half the spine would be too wide and the middle pages would be poking out of the cover. So using n and f as the first and last page, respectively, of smaller booklets allows me to print a whole book into something more practical.

I'm not worried about PEP formatting on this one I'm really just curious how this could be written more efficiently. I know python is extremely powerful and I'm not taking full advantage of what it has to offer. This is how I managed to organize my thoughts into doing what I required. I know that there are many out there who could do this in a much simpler way, and I just want to look those ways over so that I can improve my coding style!

pages = float(input("How many total pages is the book?" ))
num_booklets = pages/32
if (pages < 32):
    num_booklets = 1
pages_per_booklet = int(pages/num_booklets)
last_booklet = int(pages%32)

f = 1               #first page of the book
n = int(pages_per_booklet)  #last page of the first booklet
k = 1               #number of booklets completed
print_order = []

while (len(print_order) < pages):
    while ((len(print_order) < pages_per_booklet*k)
    and (len(print_order) < pages)):
        print_order.extend([n, f, f+1, n-1])
        n -= 2
        f += 2
    k += 1
    n = pages_per_booklet * k
    f = 1 + (pages_per_booklet*(k-1))
    if ((len(print_order)+pages_per_booklet) > pages):
        n = int(pages)
        f = int(pages) - last_booklet

print(print_order)

Style

Your code is not too far from following PEP8 and you know PEP8: it would have been a nice touch for volunteer reviewers to provide a piece of code even closer to what they expect. You'll find various tools (online or not) to check more or less precisely the compliancy of your code to PEP8. For most points, once you get used to it, it doesn't take any longer to apply as as you write the code.

Float ?

The following piece of code

pages = float(input("How many total pages is the book?"))

seems to imply that pages could be a float. As far as I can tell, this value corresponds to a number of page and an int would be more appropriate. Also, to be sure, I've tried giving a 0.5 value and the code is stuck in a loop.

Code organisation

Your code is the perfect example of a piece of code that could have been written as a clear function: input and output are both well-defined. Once you're done, you can even write unit-tests for it. Then, the code calling your function could be behind an if __name__ == "__main__": guard.

You'd get something like:

def page_order(pages):
    num_booklets = pages/32
    if (pages < 32):
        num_booklets = 1
    pages_per_booklet = int(pages/num_booklets)
    last_booklet = int(pages%32)

    f = 1               #first page of the book
    n = int(pages_per_booklet)  #last page of the first booklet
    k = 1               #number of booklets completed
    print_order = []

    while (len(print_order) < pages):
        while ((len(print_order) < pages_per_booklet*k)
        and (len(print_order) < pages)):
            print_order.extend([n, f, f+1, n-1])
            n -= 2
            f += 2
        k += 1
        n = pages_per_booklet * k
        f = 1 + (pages_per_booklet*(k-1))
        if ((len(print_order)+pages_per_booklet) > pages):
            n = int(pages)
            f = int(pages) - last_booklet
    return (print_order)

tests = {
    0: [],
    1: [1, 1, 2, 0],
    2: [2, 1, 2, 1],
    3: [3, 1, 2, 2],
    4: [4, 1, 2, 3],
    5: [5, 1, 2, 4, 3, 3, 4, 2],
    6: [6, 1, 2, 5, 4, 3, 4, 3],
    7: [7, 1, 2, 6, 5, 3, 4, 4],
    8: [8, 1, 2, 7, 6, 3, 4, 5],
    9: [9, 1, 2, 8, 7, 3, 4, 6, 5, 5, 6, 4],
    10: [10, 1, 2, 9, 8, 3, 4, 7, 6, 5, 6, 5],
    11: [11, 1, 2, 10, 9, 3, 4, 8, 7, 5, 6, 6],
    12: [12, 1, 2, 11, 10, 3, 4, 9, 8, 5, 6, 7],
    13: [13, 1, 2, 12, 11, 3, 4, 10, 9, 5, 6, 8, 7, 7, 8, 6],
    14: [14, 1, 2, 13, 12, 3, 4, 11, 10, 5, 6, 9, 8, 7, 8, 7],
    15: [15, 1, 2, 14, 13, 3, 4, 12, 11, 5, 6, 10, 9, 7, 8, 8],
    16: [16, 1, 2, 15, 14, 3, 4, 13, 12, 5, 6, 11, 10, 7, 8, 9],
    17: [17, 1, 2, 16, 15, 3, 4, 14, 13, 5, 6, 12, 11, 7, 8, 10, 9, 9, 10, 8],
    18: [18, 1, 2, 17, 16, 3, 4, 15, 14, 5, 6, 13, 12, 7, 8, 11, 10, 9, 10, 9],
    19: [19, 1, 2, 18, 17, 3, 4, 16, 15, 5, 6, 14, 13, 7, 8, 12, 11, 9, 10, 10],
}

if __name__ == "__main__":
    if False:  # Interactive
        pages = int(input("How many total pages is the book?" ))
        print(page_order(pages))
    else:  # Automatic tests
        for inpu, expected_val in tests.items():
            res = page_order(inpu)
            if res != expected_val:
                print(inpu, res, expected_val)
        print("DONE")

divmod

When you perform both a division and a modulo operation with the same values, you can use the more concise and faster divmod builtin function.

def page_order(pages):
    num_booklets, last_booklet = divmod(pages, 32)
    if pages < 32:
        num_booklets = 1

Then I do not understand enough what your code is trying to achieve to try to be able to help more. For the time being, the code looks like:

def page_order(pages):
    num_booklets, last_booklet = divmod(pages, 32)
    if pages < 32:
        num_booklets = 1
    pages_per_booklet = pages/num_booklets

    f = 1  #first page of the book
    n = pages_per_booklet  #last page of the first booklet
    k = 1  # number of booklets completed
    print_order = []

    while len(print_order) < pages:
        while len(print_order) < min(pages, pages_per_booklet*k):
            print_order.extend([n, f, f+1, n-1])
            n -= 2
            f += 2
        k += 1
        n = pages_per_booklet * k
        f = 1 + (pages_per_booklet*(k-1))
        if len(print_order) + pages_per_booklet > pages:
            n = pages
            f = pages - last_booklet
    return print_order


Locale.getISOCountries() Wrapper I have the following codes, i want to get a list of country CODE and NAME from Locale.getISOCountries() and set it into my custom made class, i managed to do it but i somehow feel that my codes is not clean enough, especially the getCountryList() part but i not sure how to minimize it, is there any suggestion? I don't mind to use lambda if possible.

private static List<CountryPair> getCountryList()
{
    String[] locales = Locale.getISOCountries();
    List<String> countryCodes = Arrays.asList(locales);
    List<CountryPair> pairList = new ArrayList<>();
    countryCodes.forEach(code -> {
    CountryPair pair = new CountryPair();
    Locale locale = new Locale("", code);
    pair.setValue(locale.getCountry());
    pair.setDisplayName(locale.getDisplayCountry());
    pairList.add(pair);
    });
  return pairList;
}

Country pair class:

public class CountryPair {

    private String value;

    private String displayName;

    public String getValue() {
      return value;
    }

    public String getDisplayName() {
      return displayName;
    }

    public void setValue(String value) {
      this.value = value;
    }

    public void setDisplayName(String displayName) {
      this.displayName = displayName;
    }
}

My recommendation is: create a constructor for CountryPair, which takes the parameter values:

    public CountryPair(String value, String displayName) {
        this.value = value;
        this.displayName = displayName;
    }

Then, you can use an elegant map-chain:

    List<CountryPair> pairList = countryCodes.stream()
        .map(code -> new Locale("", code))
        .map(locale -> new CountryPair(locale.getCountry(), locale.getDisplayCountry()))
        .collect(Collectors.toList());

Alternatively, you could also use a constructor which uses the Locale object as a parameter.


Randomises capitalisation of letters like bird spongebob A friend and I have an inside joke; well, not exactly inside if it is based off an existing meme. We would send messages in the 'retarded' form, as the meme 'Retarded Spongebob' does in the example "i WoUlD aPPreCiaTE sOmE fEeDbAcK oN mY CoDE". It takes in an input, loops through the character array formed from the string input, randomises the capitalisation, and the entire array is put back for output. Any improvements that can be suggested?

import java.util.Scanner;
import java.util.concurrent.ThreadLocalRandom;

public class RetardedSpongebobifier {
    private char prevChar = 'z';

    public static void main(String[] args) {

        // Take input of sentence
        Scanner scanner = new Scanner(System.in);
        System.out.println("Please enter desired sentence to RS.");
        // Save input
        String input = scanner.nextLine();
        // Turn input into a character array
        char[] inputArray = input.toCharArray();

        RetardedSpongebobifier obj = new RetardedSpongebobifier();
        // Create array to hold new array
        char[] charArray = new char[input.length()];

        // Run through the list of characters, maybe changing them to uppercase or lowercase
        for (int i = 0; i < input.length(); i++) {
            // Send character for random capitalizing
            char adding = obj.randCap(inputArray[i]);
            // Add uppercase/lowercase character back into the character array
            charArray[i] = adding;
        }

        // Print array
        System.out.print(new String(charArray));
    }

    private char randCap(char character) {

        // Check if character is not any of the blacklisted
        if (character == 'i' || character == 'l' || character == ' ' || character == ',' || character == '.') {
            prevChar = character;
            return character;
        }
        else {
            // Random TRUE/FALSE
            boolean isCap = (Math.random() * 100) >= 50;
            if (isCap && Character.isLowerCase(prevChar)) {
                prevChar = character;
                // Return uppercase letter
                return Character.toUpperCase(character);
            }
            else {
                prevChar = character;
                // Return lowercase letter
                return Character.toLowerCase(character);
            }
        }
    }
}

Unnecessary comments

You have a lot of comments telling WHAT the code does. Comments should tell -why- you do it in a certain way or what else you have tried but failed. Just repeating in comment what the code says is a waste of time (for the reader and the writer as well)

Method name

You method is called randCap. I prefer just naming it what it does, so randomCapitalize() for example. Makes the code more readable.

Logic

I tried to understand what the logic behind the prevCharacter is. It is not immediately clear how it works, and what is has to do with the blacklist.

I think you mean to make sure there are not many lowercase or uppercase characters in a row?

You don't need such a long blacklist, because you can safely call toUpperCase() and toLowerCase() on comma's etc.

Code in main loop

You can move a lot of the code out of the main loop, by adding a method public String spongeBobify(String s) to the class. The main method will only take care of the input/ouput.

Unnecessary calculations

What is wrong with:

boolean isCap = Math.random() >= 0.5;

or, if you have a Random object, random.nextBoolean()?

Closing the scanner

You need to close your Scanner, you can do this by using a try-with-resources:

try (Scanner scanner = new Scanner(System.in)) {
    ...


Finding the number of integers between 1 and K, who's sum of the digits is a multiple of D Good day. So, I need to find the number of integers between 1 and K (inclusive) satisfying the following condition, modulo (10^9)+7:

  The sum of the digits in base ten is a multiple of D

Here is my code:

k = int(input())
d = int(input())
count = 0

for i in range(1,k+1):
  hello = sum(list(map(int,str(i))))
  if (hello%d == 0):
    count += 1

print (count)

Basically, what the above code does is:

Takes as input two numbers: k & d.
Defines a counter variable called count, set to the default value of 0.
Iterates over all numbers in the given range: 1 to k.
If any number the sum of any number in the above range is a multiple of d, then it adds to the counter count by 1.
And then, at the end, finally, it outputs the count, meaning the number of integers between 1 and K, that have their sum of digits as a multiple of D.

Now, this code works just fine, but I am seeking to make it run for any number until 10^10000 in 1 second or less.

EDIT:

The actual problem that I am trying to solve can be found here: Digit Sum - DM::OJ

Thank you.

There's probably a more efficient method, but I can't think of one at the moment. A few suggestions, though:

Why do you have a badly-named variable hello that is only used once? I'd recommend inlining it.
There's no need for parentheses around the condition for the if statement.
There's no need to convert the map to a list before finding the sum.
You could convert the loop into the sum of a generator expression.
It's probably a good idea to convert your code to have a main function and an if __name__ == '__main__' guard.

Result (not tested):

def perform_calculation(k, d):
    return sum(sum(map(int, str(i))) % d == 0 for i in range(1, k+1))

def main():
    k = int(input())
    d = int(input())

    print(perform_calculation(k, d))

if __name__ == '__main__':
    main()


Compare two associative arrays by key I am currently for a program I am writing needing to compare two associative arrays by key and value in PHP and get the common elements as well as the difference between the common elements.

I have currently arrived at the following solution:

<?php
$newArr = [0 => ['id' => 'UT5', 'qty' => '4'], 1 => ['id' => 'WRO', 'qty' => '3'], 2 => ['id' => 'SHO', 'qty' => '3']];

$oldArr = [0 => ['id' => 'SHO', 'qty' => '1'], 1 => ['id' => 'UT5', 'qty' => '2'], ];

$newReset = [];
foreach( $newArr as $item ) {
    $newReset[$item['id']] = $item['qty'];
}

$oldReset = [];
foreach( $oldArr as $item ) {
    $oldReset[$item['id']] = $item['qty'];
}

foreach( $newReset as $key => $val ) {
    if( isset( $oldReset[$key] ) ) {
        $toAdd[$key] = max( $oldReset[$key], $val ) - min( $oldReset[$key], $val );
    }
    else $toAdd[$key] = intval($val);
}

var_dump( $toAdd );

I am just curious as to how I can go about implementing perhaps array functions or an array function with a closure etc... to refactor the approach or use another method entirely?

For reference, my output is currently as follows:

array(3) {  ["UT5"]=> int(2)  ["WRO"]=> int(3)  ["SHO"]=> int(2) }

Any help is appreciated.

Thanks!

You can reduce your code in several ways.

First, since you seem to want the absolute value of the difference between qtys for the same ìd`:

$toAdd[$key] = max( $oldReset[$key], $val ) - min( $oldReset[$key], $val );

It can be achieved more simply:

$toAdd[$key] = abs($oldReset[$key] - $val);

BTW, when id doesn't exist in $oldReset you probably want the same, so you should use $toAdd[$key] = abs($val); instead of $toAdd[$key] = intval($val); (but maybe you are sure that qty is always positive).

On the other hand, you're preparing both $newArr and $oldArr to get a simplified version of the form array(id => qty): you can do it for only one array, so using less code and probably working faster.

With all the changes proposed above, your whole code becomes:

$oldReset = [];
foreach ($oldArr as $item) {
  $oldReset[$item['id']] = $item['qty'];
}
foreach ($newArr as $key => $val) {
  $id = $val['id'];
  $qty = $val['qty'];
  if (isset($oldReset[$id])) {
    $toAdd[$id] = abs($oldReset[$id] - $qty);
  }
  else {
    $toAdd[$id] = abs($qty);
  }
}

Last point, I notice that you don't try to detect ids existing in $oldArray and not present in $newArray. But again maybe you're sure it cannot happen.


C++: Merge Sort Implementation I have this code for Merge Sort. I want to improve this code. I have learned Here

that we should use Iterartor so that we can use any container. How to use iterator in this code.

#include <iostream>
#include <vector>
#include <algorithm>

template<class T>
void merge(std::vector<T>& v, int p, int q, int r)
{
int size1 = q-p+1;
int size2 = r-q;
std::vector<T> L(size1);
std::vector<T> R(size2);

for(int i = 0; i < size1; i++)
{
    L[i] = v[p+i];
}
for(int j = 0; j < size2; j++)
{
    R[j]=v[q+j+1];
}

int i=0,j=0;
int k;
for(k = p; k <= r && i < size1 && j < size2; k++)
{
    if(L[i] <= R[j])
    {
        v[k] = L[i];
        i++;
    }
    else
    {
        v[k] = R[j];
        j++;
    }
}
for(i = i; i < size1; ++i)
{
    v[k] = L[i];
    k++;
}

for(j = j; j < size2; j++)
    {
        v[k] = R[j];
        k++;
    }
}

template<class T>
void merge_sort(std::vector<T>& v, int p, int r){
if(p < r)
{
    int q = (p+r)/2;
    merge_sort(v, p, q);
    merge_sort(v, q+1, r);
    merge(v, p, q, r);
}
}

int main()
{
std::vector<int>vec;
vec.push_back(13);
vec.push_back(5);
vec.push_back(7);
vec.push_back(7);
vec.push_back(4);
vec.push_back(2);
vec.push_back(10);
int sz = vec.size();
std::cout << "Entered array : ";
for(int n = 0; n < sz; n++){
    std::cout << vec[n] <<" ";
}
std::cout << "\n";
std::cout << "Sorted array : ";
merge_sort(vec, 0, sz-1);
for(int n = 0; n < sz; n++){
    std::cout << vec[n] << " ";
}
std::cout << "\n";

std::vector<char> c;
c.push_back('d');
c.push_back('y');
c.push_back('h');
c.push_back('l');
c.push_back('e');
c.push_back('a');
int sz1 = c.size();
std::cout << "Entered array : ";
for(int n = 0; n < sz1; n++){
    std::cout << c[n] <<" ";
}
std::cout << "\n";
std::cout << "Sorted array : ";
merge_sort(c, 0, sz1-1);
for(int n = 0; n < sz1; n++){
    std::cout << c[n] << " ";
}
std::cout << "\n";

std::vector<std::string> str;
str.push_back("car");
str.push_back("dog");
str.push_back("apple");
str.push_back("ball");
str.push_back("tree");
int sz2 = str.size();

std::cout << "Entered array : ";
for(int n = 0; n < sz2; n++){
    std::cout << str[n] <<" ";
}
std::cout << "\n";
std::cout << "Sorted array : ";
merge_sort(str, 0, sz2-1);
for(int n = 0; n < sz2; n++){
    std::cout << str[n] << " ";
}
std::cout << "\n";

    return 0;
}

Some suggestions:

Why int for indices?

What if your input vector has 2^33 elements, and your system's sizeof(int) is 4? That's perfectly possible. Use std::vector<T>::difference_type (or perhaps std::vector<T>::size_type)  for indices into the vector and std::vector<T>::size_type for its size.

(In some cases there's some benefit to using smaller indices, but I wouldn't worry about that in your situation.)

Why just std::vector?

While std::vector is a very useful data structure, it's not the only one we might use. In fact, we might just have a raw T* and a size_t length. You're forcing your client code to copy its data into a vector to have it sorted - which is quite unnecessary.

Now, the standard C++ library approach on this matter is to template on pairs of iterators - for the beginning and the end of the range you want to sort. Another acceptable alternative, but which will only work for a consecutive range of memory, is to use a gsl::span<T> (and you can easily extract a span from a vector using v.begin() and v.end()).

In the hopefully-near future you would be able to take a range, but this has not been standardized yet.

Use meaningful names

Suppose I want to use your merge_sort() function:

template<class T>
void merge_sort(std::vector<T>& v, int p, int r);

How would I know what is p and what is r? Even v I can only guess from its' type. At least use:

template<class T>
void merge_sort(std::vector<T>& data_to_sort, int start_index, int end_index);

... and see also the next point for an even better API.

Also, you have ps, qs and rs all over your code and it's confusing. So are L and R. Remember - the reader doesn't know what you meant, only what you wrote. And s/he shouldn't have to hold a lot of "semantic context" to read your code. Surely you'll agree this:

    auto start = start_index;
    auto end = end_index;
    if (start < end) {
        auto middle = (start + end ) / 2;
        merge_sort(data_to_sort, start,  middle);
        merge_sort(v, middle + 1, end);
        merge(data_to_sort, start, middle, end);
    }

is more readable than your main merge_sort() body.

Expose the API the client code would like to use

Let's ignore the iterators option for a second and stick with a vector. If I want to use your code, why should I type

merge_sort(my_vector, 0, my_vector.size() - 1);

...when I should be able to type just

merge_sort(my_vector);

...which has all the necessary information? Now in the implementation, you're using a range (via a pair of indices). But you should still offer me:

template<class T>
void merge_sort(std::vector<T>& data_to_sort)
{
     return merge_sort(data_to_sort, 0, data_to_sort.size() - 1);
}

and consider putting the rest of your code within namespace detail.

If you switch to an iterator interface, this point might become moot.

Should you really be copying so much?

You're sorting Ts. Now, sizeof(T) could theoretically be quite large, in which case the swaps you perform during the merge are very costly. In those cases, it may be more reasonable to sort a permutation of array indices, which are "compared" by comparing their corresponding Ts. (If the comparison takes about the same time as copying the whole thing, perhaps it's not that much of an issue.)

I'm not saying you should rewrite your code now, just be aware of your implicit assumption

Your merge sort requires O(n) extra space, O(1) is possible

When merging, you allocate O(n) extra space - two vectors L and R. That's ok in many cases; but it's not the optimal thing to do, complexity-wise. It's actually possible to perform a merge sort with O(1) extra memory, still maintaining O(n log(n) time - as is established in this work by Katajainen , Pasanen and Teuhola.

Again, I wouldn't go implement that instead of your merge sort.


Casting an Enumeration with Descriptions into a Dictionary The Enumeration in question is based on int, so the following Extension returns a Dictionary<int, string> of the Enum's Value and Description.

Is there a cleaner way of implementing this?

public enum TeamDepartments
{
    [Description("Geschäftsleitung")] Management = 1,
    [Description("Verkaufsleitung")] Sales = 2
}

public static class UtilityExtensions
{
    public static Dictionary<int, string> ToDictionary(this Type EnumerationType)
    {
        Dictionary<int, string> RetVal = new Dictionary<int,string>();
        var AllItems = Enum.GetValues(EnumerationType);
        foreach (var CurrentItem in AllItems)
        {
            DescriptionAttribute[] DescAttribute = (DescriptionAttribute[])EnumerationType.GetField(CurrentItem.ToString()).GetCustomAttributes(typeof(DescriptionAttribute), false);
            string Description = DescAttribute.Length > 0 ? DescAttribute[0].Description : CurrentItem.ToString();
            RetVal.Add((int)CurrentItem, Description);
        }
        return RetVal;
    }
}

I have this class in my code which is very similar to what you are looking for:

public static class EnumHelper {
    public static string GetRealName(this Enum value) {
        if (value == null) {
            throw new ArgumentException("value");
        }

        string name = value.ToString();
        var fieldInfo = value.GetType().GetField(name);
        if (fieldInfo == null) return "";
        var attributes = (RealNameAttribute[])fieldInfo.GetCustomAttributes(typeof(RealNameAttribute), false);

        if (attributes.Length > 0) {
            name = attributes[0].Name;
        }

        return name;
    }
    public static bool IsIgnored(this Enum value) {
        if (value == null) {
            throw new ArgumentException("value");
        }

        string name = value.ToString();
        var fieldInfo = value.GetType().GetField(name);

        return fieldInfo == null || fieldInfo.IsDefined(typeof(IgnoreAttribute), true);
    }

    public static Dictionary<int, string> ToDictionary(this Type enumType) {
        return Enum.GetValues(enumType)
            .Cast<object>()
            .Where(f => !((Enum)f).IsIgnored())
            .ToDictionary(k => (int)k, v => ((Enum)v).GetRealName());
    }
}


Writing better JUnit tests I am using Spring Framework, Hibernate and JUnit.  I am testing persisting of a UserEntity.

The UserEntity has the following associations:

ManyToMany - City
ManyToMany - ActivityCategory
ManyToOne - UserType

@Test
public void testCreateNotExistingEmail() {
    UserTypeEntity userTypeEntity = userTypeDao.find(1L);

    BCryptPasswordEncoder encoder = new BCryptPasswordEncoder(16);
    String password = encoder.encode("123456");

    List<CityEntity> cityEntities = new ArrayList<>();
    cityEntities.add(cityDao.find(1L));
    cityEntities.add(cityDao.find(2L));

    List<ActivityCategoryEntity> activityCategoryEntities = new ArrayList<>();
    activityCategoryEntities.add(activityCategoryDao.find(4L));
    activityCategoryEntities.add(activityCategoryDao.find(5L));

    UserEntity userEntity = new UserEntity();
    userEntity.setEmail("user@domain.com");
    userEntity.setIsActive(true);
    userEntity.setPassword(password);
    userEntity.setUserTypeEntity(userTypeEntity);
    userEntity.setCities(cityEntities);
    userEntity.setActivityCategories(activityCategoryEntities);

    userDao.create(userEntity);

    UserEntity userEntityCreated = userDao.find(userEntity.getId());

    Assert.assertNotNull(userEntityCreated);
    Assert.assertEquals(2, userEntityCreated.getCities().size());
    Assert.assertEquals(2, userEntityCreated.getActivityCategories().size());
}

Is there anything I could do better in the test?

A minor remark: your test is named testCreateNotExistingEmail. The @Test annotation already indicates that this is a test so it doesn't have to be repeated in the methodname itself.

Adding to that: I like to structure my tests in the format [UnitOfWorkName]_[ScenarioUnderTest]_[ExpectedBehaviour].

It is hard to tell what exactly your test does when I look at the code because the methodname indicates something about a nonexisting email, which leads me to think about an exception handled somewhere.

But when I look at the code I see no assertions like that at all. In fact, I only see an email being used once in a seemingly irrelevant context.

Some type of clarification is needed: a better method name. Something along the lines of createUser_WithNonExistingEmail_ShouldReturn_StandardCitiesAndCategories

Now when the test fails amongst a big group of tests, you can pinpoint more closely what scenario is going wrong exactly before you dig into it.


Convert Byte array into collection of items of different types I'm recieving UDP messages as byte arrays and depending on details in some of the first byte, I need to convert remaining bytes into a multitude of different possible arrangements of other data types.

These data types are primarily 'uint', 'ushort' and just 'byte' but can be in any order, and there can be any number of them. Each of these items will have a named variable.

I've tried using quite a few different options and am getting close, but feel that the methods I've created are not the best they could be. In places I've wanted to use 'sizeof' but don't want to mark the code as 'unsafe'. I've tried to use the 'params' keyword on the input, but this cannot be in conjunction with 'ref'. I've wanted to pass them with a maximum number of T1, T2, etc. using generics, but realised I can't enumerate these. I'm now passing an array of variables in conjunction with the 'ref' keyword, which means creating the array in memory but only the array gets updated with the changed and not the original variables.

byte[] message = new byte[] { }; //some byte array
ushort item1 = default(ushort);
byte item2 = default(byte);
var argumentArray = new object[] { item1, item2 };
ConvertArray(response, ref argumentArray);

private void ConvertArray(byte[] response, ref object[] items)
{
    int index = 0;
    for (int i = 0; i < items.Length; i++)
    {
        var item = items[i];
        var itemType = item.GetType();
        var itemSize = SizeOf(item.GetType());

        if (itemSize == 0)
        {
            continue;
        }
        else if (itemSize == 1)
        {
            items[i] = response[index];
        }
        else
        {
            var method = typeof(BitConverter).GetMethod($"To{itemType.Name}");
            var returned = method.Invoke(null, new object[] { response, index });
            items[i] = Convert.ChangeType(returned, itemType);
        }

        index = index + itemSize;
     }
}

private int SizeOf(Type type)
{
   switch (type.Name)
   {
       case nameof(UInt16):
           return 2;

       case "Byte":
           return 1;

       default:
           return 0;
   }
}

So this is partially working in that 'argumentArray' is updating with the values from the 'ConvertArray' method, but I'm sure there is a neater way to do this using Types.

Ideally I wouldn't need to create the 'argumentArray' and just pass the items (e.g. Item1 and Item2) directly as arguments to the method.

case nameof(UInt16):
           return 2;

       case "Byte":
           return 1;

Be consistent. nameof is clearly preferable.

private void ConvertArray(byte[] response, ref object[] items)
{
    int index = 0;
    for (int i = 0; i < items.Length; i++)
    {
        var item = items[i];
        var itemType = item.GetType();
        var itemSize = SizeOf(item.GetType());

The last line should use the variable itemType.
The sole point of items seems to be to extract types. So why is the parameter not params Type[] types?

        if (itemSize == 0)
        {
            continue;
        }

Would it be better to throw an exception?

        else if (itemSize == 1)
        {
            items[i] = response[index];
        }

That's a bug waiting to happen in the future when you add support for sbyte.

        {
            var method = typeof(BitConverter).GetMethod($"To{itemType.Name}");
            var returned = method.Invoke(null, new object[] { response, index });
            items[i] = Convert.ChangeType(returned, itemType);
        }

Ugh. I would much rather use T4 to generate the entire method and get rid of the reflection. That would probably also eliminate the need for Sizeof.

If you're using a recent .Net (e.g. .Net Core 2.1) then I think some combination of Span<T> and Memory<T> will do what you want. Over the past year or so, Microsoft has done a lot of work on C# features for byte-level memory manipulation. You can probably create a struct for the contents of the packet and coerce the byte[] into your struct with one or two lines of code.


Finding the minimum ratio out of many lines of input This is my code. I am having a file, whose first line's second word contains a number. Let this number is $u. 2nd line is empty, and from third line, I am given with two numbers separated by a space in each line where let first number is $c and second is $w. Now I need to figure out the minimum $b where $b for a line is ceil($u  / $w) * $c. Is there anyway I can optimize my code.

<?php
$f = file($argv[1]);
list(, $u) = explode(" ", $f[0]);
list($c, $w) = explode(" ", $f[2]);
$b = ceil($u / $w) * $c;
unset($f[0], $f[1], $f[2]);

foreach($f as $l)
{
    list($c, $w) = explode(" ", $l);
    $b = min($b, ceil($u / $w) * $c);
}

echo "$b\n";
exit;
?>

I guess my comment really was an answer in a philosophical way.  So here is the answer:

Use names for your variables that have meaning.

There are lots of advantages to naming your variables well.  Many people have looked at this question only with a very abstract idea of what you are trying to do.  This makes it difficult to find improvements.  If people know exactly what you are trying to do then they can work out clever ways to achieve that.

As palacsint said: 'It won't be any slower if you use longer and readable variable names' but it may well be faster when people understand it and can see how to improve it.


Should I split code into smaller functions? Advice on layout I have the following code.  I was wondering if anyone can give me any pointers on how to make this more concise and neater?  Should I be splitting this down into smaller functions?

    private void ReturnValue()
    {

        //create our WebService object
        var sample = new TestProjectService.Service1();
        //create our string to hold the Command information returned
        //select which web service to use
        var Command = new SimpleWebServiceApp.TestProjectService.CommandMessages();

        switch (WebServiceRadio.SelectedIndex)
        {
            case 0:
                Command = sample.GetData();
                break;
            case 1:
                Command = sample.GetDataLINQ();
                break;
        }

        //Check if a value is returned
        if (Command != null)
        {
            //Display result on screen
            LatestCommandLabel.Text = String.Format("Last Command Called (Using {0}) : {1}",WebServiceRadio.SelectedItem.ToString(),Command.Command);

            //Get the current logged in username
            string userName = System.Security.Principal.WindowsIdentity.GetCurrent().Name.ToString();

            //Write command to the database
            TestProjectLinqSQLDataContext dc = new TestProjectLinqSQLDataContext();
            TestProjectCommandMessage msg = new TestProjectCommandMessage();
            msg.Command_Type = Command.CommandType;
            msg.Command = Command.Command;
            msg.DateTimeSent = Command.DateTimeSent;
            msg.DateTimeCreated = Command.DateTimeCreated;
            msg.Created_User = userName;
            msg.Created_Dttm = DateTime.Now;
            dc.TestProjectCommandMessages.InsertOnSubmit(msg);
            dc.SubmitChanges();
        }
        else
        {
            LatestCommandLabel.Text = "No Commands Available";
        }
    }

    protected void GetCommandButton_Click(object sender, EventArgs e)
    {
        ReturnValue();
    }

I would basically write a method wherever you've written a comment on what a block of code does. This allows you to hide the specifics of a particular operation and makes it clearer which code paths are taken and what they do at a high level:

private Service1 GetService()
{
    return new TestProjectService.Service1();
}

private CommandMessages GetCommand(Service1 service)
{
    var command = new SimpleWebServiceApp.TestProjectService.CommandMessages();

    switch (WebServiceRadio.SelectedIndex)
    {
        case 0:
            command = service.GetData();
            break;
        case 1:
            command = service.GetDataLINQ();
            break;
    }

    return command;
}

private void DisplayResult(string text)
{
    LatestCommandLabel.Text = text;
}

private string GetUsername()
{
    return System.Security.Principal.WindowsIdentity.GetCurrent().Name;
}

private void SubmitData(string username, Command command)
{
    var dc = new TestProjectLinqSQLDataContext();

    var msg = new TestProjectCommandMessage
    {
        Command_Type = command.CommandType;
        Command = command.Command;
        DateTimeSent = command.DateTimeSent;
        DateTimeCreated = command.DateTimeCreated;
        Created_User = userName;
        Created_Dttm = DateTime.Now;
    };

    dc.TestProjectCommandMessages.InsertOnSubmit(msg);
    dc.SubmitChanges();
}

private void ReturnValue()
{
    var service = GetService(); // We don't care how we construct the service, just get it
    var command = GetCommand(service); // We don't care what how we get the command, just get it

    if (command != null)
    {
        // Display the result however it needs to be displayed
        DisplayResult(String.Format("Last Command Called (Using {0}) : {1}", WebServiceRadio.SelectedItem.ToString(), command.Command));
        string userName = GetUsername(); // We don't care how we get the username, just get it
        SubmitData(username, command); // We don't care how we submit the data, just submit it
    }
    else
    {
        DisplayResult("No Commands Available");
    }
}

protected void GetCommandButton_Click(object sender, EventArgs e)
{
    ReturnValue();
}