Converting numbers 1-99 to Spanish in Javascript I have written a small program to convert numeric cardinal numbers (i.e, 4, 35) into Spanish. Currently, it only supports numbers 1-99, however I would like to increase it to support numbers of any length.

Before I do this, I am seeking some feedback on my code structure. I feel that I have too many if statements and do not abstract my code enough, and I do not wish for it to be an elongated nightmare of if statements. I am also unsure how I should split up numbers of different sizes, and what variable names to use.

getSpanishCardinal = cardinal => {
  if (0 <= cardinal && cardinal <= 15) {
    return getCardinals()[cardinal];
  }
  if (16 <= cardinal && cardinal <= 19) {
    return sixteenToNineteen(cardinal);
  }
  if (cardinal >= 20 && cardinal <= 99) {
    return twentyToNinetynine(cardinal);
  }
};

twentyToNinetynine = cardinal => {
  if (cardinal % 10 == 0) {
    return getCardinals()[cardinal];
  }
  if (21 <= cardinal && cardinal <= 29) {
    return "veinti" + getSpanishCardinal(rightMostDigit(cardinal)).toString();
  }
  return (
    getCardinals()[leftMostMultiplier(cardinal)].toString() +
    " y " +
    getSpanishCardinal(rightMostDigit(cardinal)).toString()
  );
};

sixteenToNineteen = cardinal => {
  let rightMost = rightMostDigit(cardinal);
  return "dieci" + getSpanishCardinal(rightMost).toString();
};

leftMostMultiplier = cardinal => {
  let newDigit = cardinal.toString()[0];
  const cardinalString = cardinal.toString();
  const length = cardinalString.length;
  for (let i = 1; i < length; i++) {
    newDigit += "0";
  }
  return parseFloat(newDigit);
};

rightMostDigit = cardinal => {
  return cardinal % 10;
};

getCardinals = () => {
  return {
    0: "cero",
    1: "uno",
    2: "dos",
    3: "tres",
    4: "cuatro",
    5: "cinco",
    6: "seis",
    7: "siete",
    8: "ocho",
    9: "nueve",
    10: "diez",
    11: "once",
    12: "doce",
    13: "trece",
    14: "catorce",
    15: "quince",
    20: "veinte",
    30: "treinta",
    40: "cuarenta",
    50: "cincuenta",
    60: "sesenta",
    70: "setenta",
    80: "ochenta",
    90: "noventa",
    100: "cien",
    200: "doscientos",
    300: "trescientos",
    400: "cuatrocientos",
    500: "quinientos",
    600: "seiscientos",
    700: "setecientos",
    800: "ochocientos",
    900: "novecientos",
    1000: "mil",
    1000000: "millón"
  };
};

console.log(getSpanishCardinal(56));

The function declarations are at least lacking a const statement (or let or var if you prefer):

const getSpanishCardinal = cardinal => {

However I would suggest to using regular function statements instead.

The getCardinals function is a bit pointless. A simple constant instead would make more sense:

const CARDINALS =  {
  0: "cero",
  1: "uno",
  // ...
}

For readability I'd also suggest to split it up into separate objects for 0 to 15, the tens, the hundreds, etc.

This would also allow to assign assign the tens to the base number e.g.:

const TENS = {
     2: "veinte",
     3: "treinta",
     4: "cuarenta",
     // ...
}

and then you don't need to "build" numbers in leftMostMultiplier:

const twentyToNinetynine = cardinal => {
  // ...
  return (
    TENS[cardinal / 10] +
    " y " +
    getSpanishCardinal(rightMostDigit(cardinal))
  );
};

(BTW, there seem to be a lot of unnessecary toString()s.)


Writing a for loop to build a String using Streams I have the following code which works. Been trying to write it in Java 8 streams to make it more readable but unable to get my head around it. Looking for any feedback to make it more readable.
Thanks.

// Expected output like: 1~CODE1|2~CODE2|3~CODE3|4~EX|5~CODE5
private String getFormattedValue(List<Content> list) {
    final String tilde = "~";
    final String pipe = "|";
    StringBuilder codes = new StringBuilder();

    for (int i = 0; i < list.size(); i++) {
        String code = list.get(i).getCode();
        if (StringUtils.isNotBlank(code) && !"null".equals(code)) {
            code = "example".equals(code) ? "EX" : code.toUpperCase();
            codes.append(i + 1).append(tilde).append(code);
            codes.append(pipe);
        }
    }
    if (codes.toString().endsWith("|")) {
        codes.deleteCharAt(codes.length() - 1);
    }
    return codes.toString();
}

One issue is that you cannot consume a stream with index. You can more or less have the same result with a IntStream bounded to your list's size.

Then you just have map each int to a code with the same logic. And, finally, collect them (This is the only part where the Stream will shine).

String string = IntStream.range(0, codes.size())
            .mapToObj(i -> {
                String code = codes.get(i);
                code = "example".equals(code) ? "EX":code.toUpperCase();
                return String.format("%1$d~%2$s", i, code);
            })
            .collect(Collectors.joining("|"));


Type name as a string I posted this answer recently. OP in the StackOverflow question mentioned that it was for educational use, but I can imagine it being used as a debugging aid. Then I decided to improve it slightly and make it a compile-time function. Code:

#include <cassert>
#include <iostream>
#include <string_view>
#include <type_traits>
#include <vector>

#ifndef __GNUC__
static_assert(false, "GCC specific");
#endif // !__GNUC__

// Finds a character in a nul terminated string.
// Returns the last character if sought one isn't found.
// Notes:
// 1. Using this because std::strchr isn't constexpr.
// `PChr` is a separate typename instead of just using Chr* to allow `Chr*` and
// `Chr const*` as the pointer.
// 2. pstr shall not be nullptr.
template <typename PChr, typename Chr>
[[gnu::pure, gnu::nonnull, gnu::returns_nonnull, nodiscard]]
static constexpr auto*
constexpr_strchr(PChr pstr, Chr const value) noexcept {
    // PChr must be a raw pointer type because of gnu::nonnull and
    // gnu::returns_nonnull
    static_assert(std::is_pointer_v<PChr>, "PChr must be a raw pointer type");
    auto constexpr nul = Chr{};
    while (*pstr != value && *pstr != nul) {
        ++pstr;
    }
    return pstr;
}

// Returns distance from ptr to the end of the array.
// Notes:
// 1. ptr shall not be nullptr
// 2. ptr shall be inside the array (arr)
template <typename T, auto size>
[[gnu::const, gnu::artificial, gnu::nonnull, gnu::always_inline, nodiscard]]
inline static constexpr auto
distance_to_end(const T (&arr)[size], T const* const ptr) noexcept {
    return arr + size - ptr;
}

// Returns type T as a string_view.
// Ex: std::string -> "std::string"
template <typename T>
[[gnu::const, nodiscard]]
static constexpr auto type_name_finder() noexcept {
    // __PRETTY_FUNCTION__ means "$FUNCTION_SIGNATURE [with T = $TYPE]".

    // +2 here to skip "= "
    auto const* const begin = constexpr_strchr(__PRETTY_FUNCTION__, '=') + 2;

    // -2 meaning up to "]\0"
    auto const size =
            static_cast<std::size_t>(distance_to_end(__PRETTY_FUNCTION__, begin) - 2);
    return std::string_view{begin, size};
}

// Inline string_view with the type name.
template <typename T>
inline constexpr auto type_name = type_name_finder<T>();

// Example Class
template <typename T1, typename T2>
class my_class {};

int main() {
    // Example use-case
    my_class<int&, std::vector<double>> my_arr[20];
    std::cout << type_name<decltype(my_arr)>;
}

Code uses __PRETTY_FUNCTION__ in GCC and Clang to get a variable's type as a string. I would appreciate any comments on correctness and readability. I used a lot of GCC specific attributes, because __PRETTY_FUNCTION__ is GCC specific and for this reason the code can't be portable anyway.

// __PRETTY_FUNCTION__ means "$FUNCTION_SIGNATURE [with T = $TYPE]".

When you call type_name_finder(), the only part that changes in __PRETTY_FUNCTION__ is $TYPE. We can use this information to strip the static decoration surrounding that type.

template <typename T>
static constexpr auto type_name_finder() {
    std::string_view name, prefix, suffix;

#if defined(__clang__)
    name = __PRETTY_FUNCTION__;
    prefix = "auto type_name_finder() [T = ";
    suffix = "]";
#elif defined(__GNUC__)
    name = __PRETTY_FUNCTION__;
    prefix = "constexpr auto type_name_finder() [with T = ";
    suffix = "]";
#endif

    name.remove_prefix(prefix.size());
    name.remove_suffix(suffix.size());
    return name;
}


Is this Spaghetti code? I recently wrote my first webapp and here is the code. I want to make it better, but I'm not exactly sure how.  I believe the first place I should start is with the structure of it. Is this spaghetti code? If yes, how do I change that? You can view it at http://shiftfrog.com

I created a class called 'doer' that basically drives the program. Is that wrong? Should my class be called calendar and then manipulate in my post call to /calendar?

class Doer

  def makeDate(date)
    return Date.strptime(date, "%m/%d/%Y")
  end

  def buildArray(dateObj, on, off)
    array = frontpadding(dateObj, on, off)
    month = dateObj.month
    newMonth = month
    day = dateObj.mday
    date = dateObj
    while month == newMonth
      temp_array = [day,'day']
      array.push(temp_array)
      day = day + 1
      date = date + 1
      newMonth = date.month
    end
    endpadding(date, array)
    array
  end

  def endpadding(dateObj, array)
    dofw = dateObj.wday
    filler = (dofw - 6).abs + 1
    if filler == 7
      #do nothing
    else
      until filler == 0
        temp_array = ['','day']
        array.push(temp_array)
        filler = filler - 1
      end
    end
  end

  def frontpadding(dateObj, on, off)
    array = Array.new
    day = dateObj.mday
    firstOfMonth = Date.new(dateObj.year, dateObj.month)
    filler = firstOfMonth.wday
    on = on.to_i
    off = off.to_i
    mod = on + off
    if day != 1
      @days = day
      @location = @days + filler
      until day <= 1
        off.times do
          if day > 1
            temp_array = ['','day']
            array.unshift(temp_array)
            day = day - 1
          end
        end
        on.times do
          if day > 1
            temp_array = ['','dayOn']
            array.unshift(temp_array)
            day = day - 1
          end
        end
      end
    end
    until filler == 0
        temp_array = ['','day']
        array.unshift(temp_array)
        filler = filler - 1
    end
    array
  end

  def backFill(cal, on, off)
    @location = @location.to_i
    if @location > 0
      @location = @location - 1
      until @days == 0
        cal[0][1][@location][0] = @days
        @days = @days - 1
        @location = @location - 1
      end
    end
    cal
  end

  def makeCal(date, on, off)
    dateObj = makeDate(date)

    @cal = Array.new
    months = 0
    while months < 13
    #   #pass dateobj to build array
      array = buildArray(dateObj, on, off)
    #   #save array to hash with month key
      monthName = Date::MONTHNAMES[dateObj.mon]
      monthYear = "#{monthName} " + dateObj.year.to_s
      holder = Array.new
      holder.push monthYear
      holder.push array
      @cal.push holder
    #   #create new date object using month and set it to the first
      dateObj = Date.new(dateObj.year, dateObj.month)
      dateObj >>= 1
      months = months + 1
    end
    @cal = createCal(@cal, on, off)
    @cal = backFill(@cal, on, off)
  end

  def createCal(cal, on, off)
    on = on.to_i
    off = off.to_i
    @mod = on + off
    daycount = 0
    cal.each do |month|
      month[1].each do |day|
        if day[0] == ''
          #do nothing
        else
          if daycount % @mod < on
            day[1] = 'dayOn'
          end
          daycount = daycount + 1
        end
      end
    end
    cal
  end
end

get '/' do
  haml :ask
end

post '/calendar' do
  @on = params["on"]
  @off = params["off"]
  @date = params["date"]

  a = Doer.new
  @cal = a.makeCal(@date, @on, @off)
  haml :feeling
end

Doer is not a particularly expressive name. Sure, the class does something -- but what class doesn't? Your impulse to name the class Calendar is definitely on target. There are a few other naming things that are not according to Ruby style guidelines; you should avoid CamelCasing your method names -- for instance, makeCal should be make_cal and even better something like make! or create! (assuming the class itself is named calendar, saying @calendar.make_calendar is repeating yourself.)

In general, with respect to 'spaghetti' code, the issue is usually that you are not really separating concerns. The problem that Objects are trying to solve is encapsulating responsibility. It destroys any advantage you stand to gain to put all your functionality into one place. Object-oriented programming involves modularity and encapsulation -- naming a class Doer might indicate some weaknesses in terms of really grokking the paradigm. Consider studying some actual web application source code and see how concerns are separated and handled at an appropriate level of abstraction.

On that last point, one good rule of thumb is that code which is 'nearby' should be operating at roughly the same level of abstraction. Try to identify different concerns and modularize them -- make them 'loosely coupled' to one another. You might make a class which handles the business logic of padding and packing the string stuff, and separate the 'pure' calendar logic into a different class altogether; your application would then just have to 'glue' the two capabilities together.


Kotlin Android request client As part of an android app, I've written a simple wrapper that makes requests to an api and fetches stuff.

import android.app.Activity
import android.util.Log
import android.widget.Toast
import com.github.kittinunf.fuel.core.Parameters
import com.github.kittinunf.fuel.httpGet
import com.github.kittinunf.fuel.json.responseJson
import com.github.kittinunf.result.Result
import com.google.gson.Gson

data class Xkcd(val id: Int, val content: String, val link: String, val title: String)

class XkcdClient(private val main: Activity) {
    private val API = "https://279fbce3.ngrok.io"

    fun search(p: Parameters, callback: (Array<Xkcd>) -> Unit) {
        makeRequest("$API/search", p, callback)
    }

    private inline fun <reified T> makeRequest(url: String, p: Parameters, crossinline callback: (T) -> Unit) {
        buildPath(url, p)
            .httpGet()
            .responseJson { _, _, result ->
                when (result) {
                    is Result.Failure -> {
                        Log.i("failed", "request failed")
                        main.runOnUiThread {
                            Toast.makeText(
                                main,
                                "Request Failed!",
                                Toast.LENGTH_LONG
                            ).show()
                        }
                    }
                    is Result.Success -> {
                        val json = result.get().obj()
                        Log.i("json", json.toString())
                        val res = deserialize<T>(json.get("results").toString())
                        main.runOnUiThread { callback(res) }
                    }
                }
            }
    }
}

inline fun <reified T> deserialize(content: String): T {
    return Gson().fromJson(content, T::class.java)
}

fun buildPath(url: String, params: Parameters): String {
    var url = url
    if (params.isNotEmpty()) {
        url += "?"
    }

    for ((p, v) in params) {
        url += "$p=$v&"
    }
    return url.substring(0, url.lastIndex)
}

It works as I want it to. As such, I'm looking for some advice on how I can make it more idiomatic/elegant.

Just FYI: I've written the buildPath myself, as the http library I'm using (kittinuf.fuel) has a bug in its parameter setting method. When it's fixed, I'll be switching to its (hopefully correct) implementation. My method has at least one known bug in it, where if the base url has a trailing slash, it will return a url with double slashes. To avoid getting into all hairy edges cases, I've made sure my hardcoded base url is fine and stripped of a trailing slash.

You should look into coroutines.
Coroutines are the Promises from JavaScript and the CompleteableFutures of Java.
It's basically Kotlin handeling the callbacks for you.

suspend fun getString() : String {
    return "t"
}
suspend fun main() {
    val t = getString()
    println(t)
}
//is roughly the same as:
fun getString(lamb: (String)->Unit){
    lamb("t")
}
fun main() {
    getString{
        println(t)
    }
}

You don't really need to know the implementation, but just some rules:

suspend functions can only be called from suspend-functions (because of the hidden parameter)
Suspend functions allow you to replace lammbda's with normal code, even try catch blocks!!!

in our case, we can change makeRequest to use suspension functions:

private inline suspend fun <reified T> makeRequest(url: String, p: Parameters) {
    val json = buildPath(url, p)
        .httpGet()
        .awaitObjectResult(jsonDeserializer()).obj()
    Log.i("json", json.toString())
    return deserialize<T>(json.get("results").toString())
}

change the search with:

suspend fun search(p: Parameters) = makeRequest("$API/search", p, callback)

then in MainActivity:

class MainActivity : AppCompatActivity(), CoroutineScope by MainScope() {
    override fun onDestroy() {
        cancel() // cancel is extension on CoroutineScope
    }

    //you can compare using launch with having a second thread.
    fun showSomeData() = launch {
        try {
            val text = XkcdClient(private val main: Activity).search(..)
        } catch (e : Exception) {
            Log.i("failed", "request failed")
            Toast.makeText(
                main,
                "Request Failed!",
                Toast.LENGTH_LONG
            ).show()
        }
    }
}

extra dependencies:

implementation 'org.jetbrains.kotlinx:kotlinx-coroutines-android:1.3.2'
implementation 'com.github.kittinunf.fuel:fuel-coroutines:'
implementation 'org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.2'

extra info

https://medium.com/androiddevelopers/coroutines-on-android-part-i-getting-the-background-3e0e54d20bb
https://kotlinlang.org/docs/reference/coroutines/coroutines-guide.html


Write binary save file in Python I'm fixed on the save file format. There are only three types of data:

long (NumPy int32 array, independent of platform)
double (NumPy double array)
string (UTF-8, but ord(c) < 128 for all characters)

whereby the first two get saved in their binary representation. Strings get saved as string and are preceded with an int32 with its length (not fixed along one column).

I have input data of the form:

bun = {'x': np.zeros(n_col),
       'y': np.zeros(n_col, dtype=np.int32),
       'z': np.zeros(n_col),
       'u': ['abc'] * n_col}

And I need to save it line-by-line.

Pseudo code:

for i in range(n_col):
     for col in ordered_colum_names:
           save(bun[col][i])

As so many file IO's can get costly (yes, I tried it first and I have up to 1M lines) and the files are limited to be below 150MB I decided to write everything into a bytes buffer and then write in one go to the disk. But I feel my code could be simplified. Please let me know what your thoughts about it.

import io
from collections import OrderedDict

import numpy as np

###############################
### Functions to comment on ###
###############################

def join(*args):
    return b''.join(args[0])

def array2binary_list(bin):
    """ Converts an numpy array into an list of bytes.
    !!! Note returns generator !!! """
    return map(bytes, zip(*[iter(bin.tobytes())] * bin.itemsize))

def write_binary_col(fid, bun, col):
    """ Write down the data in the variable space format.
    For int32 and double this means just the binary.
    For str this means ascii representation preceded with the str length in
    int32 """
    raw = []

    for key, val in col.items():
        if (val['type'] == 'double') or (val['type'] == 'long'):
            raw.append(array2binary_list(bun[key]))
        elif val['type'] == 'string':
            str_len = np.array([len(line) for line in bun[key]], dtype=np.int32)
            raw.append(array2binary_list(str_len))
            raw.append(line.encode('ascii') for line in bun[key])
        else:
            raise TypeError('Unknown Type {}'.format(val['type']))

    # Transpose and serialize it first
    fid.write(b''.join(map(join, zip(*raw))))

#########################################################
### Some helper functions to illustrate the problem ###
#########################################################

def create_data(n_col):
    """ This generates just some fake data """
    bun = {'x': np.zeros(n_col),
           'y': np.zeros(n_col, dtype=np.int32),
           'z': np.zeros(n_col),
           'u': ['abc'] * n_col}

    col = OrderedDict()
    col['x'] = {'type': 'double'}
    col['y'] = {'type': 'long'}
    col['z'] = {'type': 'double'}
    col['u'] = {'type': 'string'}

    return (bun, col)

if __name__ == '__main__':
    fid = io.BytesIO()
    bun, col = create_data(100)

    write_binary_col(fid, bun, col)

    print(fid.getvalue())

Your join function is confusing, why use *args if you're only going to take args[0]? As far as I can see you could just rewrite it like this:

def join(arg):
    return b''.join(arg)

The only difference is that you can't pass arbitrary unused parameters now, but that would communicate the function's use better. If you have to use this as a work around then you should note why it's necessary with a docstring, since it's not obvious from context and your function just looks like a convenient way to build bytestrings.

def join(*args):
    """Return a bytestring of the first argument's elements joined.

    Will ignore all arguments after the first.
    This is necessary to deal with the foo bar."""

Semantically, I think it might make sense for you to raise a ValueError for the unknown type error. Backwards as it sounds, the user has passed the correct type (a dictionary) but that dictionary has an invalid value. It doesn't matter that the value indicates a type, the error the user has made is with the values they provided.


Insect combinatorics challenge Below is the problem taken from Berkeley's Cs61A page here

  Question 9: Insect Combinatorics*

  Consider an insect in an M by N grid. The insect starts at the bottom left corner, (0, 0), and wants to end up at the top right corner, (M-1, N-1). The insect is only capable of moving right or up. Write a function paths that takes a grid length and width and returns the number of different paths the insect can take from the start to the goal. (There is a closed-form solution to this problem, but try to answer it procedurally using recursion.)

  For example, the 2 by 2 grid has a total of two ways for the insect to move from the start to the goal. For the 3 by 3 grid, the insect has 6 diferent paths (only 3 are shown above).

def paths(m, n):
    """Return the number of paths from one corner of an
    M by N grid to the opposite corner.

    >>> paths(2, 2)
    2
    >>> paths(5, 7)
    210
    >>> paths(117, 1)
    1
    >>> paths(1, 157)
    1
    """
    "*** YOUR CODE HERE ***"

This solution is with the knowledge of 'higher order function' and 'recursion'. I've yet to learn data structures and algorithms (if required).

Idea: Started from the destination and found the possibilities. As per the skill level, the solution took 3 hours of my time. Please provide feedback on this.

def paths(m, n):
    """Return the number of paths from one corner of an
    M by N grid to the opposite corner.

    >>> paths(2, 2)
    2
    >>> paths(5, 7)
    210
    >>> paths(117, 1)
    1
    >>> paths(1, 157)
    1
    """
    count_paths = 0
    def find_number_of_paths(x, y):
        if x == 0 and y == 0:
            nonlocal count_paths
            count_paths += 1
            return
        if x > 0:
            find_number_of_paths(x-1, y)
        if y > 0:
            find_number_of_paths(x, y-1)
    find_number_of_paths(m-1, n-1)
    return count_paths

Can we avoid re-assignment operator on count_paths?
Can we avoid nested function definitions?
Is there a name for above solution approach in algorithm world? Any better approach?

Note: As per this assignment, no usage of data model is recommended.

Using nonlocal rather than global is certainly good, but better yet would be returning values.

def paths(m, n):
    def find_number_of_paths(x, y):
        if x == 0 and y == 0:
            return 1

        ret = 0

        if x > 0:
            ret += find_number_of_paths(x-1, y)

        if y > 0:
            ret += find_number_of_paths(x, y-1)

        return ret

    return find_number_of_paths(m-1, n-1)

That lets us elide the outer function entirely:

def paths(x, y):
    if x == 1 and y == 1:
        return 1

    ret = 0

    if x > 1:
        ret += paths(x-1, y)

    if y > 1:
        ret += paths(x, y-1)

    return ret

It's a bit strange to critique this since "no closed-form solution" basically means no good solution. There are ways of speeding this up, though, that avoid that. A trivial one is memoization:

_paths_cache = {(1, 1): 1}
def paths(x, y):
    if (x, y) in _paths_cache:
        return _paths_cache[x, y]

    ret = 0

    if x > 1:
        ret += paths(x-1, y)

    if y > 1:
        ret += paths(x, y-1)

    _paths_cache[x, y] = ret
    return ret


Listing Docker Images without registry URI I have a private container registry behind a firewall that will need some Google registry images. To make sure things are updated sanely, I'm writing a mirror script in Bash. I have one line that lists the images without their registry URIs, but it's overly complicated and I am piping through 3 awk statements.

docker images | awk '{printf("%s\:%s\n", $1, $2)}' | awk 'NR>1' | awk -F '/' '{print $3}'

First awk statement:

$ docker images | awk '{printf("%s\:%s\n", $1, $2)}'
REPOSITORY:TAG
gcr.io/google_containers/skydns:2015-10-13-8c72f8c

Second awk statement:

$ docker images | awk '{printf("%s\:%s\n", $1, $2)}' | awk 'NR>1'
gcr.io/google_containers/skydns:2015-10-13-8c72f8c

Third awk statement:

$ docker images | awk '{printf("%s\:%s\n", $1, $2)}' | awk 'NR>1' | awk -F '/' '{print $3}'
skydns:2015-10-13-8c72f8c

How can I reduce this to one awk statement?

It looks like the purpose of the second command is to skip the REPOSITORY:TAG table header, which would always be the first line of the docker images output.  You can therefore combine the first two awk commands as:

awk 'NR > 1 { printf("%s:%s\n", $1, $2) }'

To keep just the trailing part of the REPOSITORY field, you could do gsub('.*/', '', $1).  Note that that is not the same as awk -F '/' '{print $3}', but likely better.

Therefore, I would write the solution as

docker images | awk 'NR > 1 { gsub('.*/', '', $1); printf("%s:%s\n", $1, $2); }'


Kill/stop a busy FutureTask Thread I am trying to get an Auth ticket from a 3rd party server. I am using FutureTask to perform the task. However, if I don't get response from the server after 10 seconds, I want to timeout the call. At the same time, I want to kill the Thread which was busy in fetching the Auth Ticket from 3rd party, but got stuck because of some issue. Here is the simplified code.

public String getAuthTicket(final String userName) throws Exception{
    String authTicket=null;
    final FutureTask<String> future =
            new FutureTask<String>(new Callable<String>() {
                public String call() throws Exception {
                    log.info("calling server to get trusted ticket.");
                    if(Thread.interrupted()){
                        log.info("Call to server got interrupted.");
                        return "-1";
                    }
                    String authTicket = getTrustedTicket(userName);
                    return authTicket;
                }
            });

    final Thread thread = new Thread(future);
    thread.start();
    String authTicket ="-1";

    try {
        authTicket = future.get(10000, TimeUnit.MILLISECONDS);
        log.info("authTicket: "+ authTicket);
        return authTicket;
    } catch (Exception e){
        log.warning("Timeout occured while waiting to get auth ticket: Reason:"+ e);
        future.cancel(true);
    }

    return authTicket;
}

Could anyone please review my code?

Instead of starting a new thread you can instead create a executor which will take Runnables and reuse threads as needed. you can create a executor usign one of the Executors' static factories.

This code relies on getTrustedTicket() responding correctly to a thread interruption. Which you didn't include so I can't judge it. (A lot of IO functions don't respond to it by default).

The catch-all when waiting on the result is not a great thing. You should instead catch only the specific exceptions you can get and only declare those you throw.

try {
    authTicket = future.get(10000, TimeUnit.MILLISECONDS);
    log.info("authTicket: "+ authTicket);
    return authTicket;
} catch (InterruptedException e){
    log.warning("Current thread interrupted while waiting to get auth ticket: Reason:"+ e);
    future.cancel(true);
    Thread.currentThread().interrupt(); //reset interrupted state or rethrow the exception.
} catch (ExecutionException e){
    log.warning("Exception occured getting the auth ticket: Reason:"+ e.getCause());
} catch (TimeoutException e){
    log.warning("Timeout occurred while waiting to get auth ticket: Reason:"+ e);
    future.cancel(true);
}


Shift a python dictionnary values I wrote this function today that shift the values of a dictionnary:

def shift_dict(dic, shift):
    # Save the dic length
    dic_len = len(dic)

    # Reduce the shift to the dic length
    shift = shift % dic_len

    # convert the dict to a tupple to reference by index
    list_dic = [(k,v) for k, v in dic.iteritems()]

    # Create the new shifted dict
    shifted = {
        list_dic[x][0]: list_dic[ (x - shift) % dic_len ][1]
        for x in xrange(dic_len)
    }

    return shifted

The idea is to keep the keys but to assign the nth - shift value to it.

A few examples:

dic = {
    'a':10,
    'b':2,
    'c':30,
    'd':4,
    'e':50,
    'f':6,
}
print dic
print shift_dict(dic, 1)
print shift_dict(dic, 4)
print shift_dict(dic, -2)

Result:

{'a': 10, 'c': 30, 'b': 2, 'e': 50, 'd': 4, 'f': 6}
{'a': 6, 'c': 10, 'b': 30, 'e': 2, 'd': 50, 'f': 4}
{'a': 2, 'c': 50, 'b': 4, 'e': 6, 'd': 10, 'f': 30}
{'a': 30, 'c': 2, 'b': 50, 'e': 4, 'd': 6, 'f': 10}

I find that the "create a list and then another dictionnary" is not very effective, but I couldn't think of another way to do it.

Any thoughts?

Use OrderedDict if you want an ordered dictionary.

from collections import OrderedDict
from itertools import islice, cycle

def shift_dict(dct, shift):
    shift %= len(dct)
    return OrderedDict(
        (k, v)
        for k, v in zip(dct.keys(), islice(cycle(dct.values()), shift, None))
    )


C++ singleton using templates Is this a good Singleton implementation? Is there anything I should be aware of? If so, how can it be improved?

template <class T>
class Singleton
{
public:
    static T& getInstance()
    {
        static T instance;
        return instance;
    }

protected:
    Singleton() {}
    ~Singleton() {}

private:
    Singleton(Singleton const&);
    void operator=(Singleton const&);
};

Usage example:

#include <stdio.h>

class A : public Singleton<A> {
public:
    A() {
        a = 100;
    }

    int get_a() { return a; }
    void set_a(int v) { a = v;  }
private:
    int a;
};

A& s = A::getInstance();

void free1() {
    printf("%d\n", s.get_a());
}

void free2() {
    s.set_a(200);
}

void free3() {
    printf("--> %d", s.get_a());
}

void main() {
    free1();
    free2();
    free3();
}

Let's consider an alternative to avoid the usage of Singletons, instead having:

class A : public Singleton<A>

let's say it becomes:

class A

and let's say we define a global variable:

A g_s = A();

and then this global variable will be accessed by other files using external, would that bring some sort of benefit?

external A g_s;

You cannot really say that it's a good or bad implementation. It depends, but this doesn't look ok to me: class A : public Singleton<A>.
You don't need to inherit from it to use it: Singleton<A>::instance()

If you want to to use it as you wrote: A& s = A::getInstance(); then probably A's ctor/dtor shouldn't be public.


Single Rotate Method I just started coding a simulation-like application. It's about our solar system. Right now, I'm working on rotating the moon. I have this constructor

function Mass(elm) {
    if (typeof elm == "string") {
        this.elm = document.getElementById(elm);
    } else {
        this.elm = elm;
    }
    this.width      = parseInt(window.getComputedStyle(this.elm).width, 10);
    this.height     = parseInt(window.getComputedStyle(this.elm).height, 10);
    this.top        = parseInt(window.getComputedStyle(this.elm).top, 10);
    this.left       = parseInt(window.getComputedStyle(this.elm).left, 10);
    this.radius     = this.width / 2;
    this.origin     = {
        x : this.left + this.width / 2,
        y : this.top + this.height / 2,
    };
    this.angle = 0;
    this.rotation   = function () {
        if (this.angle >= 360) { this.angle = 0; }
        this.angle += 1;
        this.elm.style.webkitTransform = "rotate(" + this.angle + "deg)";
    };
    this.rotate = function (mthd) {
        setInterval(mthd, 100);
    }
}
var moon = new Mass("moon");
moon.rotate("moon.rotation()");

The issue I'm having is this moon.rotate("moon.rotation()");. There are two methods, and the way I call them is um.. weird? I want to use a single method to rotate the object. Like moon.rotate().

I tried a few ways to put the code in just one method, but couldn't figure it out.

Fiddle

Even though I'm done with the problem mentioned in this question, I wanted to add something, the full code. In case if anyone is interested.

Fiddle

For one, you don't want to pass a string to setInterval(); better to use a function. You can bind that function to the object's context, so it'll call the right thing.

For another, aim for less ambiguous names. rotation should probably be updateRotation, since that's what it does. And rotate should likely be startRotating (and they you could have an equivalent stopRotating).

That be something like:

this.startRotating = function () {
  var self = this; // to maintain the object's context
  setInterval(function () {
    self.updateRotation();
  }, 100);
};

However, if you're planning to add more planets etc., it would probably be nicer to have one central timer that calls an update function on all your objects.

And I'd also define a "proper" prototype for all this, and extract some general logic/avoid repetition:

// extract this logic, since it's generally useful
function getElement(element) {
  if (typeof element == "string") {
    return document.getElementById(element);
  }
  return element;
}

function Mass(element, angularVelocity) { // added the rotation speed as an arg
  var computedStyle;

  this.element = getElement(element);

  // fetch this once
  computedStyle = window.getComputedStyle(this.element);

  this.width  = parseInt(computedStyle.width, 10);
  this.height = parseInt(computedStyle.height, 10);
  this.top    = parseInt(computedStyle.top, 10);
  this.left   = parseInt(computedStyle.left, 10);
  this.radius = this.width / 2;

  this.origin = {
    x: this.left + this.radius, // you just calculated the radius; might as well use it here
    y: this.top + this.radius
  };

  this.angularVelocity = angularVelocity;
  this.rotation = 0;
}

// make nice prototype
Mass.prototype = {
  update: function () {
    this.rotation = (this.rotation + this.angularVelocity) % 360;
    this.element.style.webkitTransform = "rotate(" + this.rotation + "deg)";
  }
};

// do the timer magic outside the object
var moon = new Mass("moon", 1.0),
    timer;

timer = setInterval(function () {
  moon.update();
}, 100);

fiddle


Single Linked List JavaScript implementation How can I improve my singly linked list implementation?

"use strict";

var LinkedList = function(){
  this.head = null;
  this.tail = null;
  this.count = null;
}
var Node = function(value){
  this.value = value;
  this.next = null;
}

LinkedList.prototype.addToHead = function(value){
  var newNode = new Node(value);
  if(this.head) {
    this.next = this.head;
    this.head = newNode
    this.count++;
  }
  else this.tail = newNode
}
LinkedList.prototype.addToTail = function(value){
  var newNode = new Node(value);
  if(this.tail) {
    this.tail.next = newNode
  }
  else this.tail = newNode;
}
var ll = new LinkedList();
ll.addToHead(3);
ll.addToHead(4);
ll.addToHead(2);
ll.addToTail(6);
console.log(JSON.stringify(ll))

This doesn't appear to function correctly right now so it may actually belong on Stack Overflow rather than on Code Review.

Regardless, here are the issues I'm seeing:

When you attempt to add values to the head of the list, they instead are getting added to the tail of the list since the head of the list is always null. The new value should always end up as the head of the list when it is added to the head of the list.
Your code is a bit visually confusing since you are using this to refer to 2 different objects in the same code block.
You are only incrementing the count of the list's elements inside the part of the addToHead which is unreachable, resulting in a count of null regardless of what you do. Instead the count should be incremented on any add interaction.
There doesn't seem to be an interaction to remove an item from the list, either front or back. Being able to remove items from the list is a core part of the functionality.

For the full definition of what a singly linked list needs to support, see this page.


Getting a local file with file_get_contents my url is like page.php?path=content/x/y/z/aaa.md. Is the following php code secure?

include "Parsedown.php";

function path_purifier($path) {
  if(substr($path, 0, 8) !== "content/")
    return null;
  if (strpos($path,'..') !== false)
    return null;
  return "./" . $path;
}

$parsedown = new Parsedown();
$path = $_GET['path'];
$path = path_purifier($path);
echo $parsedown->text(file_get_contents($path));

As @KIKO stated, this is a bit dangerous to use relative paths (here, contents/ which is ./contents) because if your php script becomes included or reused, relative path (.) may points to a different location.

You can create a constant or put that absolute path in a config file. At the end, you'll check that the absolute path given by user is beginning by the expected absolute path:

define('THIS_ROOT','/this/is/my/file/root/');

function path_check($path) {
   return strncmp(THIS_ROOT, realpath($path), strlen(THIS_ROOT)) == 0;
}

If the given $path does not exist, realpath will return false so this check manages both security and resource existence.


Preparing a walkthrough: Freeze effect :s I have a method that load my walkthrough view. I have my view controller composed of a scroll view. And others views controllers used only for the walkthrough (maybe I should used xib uiview instead of uiviewcontroller). And when I'm on the viewWillAppear, I'm calling this method:

/**
Prepare dynamicly the walkthrough views ( app presentation ), like the final content size of `myScrollView, the origin of the views, etc..
So if we want to add another view on our walkthrough, we just add it to `listPages` and on the storyboard.

Copied from an old personal project
*/
func prepareWalkthroughViews() {

    // I use a contentView with the scroll view because of the auto layout.
    var i = CGFloat(self.listPages.count-1)

    // We iterate each view controller. We add the views on the inverse order, like a stack
    for currentController in reverse(self.listPages) {
        contentView.addSubview(currentController.view)
        var currentFrame       = currentController.view.frame
        currentFrame.origin.x  = i-- * currentFrame.width // ex : 6 * 320
        currentController.view.frame = currentFrame
    }

    self.myScrollView.addSubview(self.contentView)
    let scrollWidth  = CGFloat(self.listPages.count) * self.view.frame.width
    let scrollHeight = self.myScrollView.frame.height
    self.myScrollView.contentSize = CGSizeMake(scrollWidth, self.contentView.frame.height)
}

When my segue is performed, there is something like 2-3 seconds of freeze (the time the method is executed).

How can this be improved?

I thought about adding some properties global so I can call the method in background queue when I launch the app, but I think that's not very safe to have a lot of global properties everywhere.

The problem here isn't going to just be that the segue is performed slowly.  At a certain point, if you add enough view controller's views to this scroll view, it's very likely that you'll run into memory issues as well, particularly on mobile devices.

We have a scroll view.  Only one screen's worth of the scroll view can be on the screen at any given time.  But in viewWillAppear, we are attempting to load several screen's worth of views onto our scroll view all at once.  This takes time.  And the more views you are adding to it, the more time it takes.

Consider UITableView and UICollectionView are both direct subclasses of UIScrollview.  How do these guys handle adding views to the scroll view?  They do it one at a time, as needed.

If you put a breakpoint or any sort of print statement in the cellForRowAtIndexPath: (or the collection view equivalent), you'll see that this method is only called for any given cell just as that index path is about to scroll on screen.

Using your approach of adding at the views at the same time means you'll holding them all in memory at once, so there's a top end cap on how many views you can stick on there, and it means you're loading them all at once, so there's a big block of time where it takes to run through adding them to the scroll view.  With a table view or collection view, it's not necessarily any faster to add each individual view to the screen, but only the ones necessary are added at once.  Moreover, as views scroll off the screen, they're released from memory as new ones are added.  So you can have an unlimited number of views on the collection view without running out of memory and without it taking an unmanageably long amount of time to load.

Instead of re-inventing the wheel and try to implement some UIScrollViewDelegate methods by hand to add and remove your views as they're needed, I instead recommend that you change your scroll view to a collection view and work with that.


MaxHeap implementation in JavaScript Here is MaxHeap implementation in pure JavaScript:

/**
 * We initialize index 0 because, calculations for finding
 * children of a node and the parent of a node is easier
 * parent = [ k/2 ], children: [ k*2, k*2+1 ]
 */
class Heap {
    constructor() {
        this.queue = [0];
    };

    /**
     * Bottom-up reheapify (swim) : If the heap order is violated because a node's key
     * becomes larger than that node's parents key, then we can make progress
     * toward fixing the violation by exchanging the node with its parent
     *
     * @param {Number} k
     */
    swim(k) {
        while ( k>1 && this.less(k, Heap.flr(k/2)) ) {
            this.exch(k, Heap.flr(k/2) ) ;
            k = Heap.flr(k/2);
        }
    };

    /**
     * Top-down heapify (sink) :  If the heap order is violated because a node's key becomes
     * smaller than one or both of that node's children's keys, then we can make progress
     * toward fixing the violation by exchanging the node with the larger of its two children.
     *
     * @param {Number} k
     */
    sink(k) {
        while ( 2*k <= this.n() ) {
            const c1 = 2*k;
            const c2 = 2*k + 1;
            const j = this.more(c1,c2)?c2:c1;

            if (this.more(k, j) ) {
                this.exch(k, j);
            } else {
                break;
            }
            k = j;
        }
    };

    /**
     *
     * @param {Number} i
     * @param {Number} j
     */
    less(i,j) {
        return this.queue[i]>this.queue[j];
    };

    /**
     *
     * @param {Number} i
     * @param {Number} j
     */
    more(i,j) {
        return this.queue[i]<this.queue[j];
    };

    /**
     *
     * @param {Number} i
     * @param {Number} j
     */
    exch(i, j) {
        const tmp = this.queue[i];
        this.queue[i] = this.queue[j];
        this.queue[j] = tmp;
    };

    /**
     * Inserts element into the internal queue
     * @param {Number} k
     */
    insert(k) {
        this.queue.push(k);
        this.swim(this.n());
    };

    /**
     * Returns max element in the internal queue
     */
    max() {
        if (!this.isEmpty() ) {
            return this.queue[1];
        }
        return false;
    };

    /**
     * Deletes and returns max element from the internal queue
     */
    delMax() {
        if (!this.isEmpty()) {
            const mx = this.queue[1];
            this.queue[1] = this.queue.pop();
            if (!this.isEmpty()) {
                this.sink(1);
            }
            return mx;
        };
        return false;
    };

    /**
     * Checks for emptyness (here we consider the queue empty if the length of the queue is 1)
     */
    isEmpty() {
        return this.queue.length === 1;
    };

    /**
     * Returns last element index in the internal queue
     */
    n() {
        return this.queue.length-1;
    };

    /**
     * in JS dividing odd number by even number gives fraction not integer,
     * we need to get rid of this part
     * @param {Number} i
     */
    static flr(i) {
        return Math.floor(i);
    }
}

Is the implementation correct? What are the shortcomings that can be fixed?

Bug

There's a bug in delMax:

const mx = this.queue[1];
this.queue[1] = this.queue.pop();
if (!this.isEmpty()) {
    this.sink(1);
}
return mx;

What will happen when the heap has 1 element? The code will:

Replace the first element with itself (the last element)
It doesn't become empty

In other words, the last element cannot be deleted.

Avoid hacky solutions

This code happens to work, but it's hacky:

while ( 2*k <= this.n() ) {
    const c1 = 2*k;
    const c2 = 2*k + 1;
    const j = this.more(c1,c2)?c2:c1;

What's wrong with it? When 2*k == this.n(), then c2 will point past the end of the queue, and more will compare queue[c1] with undefined, and always return false. The code works, but it would be better to not rely on numeric comparisons with undefined.

Look for generalization opportunities

It's a good idea to use a function to decide the ordering of elements, similar to what you did with less and more. This could open the opportunity to make the implementation work with arbitrary ordering.

The current implementation doesn't make it easy to pass in these functions to customize the behavior. It would be good to make that possible. And it would be even better to make it work with a single function, let's say less.

If you do that, then you should rename some methods, since max and delMax won't make sense for a min-heap ordering. You could rename these to peek and pop.

Hide implementation details

The class exposes many methods that are unnecessary for users:
swim, sink, less, more, exch, flr.
These are implementation details, and it would be best to hide them.

Drop the unnecessary trick

This trick:

/**
 * We initialize index 0 because, calculations for finding
 * children of a node and the parent of a node is easier
 * parent = [ k/2 ], children: [ k*2, k*2+1 ]
 */

Looks completely unnecessary... I don't see a reason whatsoever why the dummy first element helps. You can eliminate it, and the implementation will be simpler and clearer.

Dividing by 2

I'm not sure if this is recommended in JavaScript world,
but instead of Math.floor(x / 2),
x >> 1 feels cleaner.

Naming

Some method names could be improved:

exch -> swap
n -> size
insert -> add
flr -> floor, or instead of always calling it with flr(k / 2) you could name it childIndex and call it with k, letting it do the necessary math
flr uses parameter i. The name i is best in simple counting loops.


Prime numbers generator This is a prime numbers generator class, loosely based on the sieve of Eratosthenes. It is supposed to generate a decent quantity (i.e. millions) of prime numbers.

static class PrimesGenerator
{
    private const int StartFrom = 2;

    public static IEnumerable<int> GetPrimes()
    {
        var number = StartFrom;
        var dividers = new List<int>();
        while (true)
        {
            if (!IsDividedBy(number, dividers))
            {
                yield return number;
                dividers.Add(number);
            }
            number++;
        }

        bool IsDividedBy(int inputValue, IEnumerable<int> increasingSeriesOfDividers)
        {
            foreach (var divider in increasingSeriesOfDividers)
            {
                if (divider * divider > inputValue)
                    return false;
                if (inputValue % divider == 0)
                    return true;
            }

            return false;
        }
    }
}

For C# 6.0 or earlier IsDividedBy can be written as a private method instead of a local function.

Here's a console application I've used to estimate the generation time:

class Program
{
    static void Main(string[] args)
    {
        var stopwatch = new Stopwatch();
        stopwatch.Start();
        var primes = PrimesGenerator.GetPrimes().Take(1000000).ToList();
        stopwatch.Stop();
        Console.WriteLine(string.Join(", ", primes));
        Console.WriteLine($"Time elapsed: {stopwatch.Elapsed}");
    }
}

What I want to know:

Is there a way to get rid of the dividers list, saving the memory, without decreasing the performance? (by "performance" here I mean time needed to generate the sequence)
How can I decrease the computational complexity of this code? For now, the bigger value the current value is, the noticeably slower it works.
How this code can be improved in a broad sense?

There's a lot to be improved here.  What you call dividers would be simply called primes.  This may have loosely been inspired by a sieve, but its inner mechanics are nothing like a sieve.  It's no surprise that performance degrades with larger numbers, and if you go too high, you may run into memory issues since the dividers (or primes) keeps a list in memory.

Performance should also suffer because you check every number from 2 onward.  You could check 2 to eliminate all evens first, and then only check odd numbers after that.

We discourage one line calls here unless enclosed in braces.  So change this:

if (divider * divider > inputValue)
    return false;

To this:

if (divider * divider > inputValue)
{
    return false;
}

You may want to investigate using a sieve.  Here are old examples:

Sieve31, my sieve of Eratosthenes returning IEnumerable<int>

Sieve32, a simple 32 bit sieve returning IEnumerable<uint> using C#

Sieve32FastV2 - A fast parallel Sieve of Eratosthenes


bash script to organize files and cleanup files I have been working on a bash script for a while now to organize my movie files.  I can do it by hand, but at the time I was taking a bash scripting class and thought I would like to use it to create this project.  I don't know if there is a better way of doing it or not, but I would like you to take a look at my script and give me feedback on it.  I believe it works, but I haven't put it through the paces yet.

#!/bin/bash

MovieList="points to .txt file with name of files"
DirList="points to a .txt file with the name to use to create the directory"
dest_Dir="where to put the files "
src_Dir="where to look for the files"
CreateLogFile="log file of directory created"
MoveLogFile="log file of move files"

# Will read in a list of movies and make the directories for them

createDirectory()
{
    while read -r line
    do
    if [ ! -d $dest_Dir"$line" ]
    then
            mkdir -v $dest_Dir"$line" >> $CreateLogFile
    else
            :
    fi
done < "$DirList"
}
# end createDirectory

# After directories have been created will go and find those movies and move
# them to their proper directory

movieMovies()
{
    while read -r line
do
    find $src_Dir  -type f \( -iname "$line" ! -iname ".*" \) | xargs -I '{}' mv -v {} $dest_Dir"$line" >> $MoveLogFile
done < "$MovieList"
}
# end of moveMovies

# Time to tidy up by going back and seeing if there is any empty directory
# and if so removing said directory

DirectoryEmpty()
{
while read -r line
do
    if [ $(ls -A $dest_Dir"$line" | wc -l) -eq 0 ]
    then
        rmdir $dest_Dir"$line"
    else
        :
    fi
done < "$DirList"
}
# end of DirectoryEmpty

# ---------------------------------Main---------------------------------------
createDirectory
moveMovies
DirectoryEmpty
# ----------------------------------End---------------------------------------

errors

typo: function named movieMovie, invoked as moveMovie

potential errors

use of $dest_Dir"$line"

you expect dest_Dir to end with a slash, but that's not validated anywhere
use "$dest_Dir/$line" instead (it's OK to have multiple slashes)

don't use ls to verify the presence/absence of files:

removeEmptyDirectory() {
    dir="$1"
    shopt -s globstar nullglob dotglob
    files=("$dir"/*)  # an array holding all the filenames
    if (( ${#files[@]} == 0 )); then
        # the directory is empty
        rmdir "$dir"
    else
        echo "warning: directory not empty: $dir" >&2
    fi
}

Even better, use find:

removeEmptyDirectories() {
    while read -r line; do
        find "$dest_Dir/$line" -depth 0 -empty -exec rmdir '{}' \;
    done < "$DirList"
}

Double quote all your variables, everywhere, unless you have a specific reason not to.

style

clean up your indentation.  It is idiomatic to put then and do at the end of the same line as if and while (see removeEmptyDirectories() example above).
use a consistent naming convention

variables DirList, src_Dir -- use camelcase or underscores, but both?

don't need the else : branch
"line" is not a descriptive variable name.
use find -exec instead of piping to xargs.


Program to move in a 2d array given instructions, array and starting position Original problem (not in English): https://pokval21.kattis.com/problems/pokval21.robotdammsugaren
How can my code be optimized?
import sys
j = 0
instructions = []
room = []
for i in sys.stdin:
    inp = i.split()
    if(j == 0):
        r = int(inp[0])
        c = int(inp[1])
        k = int(inp[2])
    if(j==1):
        instructions = list(inp[0])
    if(j >= 2):
        room.append(list(inp[0]))
    if(len(room) == r):
        break
    j += 1
def findStartPos(c, room):
    for y, sublist in enumerate(room):
        pos = []
        if c in sublist:
            x = room[y].index(c)
            pos.append(x)
            pos.append(y)
            return pos
    return -1
botPos = findStartPos("O", room) # [x, y]
x = botPos[0]
y = botPos[1]
i = 0
visited = [botPos]
clean = 1
append = visited.append
for i in instructions:
    if(i == ">"):
        while room[y][x+1] != "#":
            if([x+1, y] in visited):
                pass
            else:
                append([x+1, y])
                botPos = [x+1, y]
                clean += 1
            x += 1
        botPos = [x, y]
    elif(i == "<"):
        while room[y][x-1] != "#":
            if([x-1, y] in visited):
                pass
            else:
                append([x-1, y])
                botPos = [x-1, y]
                clean += 1
            x -= 1
        botPos = [x, y]
    elif(i== "^"):
        while room[y-1][x] != "#":
            if([x, y-1] in visited):
                pass
            else:
                append([x, y-1])
                botPos = [x, y-1]
                clean += 1
            y -= 1
        botPos = [x, y]
    elif(i == "v"):
        while room[y+1][x] != "#":
            if([x, y+1] in visited):
                pass
            else:
                append([x, y+1])
                botPos = [x, y+1]
                clean += 1
            y += 1
        botPos = [x, y]
print(clean)

What I'm doing is taking an input where the first line is r c k.
r is the max y value in the 2d array starting from 1, c is the max x value starting from 1 arr[y][x] and k is the amount of instructions.
the second is a line of instructions eg. <>^v for left, right, up and down.
In the function findStartPos I am iterating though the 2d array of strings I created from the rest of the input eg,
[[#, #, #, #], [#, O, #, ., #], [#, . ., ., #], [#, #, ., ., #], [#, #, #, #]]
to find the starting cordinates of O.
In the rest of the code I go though the instructions and moving the O around to count how many unique points it has been on. It can't be on a #. eg.
###### instuction: <
#....#
#.#.O#
###### instuction: ^
#....#
#.#O.#
###### instuction: <
#..O.#
#.#..#
###### instuction: None
#O...#
#.#..#
been on 5 unique "squares"

Samples:
5 5 4
v>^v
#####
#O#.#
#...#
##..#
#####
returns: 6

8 10 14
<v>^<v>v<^^><>
##########
#.#......#
#....#...#
##......O#
#........#
#..#.....#
#....#...#
##########
returns: 33

Hope the code is readable enough to be understood.

A loop that does different things based on how many times the loop has executed is a code smell.  It's harder to understand then breaking the loop into discrete steps.
And put it in a function.  Pass in the file, so you can use a file or io.StringIO instead of sys.stdin for testing.
def load_problem(file_input):
    r, c, k = map(int, file_input.readline().strip().split())

    instructions = file_input.readline().strip()

    room = [file_input.readline() for _ in range(r)]

    return room, instructions

Simplified find_start() a bit:
def find_start(room):
    for r,row in enumerate(room):
        c = row.find('O')
        if c >= 0:
            return r,c

Code to solve the problem can go in another function.
A set() is great for keeping track of places you've visited.
The duplication of code can be eliminated by using the instruction to select an x-step (dc) and y-step (dr) to be added to the current position.
STEP = {
    '>':( 0, 1),
    'v':( 1, 0),
    '<':( 0,-1),
    '^':(-1, 0)
}

def solve_problem(room, instructions):
    r, c = find_start(room)

    visited = set()
    visited.add((r, c))

    for instruction in instructions:
        dr, dc = STEP[instruction]

        while room[r + dr][c + dc] != '#':
            r += dr
            c += dc
            visited.add((r, c))

    return len(visited)

Drive the whole thing:
room, instructions = load_problem(sys.stdin)
result = solve_problem(room, instructions)
print(result)


Checking if input is a string or an integer I'm teaching myself C++ by reading Stroupstrup's Programming Principles and Practice Using C++, and I'm working on an exercise in chapter 4. The exercise asks to convert input as a digit to the corresponding spelled-out value e.g. input of 1 gives the output "one".

#include "std_lib_facilities.h"

using namespace std;

//Workaround for compiler issue.
int stoi( const std::string& str, std::size_t* pos = 0, int base = 10 )
    {
        const char* begin = str.c_str() ;
        char* end = nullptr ;
        long value = std::strtol( begin, &end, base ) ;

        if( errno == ERANGE || value > std::numeric_limits<int>::max() )
            throw std::out_of_range( "stoi: out ofrange" ) ;

        if( end == str.c_str() )
            throw std::invalid_argument( "stoi: invalid argument" ) ;

        if(pos) *pos = end - begin ;

        return value ;
    }

int main()
{
    struct Numbers
    {
        string spelled;
        int digit;
    };
    const Numbers nums[]
    {
        {"zero", 0},
        {"one", 1},
        {"two", 2},
        {"three", 3},
        {"four", 4},
        {"five", 5},
        {"six", 6},
        {"seven", 7},
        {"eight", 8},
        {"nine", 9},
        {"ten", 10}
    };

    string response = " ";

    cout << "Type a number either spelled out or as a digit.\n";

    cin >> response;
    if(response.size() == 1)
    {
        cout << nums[stoi(response)].spelled;
    }
    else if(response.size() > 1)
    {
        for(int i=0; i<=10; ++i)
        {
            if(response == nums[i].spelled)
            {
                cout << nums[i].digit << endl;
                break;
            }
            else;
        }
    }
}

Is there a better way to check the type of input the user gives other than checking the response size? Would it have been easier/more efficient if I had used a vector? In my for loop, does that break in the if statement break the loop?

This is now a conflict:

using namespace std;

//Workaround for compiler issue.
int stoi( const std::string& str, std::size_t* pos = 0, int base = 10 )

There is an stoi in the standard namespace. Because you used using this will be considered with your function. They both have the same parameter types so this should generate a compiler error.

The only reason it may not be doing so is that you are using an archaic version of the language.

You have to reset errno to a zero state before using a function that sets it (std::strtol does not change the value if there is no error it just sets the value if there is an error).

    if( errno == ERANGE || value > std::numeric_limits<int>::max() )
        throw std::out_of_range( "stoi: out ofrange" ) ;

This will fail if a previous library call set errno to ERANGE.

Don't like the extra indention of you r function:

int stoi( const std::string& str, std::size_t* pos = 0, int base = 10 )
    {
^^^^!

Prefer to use {} around block statements.

if( errno == ERANGE || value > std::numeric_limits<int>::max() )
            throw std::out_of_range( "stoi: out ofrange" ) ;

// Best to be safe
// So consistent use of '{}' is usually best.
if( errno == ERANGE || value > std::numeric_limits<int>::max() ) {
            throw std::out_of_range( "stoi: out ofrange" ) ;
}

Readability enhanced by using another line:

if(pos) *pos = end - begin ;

// Easier to read for your next maintainer.
if(pos) {
    *pos = end - begin ;
}


Title Case function in C I've written the following function in C which shall take as input a character array (decayed to a char * of course), and mutate the string into a title-cased string in the form "this is a string" -> "This Is A String".

void title_case(char str[])
{
    assert(str);
    size_t i;
    str[0] = toupper((int) str[0]);
    if(strlen(str) == 1) return;
    for(i = 1; i < strlen(str); ++i)
    {
        str[i] = tolower((int)str[i]);
        if(str[i -1] == ' ')
        {
            str[i] = toupper((int)str[i]);
        }
    }
    return;
}

Please provide any feedback you see fit. Is there a more concise way to do this? Is array bracket notation the most clear way?

Driver:

#include <stdio.h>
#include <string.h>
#include <assert.h>
#include <ctype.h>

void title_case(char str[]);
int main(void)
{
    char sentence[] = "this is a SeNTeNce.";
    title_case(sentence);
    printf("%s\n", sentence);
    return 0;
}

In the context of function parameter passing, char* and char[] are equivalent (because char[] decays to char*).

Anyway, I would rather use title_case(char* s) because it more clearly expresses the truth: what you really get within the function is a pointer to the first character of the array.
Accepting char* as string parameter implicitly suggests that the passed string will be '\0'-terminated. As you probably know, there are many known security issues with this.
If that's not the case, that means if you cannot guarantee the '\0'-ending always, consider passing the string length also, for ex.:
title_case(char* s, size_t len)
The cast from char to int (int)s[0] should not be needed, because conversion from smaller to the bigger integral type is always done implicitly and should trigger no warning on any compiler I know.

4.The strlen() in this loop for(i = 1; i < strlen(str); ++i) is called on every iteration, so the overall complexity will be quadratic. The common practice to avoid this is to store length into a variable and then use it, like:

size_t len = strlen(str);
for (i = 1; i < len; ++i) ...

The more concise way with linear complexity would be something like this

>

#include <stdio.h>
#include <string.h>
#include <ctype.h>

void title_case(char *s)
{
    if (s == NULL) return;

    for (char *p = s; *p != '\0'; ++p)
        *p = (p == s || *(p-1) == ' ') ? toupper(*p) : tolower(*p);
}

int main()
{
    char s[] = "this is some     title";
    title_case(s);
    printf("%s\n", s);
}

Note: In the boolean expression p == s || *(p-1) == ' ' short-circuit evaluation capability of C/C++ is used: if p==s is true, then *(p-1) is not evaluated. Otherwise we would dereference invalid pointer -> which would lead to undefined behaviour.

Edited: Emphasize quadratic complexity issue, as proposed in comments by @sudo


LeetCode: Increasing Order Search Tree C# https://leetcode.com/problems/increasing-order-search-tree/

Please review for performance

  Given a binary search tree, rearrange the tree in in-order so that the
  leftmost node in the tree is now the root of the tree, and every node
  has no left child and only 1 right child.

  Constraints:

  The number of nodes in the given tree will be between 1 and 100. Each
  node will have a unique integer value from 0 to 1000.

using System.Collections.Generic;
using GraphsQuestions;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace TreeQuestions
{
    /// <summary>
    /// https://leetcode.com/problems/increasing-order-search-tree/
    /// </summary>
    [TestClass]
    public class IncreasingBstTest
    {
        [TestMethod]
        public void ExampleTest()
        {

                //      5
                //     / \
                //    3   6
                //   / \   \
                //  2   4   8
                // /      / \
                // 1     7   9

            TreeNode root = new TreeNode(5);
            root.left = new TreeNode(3);
            root.left.left = new TreeNode(2);
            root.left.right = new TreeNode(4);
            root.left.left.left = new TreeNode(1);

            root.right = new TreeNode(6);
            root.right.right = new TreeNode(8);
            root.right.right.left = new TreeNode(7);
            root.right.right.right = new TreeNode(9);
            var forEach = new InOrderForEach();
            root = forEach.IncreasingBST(root);

            int res = 1;
            var curr = root;
            while (curr != null)
            {
                Assert.AreEqual(res, curr.val);
                curr = curr.right;
                res++;
            }
        }
    }

    }

    public class InOrderForEach
    {
        public TreeNode IncreasingBST(TreeNode root)
        {
            if (root == null)
            {
                return null;
            }
            List<int> vals = new List<int>();
            InOrder(root, vals);
            var ans = new TreeNode(0);
            TreeNode curr = ans;
            foreach (var v in vals)
            {
                curr.right = new TreeNode(v);
                curr = curr.right;
            }
            return ans.right;
        }

        private void InOrder(TreeNode root, List<int> vals)
        {
            if (root == null)
            {
                return;
            }
            InOrder(root.left, vals);
            vals.Add(root.val);
            InOrder(root.right, vals);
        }

    }
}

There isn't much to review. InOrder() is merely a depth-first-search, so maybe I would call it that.

You could though optimize a bit, if you created the new "tree" as you traverse the old one:

  public class InOrderForEach
  {
    TreeNode newRoot = new TreeNode(0);
    TreeNode current = null;

    public TreeNode IncreasingBST(TreeNode root)
    {
      if (root == null)
      {
        return null;
      }
      current = newRoot;
      InOrder(root);
      return newRoot.right;
    }

    private void InOrder(TreeNode root)
    {
      if (root == null)
      {
        return;
      }
      InOrder(root.left);
      current = current.right = new TreeNode(root.val);
      InOrder(root.right);
    }

  }


Validate iTunes links I'm working on this code and I can't find a better way to improve this code, can anyone improve it more? My PHP is pretty bad, this is the best I could do.

The code checks if iTunes link exists in the database if not search it on iTunes with API and if successfully found a link in itunes api it will be added in the database. If iTunes search is empty and iTunes row in the database is empty a string 'gol' will be saved in the MySQL and based on this string it will be rechecked in the future because Itunes has a 20 searches per minute limit.
Hope i explained what is the code supposed to do.

if (empty($video['itunes'])) {
    //if mysql row $video['itunes'] is empty check the function ituneslink
    $itunecheck = ituneslink($mystrings);
    //ituneslink is a function that searches the itunes library for Movies/Music
    if (empty($itunecheck)) {
    //if ituneslink function is empty trigger the empty string itunesgol and
        $itunesgol = '';
        $smarty->assign('itunesgol', $itunesgol);
        itunes_add_item("gol", $video['uniq_id']);
    //add in mysql 'gol' that the itunescheck didn't found nothing on itunes
    //search api, this will be useful for future rechecks
    }
} elseif($video['itunes'] != "gol") {
    // if row itunes is not 'gol' it means that itunescheck got the itunes link
    //and succesfully added in database
    $itunecheck = $video['itunes'];
} else {
    $itunesgol = '';
    $smarty->assign('itunesgol', $itunesgol);
}

if ($video['itunes'] == 'gol') {
    //because itunes seach api has a limit of 20 req per minute i had to recheck
    //for this string if available because maybe the first search got 403
    //fobiden because of to many queries
    $itunecheck = ituneslink($mystrings);
    itunes_add_item($itunecheck, $video['uniq_id']);
}

if (empty($video['itunes']) && !empty($itunecheck) && $video['itunes'] != "gol") {

    //NOW here i check if itunes is empty and itunescheck not empty and row
    //itunes not 'gol'. If all of them comply it will be added in the database
    itunes_add_item($itunecheck, $video['uniq_id']);
}

The code works well, what i want is: to improve it, to shorten the code, to make it smoother, faster, it looks too messy

if (empty($video['itunes']) && !empty($itunecheck) && $video['itunes'] != "gol") {

If $video['itunes'] is empty, then you already know that it does not equal "gol".  So you could just say

if (empty($video['itunes']) && !empty($itunecheck)) {

And you would get the same effect.

Similarly, we could rewrite the whole thing as

if (empty($video['itunes'])) {
    //if mysql row $video['itunes'] is empty check the function ituneslink
    $itunecheck = ituneslink($mystrings);
    //ituneslink is a function that searches the itunes library for Movies/Music
    if (empty($itunecheck)) {
        //if ituneslink function is empty trigger the empty string itunesgol and
        $itunesgol = '';
        $smarty->assign('itunesgol', $itunesgol);
        itunes_add_item('gol', $video['uniq_id']);
        //add in mysql 'gol' that the itunescheck didn't find anything on itunes
        //search api, this will be useful for future rechecks
    } else {
        //NOW here I check if itunes is empty and itunescheck not empty and row
        //itunes not 'gol'. If all of them comply it will be added in the database
        itunes_add_item($itunecheck, $video['uniq_id']);
    }
} elseif($video['itunes'] == 'gol') {
    $itunesgol = '';
    $smarty->assign('itunesgol', $itunesgol);
    //because itunes search api has a limit of 20 req per minute I had to recheck
    //for this string if available because maybe the first search got 403
    //forbidden because of too many queries
    $itunecheck = ituneslink($mystrings);
    itunes_add_item($itunecheck, $video['uniq_id']);
} else {
    // if row itunes is not 'gol' it means that itunescheck got the itunes link
    //and successfully added in database
    $itunecheck = $video['itunes'];
}

This gets rid of the redundant if statements, adding to the original blocks instead.

Or

if (empty($video['itunes']) || ($video['itunes'] == 'gol')) {
    // if no cached result, or cached as `gol`
    // fetch new results from iTunes
    $itunecheck = ituneslink($mystrings);
    if (empty($itunecheck)) {
        $itunesgol = '';
        $smarty->assign('itunesgol', $itunesgol);
        itunes_add_item('gol', $video['uniq_id']);
    } else {
        itunes_add_item($itunecheck, $video['uniq_id']);
    }
} else {
    // otherwise use cached result
    $itunecheck = $video['itunes'];
}

This changes the behavior slightly.  It doesn't assume that a second iTunes search will always return results.  Instead, it treats a cached failure the same as no result in cache.

But if we do that, we can go ahead and just say

if (empty($video['itunes'])) {
    // if no cached result
    // fetch new results from iTunes
    $itunecheck = ituneslink($mystrings);
    if (empty($itunecheck)) {
        $itunesgol = '';
        $smarty->assign('itunesgol', $itunesgol);
    } else {
        itunes_add_item($itunecheck, $video['uniq_id']);
    }
} else {
    // otherwise use cached result
    $itunecheck = $video['itunes'];
}

This removes the concept of caching failures.  Instead, we'll just generate a cache miss next time.

You could also go the other direction and add more complexity such that you sometimes redo a 'gol' and sometimes don't.  Or whatever.  But as things are written, there doesn't seem to be any reason to have two cases with mostly the same behavior.

It is of course possible that I'm missing something.  You don't include definitions for ituneslink, itunes_add_item, and $smarty->assign, so there could be special stuff happening there.  Or prior to this code in setting $video['itunes'].  Or after this code that uses $itunesgol or $itunecheck.  But from what I see of this code, this seems to do mostly the same thing as the original.


Eager Loading Deeply into a Model with a Collection Property Whose Type is Inherited Visual Studio generated this great route for me, where I can load an entity:

// GET: api/Jobs/5
[ResponseType(typeof(Job))]
public async Task<IHttpActionResult> GetJob(int id)
{
    Job job = await db.Jobs.FindAsync(id);

    if (job == null)
    {
        return NotFound();
    }

    return Ok(job);
}

Here's the Job model this is based on:

public class Job
{
    public Job()
    {
        this.Regions = new List<Region>();
        this.Files = new List<JobFile>();
    }

    public int ID { get; set; }
    public string Name { get; set; }
    public List<Region> Regions { get; set; }
    public JobTypes JobType { get; set; }
    public int UserIDCreatedBy { get; set; }
    public int? UserIDAssignedTo { get; set; }
    public List<JobFile> Files { get; set; }
    public bool IsLocked { get; set; } // Lock for modification access
}

Here's the JobFile class, which Jobs have a list of:

public class JobFile
{
    public int ID { get; set; }
    public string Name { get; set; }
    public string Url { get; set; }
    public int Job_ID { get; set; }
}

and Pdf, a subclass of JobFile:

public class Pdf : JobFile
{
    public Pdf()
    {
        this.PdfPages = new List<PdfPage>();
    }

    public int Index { get; set; }

    public List<PdfPage> PdfPages { get; set; }
}

Now, when I hit that route, I'd like to eagerly load all the Pdfs for a Job, including their pages. I modified the route to look like this, and it works.

// GET: api/Jobs/5
[ResponseType(typeof(Job))]
public async Task<IHttpActionResult> GetJob(int id)
{
    Job job = await db.Jobs.FindAsync(id);

    // Lookup the PDFs for this job and include their PdfPages
    List<JobFile> jobPdfs = db.Pdfs.Include(pdf => pdf.PdfPages).Where(pdf => pdf.Job_ID == id).ToList<JobFile>();

    // Attach the job files to the job
    job.Files = jobPdfs;

    if (job == null)
    {
        return NotFound();
    }

    return Ok(job);
}

Is this the best way to eagerly load all these models? Could this somehow be collapsed into one statement? It seems right now it hits the database twice. Could I build off of the original

Job job = await db.Jobs.FindAsync(id);

to load the Pdfs and their PdfPages all in one query?

This question provided some helpful insight, but I'm not sure how I can capitalize on its conclusions. I think I need the ToList<JobFile>() (which according to the question does a trip to the database) because I actually need that data. So unless I can squash it into one more complicated Linq statement, perhaps it's unavoidable to make two trips.

The problem here is that you'd actually want to include subtypes. If Job had a collection of Pdfs, you could have done

Job job = await db.Jobs
                  .Include(j => j.Pdfs.Select(pdf => pdf.PdfPages))
                  .SingleOrDefaultAsync(j => j.Id == id);

But Pdf is a subtype, and EF doesn't support a syntax like

Job job = await db.Jobs
                  .Include(j => j.Files.OfType<Pdf>().Select(pdf => pdf.PdfPages))
                  .SingleOrDefaultAsync(j => j.Id == id);

So what you do is the only way to get the Job with Pdfs and PdfPages.

There are some improvements to be made though:

You can just load the child objects into the context without assigning them to job.Files yourself. EF will knit the entities together by relationship fixup.
You can first check if the Job is found and then load the Pdfs.

Turning it into this:

Job job = await db.Jobs.FindAsync(id);
if (job == null)
{
    return NotFound();
}
else
{
    // Load the PDFs for this job and include their PdfPages
    await db.Pdfs.Include(pdf => pdf.PdfPages).Where(pdf => pdf.Job_ID == id)
        .LoadAsync();
}

return Ok(job);


Find subsets of size K in N set The question is: There is a set An, and it consists of integers from 1 to n.

An={1, 2, 3, ..., n}

Print all subsets of size K in An. It must be generated in order like the example below.

So for example, n=5 k=3

{1, 2, 3} {1, 2, 4} {1, 2, 5} {1, 3, 4} {1, 3, 5} {1, 4, 5} {2, 3, 4} ... {3, 4, 5}

I am not sure about if there is another way that doesn't use recursion. I did this with recursion but the problem is that all test cases should be done within 1 sec. When N and K are like 5, 3 and 12, 6 it is okay but when it comes to like 50, 48 or 100, 95, it takes too long. I am having a real struggle with this problem.

#include <stdio.h>
#include <stdlib.h>
#include <time.h>

void subset(int n, int k, int arr[], int pos[], int index, int start){
    int i, j;

    if(index == k){
        for(j=0; j<k; j++){
            if(j==k-1)
                printf("%d\n", pos[j]);
            else{
                printf("%d ", pos[j]);
            }
        }
        return;
    }

    for(i=start; i<n; i++){
        pos[index] = arr[i];
        subset(n, k, arr, pos, index+1, i+1);
    }
}

int main(){
    int n, k, arr[100], index=0, start=0;

    scanf("%d %d", &n, &k);

    // 1<=n<=100 ; 1<=k<=n
    if(n>100||n<1 && k>n||k<1)
        return 0;

    int i;
    for(i=0; i<n; i++)
        arr[i]=i+1;

    int *pos = (int*)malloc(sizeof(int)*k);

    time_t clockstart=0, clockend=0;
    float gap;
    clockstart = clock();
    subset(n, k, arr, pos, index, start);
    clockend = clock();
    gap = (float)(clockend-clockstart)/(CLOCKS_PER_SEC);

    printf("%f\n", gap);

    return 0;
}

I think i should use something like tail recursion or vector in C++ but I can't figure out how to work with those.

As you probably noticed, the slowdown happens when n is large and k is close to n. If it is slower than it should be, it is likely because it is doing unnecessary work. Lets have a look at the case where n=5 and k=3, and go to the point in the execution of your program where arr[0] = 1 and index = 1. In the for-loop in subset(), you are looking at all possible values from start = 2 to n = 5. However, not all values will actually result in valid solutions. Here is a list of all {1, i} you check, and which subsets are possible:

{1, 2} -> {1, 2, 3} {1, 2, 4} {1, 2, 5}
{1, 3} -> {1, 3, 4} {1, 3, 5}
{1, 4} -> {1, 4, 5}
{1, 5} -> no solutions

As you can see, {1, 5} does not have any possible subsets. Or put otherwise, 5 is too large a number at pos[1] to give any solutions.

Instead of trying all numbers from start to n, try to find out what the actual maximum is that will still give solutions.


Setting global environment vars from Python or Bash #!/usr/bin/env python

from os import environ, path, name as os_name, getuid
from sys import exit
from fileinput import input

update_environ = lambda: environ.update(dict(env.split('=')
                                        for env in
                                        open('/etc/environment', 'r').readlines()))

if os_name != 'nt' and getuid() != 0:
    print "Rerun {0} as `sudo`".format(__file__)
    exit(1)

def custom_vars(e_vars):
    if not path.isfile('custom_vars.txt'):
        return e_vars
    with open('custom_vars.txt', 'r') as f:
        for line in f.readline():
            var = line.split()
            e_vars[var[0]] = var[1]

def append_environment(e_vars):
    for line in input('/etc/environment', inplace=True):
        var, _, val = line.partition('=')
        nline = '{var}={val}'.format(var=var, val=e_vars.pop(var, val))
        if nline and nline != '=' and nline != '\n' and nline != '=\n':
            print nline  # inplace editing

    if e_vars:
        lines = filter(lambda l: l != '=\n' and nline != '\n',
                       ['{var}={val}\n'.format(var=k, val=e_vars[k])
                        for k in e_vars])
        with open('/etc/environment', 'a') as f:
            f.writelines(lines)

if __name__ == '__main__':
    environment_vars = {'NOM_REPOS': path.join(path.expanduser("~"), 'NOM'),
                        'NOM_API': path.join('/var', 'www')}

    environment_vars = custom_vars(environment_vars)
    append_environment(environment_vars)

I have written a similar script in Bash; with identical functionality:

#!/usr/bin/env bash

sudo sed -i "/^NOM*/d" /etc/environment
for i in `env | grep NOM | cut -f1 -d=`; do
    unset "$i";
done

function set_var()
{
    VAR="$1"
    VAL="$2"

    sudo sed -i "/^$VAR/d" /etc/environment
    printf "$VAR=$VAL\n" | sudo tee -a /etc/environment > /dev/null

    # make the variable available for use in this script and custom_env_vars.sh
    export "$VAR=$VAL"
}

set_var NOM_REPOS "$HOME/NOM"
set_var NOM_API /var/www

if [ -f custom_env_vars.sh ]; then
    . ./custom_env_vars.sh
else
    echo "You can set custom environment variables in 'custom_env_vars.sh'"
fi

env | grep '^NOM'  would more exactly align with the sed command.

ensure you're exactly matching the variable name:

sudo sed -i "/^$VAR=/d" /etc/environment
# .................^

Use %-formatters for printf:

printf '%s="%s"\n" "$VAR" "$VAL"

In the function, use local to limit the scope of VAR and VAL

had another thought about that function. Change

sudo sed -i "/^$VAR/d" /etc/environment
printf "$VAR=$VAL\n" | sudo tee -a /etc/environment > /dev/null

to

tmpfile=$(mktemp)
grep -v "^$VAR=" /etc/environment > $tmpfile
echo "$VAR=$VAL" >> $tmpfile
sudo mv $tmpfile /etc/environment

That minimized any possibility that the /etc/environment file contains "invalid" data -- it overwrites the previous good file with the new good file all at once.


Checking for a condition, even if it's always true In my program there is the following piece of code where I must delete a ' ' from the second to last index in a StringBuilder:

builder.deleteCharAt(builder.length() - 2);

I do not surround it with checks to either condition builder.length() >= 2 or builder.charAt(builder.length() - 2) == ' '), because I am 100% sure that both these conditions will always be true, even though at first glance, it is not so obvious.

I'm torn between feeling that just because I have a higher understanding of the code, doesn't mean I should neglect the conditions for the sake of others in the team. But at the same time, if I do check for these conditions, it would be absolutely unnecessary.

My solution is to simply add a comment, but I was wondering if is this considered to be good practice?

builder.deleteCharAt(builder.length() - 2); // should always be ' '

Don't use a comment. Use code. Specifically, assert builder.length() >= 2 and assert builder.charAt(builder.length() -2) == ' '. Assertions will clearly indicate the preconditions, and they can be excluded by the compiler or jvm so they don't impact the deployed code.


Extracting values from dictionaries where the keys match I have two Dictionaries with around 65,000 KeyValuePairs each. I used foreach and if-else statements to compare them and get values, but it goes very slow. How could I optimize my code and gain more speed?

private void bgwCompare_DoWork(object sender, DoWorkEventArgs e)
{
    var i = 0;

    foreach (KeyValuePair<string, string> line1 in FirstDictionary)
    {
        foreach (KeyValuePair<string, string> line2 in SecondDictionary)
        {
            if (line1.Key == line2.Key)
            {
                ResultDictionary.TryAdd(line1.Value, line2.Value);

                ListViewItem item = new ListViewItem(line1.Value);
                item.SubItems.Add(line2.Value);

                ResultList.Items.Add(item);
            }

            i++;
            bgwCompare.ReportProgress(i * 100 / (FirstDictionary.Count() * SecondDictionary.Count()));
        }
    }
}

If I'm understanding it correctly, you're populating a ConcurrentDictionary from the values of two other ConcurrentDictionaries, where the keys are equal.

Try this, it's vastly faster than your loop in my tests.

var matches = FirstDictionary.Keys.Intersect(SecondDictionary.Keys);
foreach (var m in matches)
    ResultDictionary.TryAdd(FirstDictionary[m], SecondDictionary[m]);


Add element alternatively to a list I want to place an element alternatively between any two elements of a list.

l1 = list('abcde')
fill = 'z'

How it should look like:

['a', 'z', 'b', 'z', 'c', 'z', 'd', 'z', 'e']

My solution:

fill = 'z'
l2 = [fill]*(len(l1)-1)
[x for t in map(None, l1, l2) for x in t if x]

Is there a better approach, i.e. that doesn't need to create another list?

What if fill were a comma?  Does that remind you of a common string-handling idiom?

>>> ','.join('abcde')
'a,b,c,d,e'

The only remaining task is to convert that result into a list.

Therefore, the solution would be better written as:

list(fill.join(l1))


Calculating the total daily amount of confirmed cases of Coronavirus I'm writing a small program to plot new COVID-19 infections. As of right now, I have it so the program reads the given data file, pulls out the daily cases and dates for each country, and adds together all the cases for a given date. However, because both of the lists generated have lengths of over 2000, it currently runs extremely slowly. Is there any change I can make to improve the speed of my program?
import pylab as pl

cases = pd.read_csv("daily-cases-covid-19.csv")
dc = cases.loc[:,'Daily confirmed cases (cases)']
dd = cases.loc[:,'Date']

worldCases = []

for i in range(0,len(dd)):
    count = 0
    for j in range(0,len(dd)):
        if dd[j]==dd[i]:
            count+=dc[i]
    worldCases.append(count)

Here is an example of the CSV I am reading through. The purpose of the nested loops is to add together all of the confirmed cases in each country on a given date.
Afghanistan,AFG,"Jan 1, 2020",0
Afghanistan,AFG,"Jan 2, 2020",0
Afghanistan,AFG,"Jan 3, 2020",0
Afghanistan,AFG,"Jan 4, 2020",0
Afghanistan,AFG,"Jan 5, 2020",0
Afghanistan,AFG,"Jan 6, 2020",0
Afghanistan,AFG,"Jan 7, 2020",0
Afghanistan,AFG,"Jan 8, 2020",0
Afghanistan,AFG,"Jan 9, 2020",0
Afghanistan,AFG,"Jan 10, 2020",0
Afghanistan,AFG,"Jan 11, 2020",0
Afghanistan,AFG,"Jan 12, 2020",0
Afghanistan,AFG,"Jan 13, 2020",0
Afghanistan,AFG,"Jan 14, 2020",0
Afghanistan,AFG,"Jan 15, 2020",0
Afghanistan,AFG,"Jan 16, 2020",0
Afghanistan,AFG,"Jan 17, 2020",0
Afghanistan,AFG,"Jan 18, 2020",0
Afghanistan,AFG,"Jan 19, 2020",0

So it seams like you have a Pandas dataframe you are working with.
At the moment it looks like this section of code does the following

worldCases = []

for i in range(0,len(dd)):               #iterate through all dates
    count = 0                            #setup a counter
    for j in range(0,len(dd)):           #iterate through all dates
        if dd[j]==dd[i]:
            count+=dc[i]                 #add one to counter if inner date == outer date
    worldCases.append(count)             #track number of times a unique date occurs

You are effectively binning your data, getting a count of the number of times each unique date occurs. Pandas offers you more efficient and convenient tools for doing this. Specifically look into the groupby method.

A much faster way to get the same output for worldCases would be to do the following:

# group the daily cases data by the date and then compute
# the sum of cases within each date group

dc = 'Daily confirmed cases (cases)'
worldCases = cases.loc[:, dc].groupby(cases['Date']).sum()

Also, if you find yourself writing lots of loops like the above, you may want to check out the user guide for "group by" operations in the pandas documentation. It is a very powerful way of handling situations like this, and more, but can take a bit of getting used to. It can feel like a whole different way of thinking about your data.


Encoding 0/1 bits to 0/255 bytes in C++ I need to map a sequence of bits that represents a float into a sequence of bytes by this rule: the value 1 maps to the value 255 and the value 0 maps to the value 0.

This is the code I have now:

int FloatToBit(int *buffer, int startIndex, float value) {
  int fl = *(int*)&value;
  int i = 0;
  for (; i < sizeof(float) * 8; ++i) {
    buffer[startIndex + i] = ((1 << i) & fl) != 0 ? 255 : 0;
  }
  return i;
}

The code above is working, still I need something faster. Is it possible to improve its performance?

Edit 1:
I have created an online benchmark for this function.

Edit 2:
Using buffer[startIndex+i] = (unsigned char)-((fl>>i)&1) as @matt-timmermans suggested in the original question. Looks like there is no evident performance improvement.
Online benchmark.

Edit 3:
This benchmark shows that the approach suggested by @greybeard is ~29 times faster than the original implementation.

Edit 4:
Created a benchmark that measures all the suggested approaches.

Writing memory is s.l.o.w. - especially if writing less than the processor word/bus width.
Loop jamming has a chance to speed up things - from a distance, this is just a source code variant of the Streaming SIMD code hoffmale linked.

Instead of table lookup, one can turn LeastSignificantFirst bits in word to MSF bytes in u32 using bit manipulation - I failed to produce a readable variation taking full advantage of CHAR_BIT & co.:

leastBits = ((((1<<CHAR_BIT)+1 << CHAR_BIT)+1 << CHAR_BIT)+1);
bytes = (bits & 0xf) * ((((8<<CHAR_BIT)+4 << CHAR_BIT)+2 << CHAR_BIT)+1);
bytes = (bytes >> 3) & leastBits;
bytes = (bytes<<CHAR_BIT) - bytes;

should work like an evil spell, even extended to eight bytes.
The problem with LSF bits to MSF bytes and no less bits to convert than bits/byte is part products running into each other - use

bytes = (bits & 0xf | ((bits & 0xf0)<<32))*0x204081;

(and no >> 3(7)))

Warning tried out, but not even tested systematically.

const int NBFL = sizeof(float) * CHAR_BIT;

static void BitsToBytesImproved(benchmark::State& state) {
  // Code not measured
  float value = 1000.1234567;  // Params initialization
  unsigned char * buffer0 = new unsigned char[1024];
# define BASE_TYPE long
# define UNSIGNED_TYPE unsigned BASE_TYPE
# define UNSIGNED_SIZE sizeof(UNSIGNED_TYPE)
# define UNSIGNED_BIT  (1 << UNSIGNED_SIZE)
  UNSIGNED_TYPE bits = 0xff,
  *buffer = (UNSIGNED_TYPE *)buffer0,
  patterns[UNSIGNED_BIT] = { 0 };
 // set up pattern look-up table
  for (int done = 1 ; done < UNSIGNED_BIT ; bits <<= CHAR_BIT)
    for (int i = 0, next = 2*done ; done < next ; )
      patterns[done++] = patterns[i++] | bits;

  for (auto _ : state) { // this loop is measured repeatedly
    value += .1;            // to prevent static evaluation
    int fl = *(int*)&value;
    for (int i = 0 ; i < NBFL ; i += UNSIGNED_SIZE) {
      UNSIGNED_TYPE *pui = (UNSIGNED_TYPE *)(buffer0 + i);
      bits = patterns[fl & UNSIGNED_BIT-1];
      benchmark::DoNotOptimize(*pui = bits);
      fl >>= UNSIGNED_SIZE;
    }
  }
}

(Benchmark results have been too ticklish to the way I put the code for me to put much trust in them. FWIW, table look-up was reported faster than bit-bashing.)


FizzBuzz in the MIPS ISA The title pretty much explains it. This is the first program I've written in assembly, so any criticism, either on programming style or saving instructions, would be appreciated.

li $t0, -1
  .data
nl: .asciiz  "\n"
  .text
loop:beq $t0, 100, exit
 add $t0, $t0, 1
 add $a0, $t0, $zero
 li $v0, 1
 syscall
 rem $t1, $t0, 3
 rem $t2, $t0, 5
 or $t3, $t1, $t2
 beqz $t3, b1
 beqz $t1, b2
 beqz $t2, b3

 la $a0, nl
 li   $v0, 4
 syscall

 j loop

b1:  li $a0, 3
 jal printb
 j loop
b2:  li $a0, 1
 jal printb
 j loop
b3:  li $a0, 2
 jal printb
 j loop

exit:li $v0, 10
 syscall

  .data
fizz: .asciiz  " Fizz\n"
buzz: .asciiz  " Buzz\n"
fizzbuzz: .asciiz  " Fizz Buzz\n"
  .text
printb:
  beq $a0, 1, l1
  beq $a0, 2, l2
  beq $a0, 3, l3
  l1: la $a0, fizz
  j print
  l2: la $a0, buzz
  j print
  l3: la $a0, fizzbuzz
  j print
print:li   $v0, 4
  syscall
  jr $ra

Given concern for performance, the obvious change would be to allocate a buffer large enough for all the output data. As you compute each result, just write the results to the output buffer. Then, when you've accumulated it all in the buffer, write the entire buffer to the output in one big chunk.


A free-standing weak_ptr equivalent to handle lifetime of non-shared callback objects I've been finding myself wanting the following pattern recently:

void perform_async_operation(std::function<void(int)>);

class some_class {
  public:
    void foo() {
      perform_async_operation([this](int result) {
        data_ = result;
      });
    }

  private:
    int data_;
};

Now, the issue here is that the lifetime of some_class if fully disconnected from when the callback will be invoked, and I do not want to move to shared ownership to put a shared_ptr in the lambda (like how the boost::ASIO examples handle that situation).

I do, however, have a saving grace: I know for certain that the callback will be invoked in the same thread that manages the lifetime of some_class.

In order to solve this, I've created a bastardized version of weak_ptr (with a matching equivalent of enabled_shared_from_this) that does not require the object to be managed by a shared_ptr in the first place.

#ifndef SLT_WEAK_REF_INCLUDED_H
#define SLT_WEAK_REF_INCLUDED_H

#include <memory>

template <typename T>
class weak_ref {
 public:
  weak_ref(std::shared_ptr<T*> weak_data) : weak_data_(std::move(weak_data)) {}

  operator bool() const { return *weak_data_ != nullptr; }
  T* operator->() const { return *weak_data_; }

 private:
  // Can be safely assumed to always be set.
  std::shared_ptr<T*> weak_data_;
};

template <typename CRTP>
class enable_weak_ref {
 public:
  enable_weak_ref() = default;
  enable_weak_ref(enable_weak_ref&& rhs)
      : weak_data_(std::move(rhs.weak_data_)) {
    if (weak_data_) {
      *weak_data_ = this;
    }
  }

  enable_weak_ref& operator=(enable_weak_ref&& rhs) {
    weak_data_ = std::move(rhs.weak_data_);
    if (weak_data_) {
      *weak_data_ = static_cast<CRTP*>(this);
    }
  }

  // explicitely do nothing
  enable_weak_ref(enable_weak_ref const& rhs) {}
  enable_weak_ref& operator=(enable_weak_ref const&) {}

  ~enable_weak_ref() {
    if (weak_data_) {
      *weak_data_ = nullptr;
    }
  }

  weak_ref<CRTP> get_weak_ref() {
    if (!weak_data_) {
      weak_data_ = std::make_shared<CRTP*>(static_cast<CRTP*>(this));
    }

    return weak_ref<CRTP>(weak_data_);
  }

 private:
  // TODO: perhaps switch to a simple manually ref-counted type here.
  // instead of the overkill shared_ptr<>
  std::shared_ptr<CRTP*> weak_data_;
};

#endif

sample usage:

#include "weak_ref.h"

#include <queue>
#include <iostream>
#include <functional>

std::queue<std::function<void(int)>> queue_;

void queue_delayed(std::function<void(int)> cb) {
  queue_.emplace(std::move(cb));
}

class my_class : public enable_weak_ref<my_class> {
public:
  my_class() {
    queue_delayed([ref = get_weak_ref()](int v){
      if(ref) {
        std::cout << "passed " << v << std::endl;
        ref->value = v;
      }
      else {
        std::cout << "saved " << v << std::endl;
      }
    });
  }

  int value = 0;
};

int main() {
  my_class a;
  queue_.front()(12);
  queue_.pop();
  {
    my_class b;
  }
  queue_.front()(5);
  queue_.pop();

  return 0;
}

Notes:

Moving an object in a lambda is a C++14 feature, without which, a lot of back-and-forth ref-counting happens.
As mentioned, this is intentionally not thread-safe. Thread safety can be added if anyone needs it eventually.

Issues:

Is there a nicer way to tackle this I'm overlooking here?
weak_ref is very weak name, I'd gladly welcome better suggestions.
As always, comments on general code quality are most desired.

I never saw a similar pattern so it was really interesting to understand your code !

First, about the name weak_ref, it implies that it has the syntax of a reference, but it is not the case (for instance, operator-> is for pointers). I personnaly named it something like ptr_to_instance.

Then, it seems the weak_ref should not be used on its own, without calling enable_weak_ref::get_weak_ref(). So I will make the weak_ref constructor private, making enable_weak_ref a friend. I will put the weak_ref class inside enable_weak_ref :

template<typename CRTP>
class enable_weak_ref {
  public:
    class weak_ref { /**/ };
    /**/
};

So instead of weak_ref<T> we have enable_weak_ref<T>::weak_ref. I think it makes more understandable the use of these classes.

Also, because weak_ref should not modify the pointer in weak_data_, I would use std::shared_ptr<T* const> so it is clear that this pointer is for read-only use.

If you do not want to use std::shared_ptr, you should create a int* (counting references), a CRTP** (which will hold this or nullptr) in enable_weak_ref, and weak_ref should have a int* const and a CRTP const* const (const specifier shows that weak_ref is linked to only one enable_weak_ref and can not be changed).