Alien Numbers - how Scala-ish is my solution? I'm trying to solve an old GCJ. It's a very simple puzzle, but I'm trying to sharpen my Scala-fu.

Basically, you're getting a list of triple number srcLanguage dstLanguage, where number is an integer given in the numeral system of srcLanguage. You should translate it to the numeral system of dstLanguage.

A numeral system is simply a string of all possible digits, in ascending order. The decimal numeral system is represented by 0123456789, the binary numeral system is 01, and the hexadecimal one 0123456789ABCDEF.

For example:

3 0123456789 01 -> 11
3 0123       AB -> BB

Here's how I implemented it in Scala:

case class Langs(num:String,srcLang:String,dstLang:String)
object Langs {def fromLine(line:String):Langs = {
    val ar = line.split(" ");return Langs(ar(0),ar(1),ar(2))}}
object Translate {
  def lang2int(lang:String,num:String):Long = {
    var b = BigDecimal(0)
    val dmap = (lang.toList.zipWithIndex).toMap
    val digitsList = num map dmap
    val valueList = digitsList.reverse.zipWithIndex map (
        x => x._1 -> math.pow(dmap.size,x._2))
    return valueList.map(x=>x._1*x._2).sum.toLong
  }
  def int2lang(lang:String,_num:Long):String = {
    var num = _num
    val dmap = (lang zip (0.toLong to lang.size)).map(_.swap).toMap
    val sb = StringBuilder.newBuilder
    while (num > 0) {
        sb.append(dmap(num % dmap.size))
        num = num/dmap.size
    }
    sb.reverse.toString
  }
  def lang2lang(l:Langs):String = int2lang(l.dstLang,lang2int(l.srcLang,l.num))
}

object mymain {
  def main(args : Array[String]) : Unit = {
    val s = "A-large-practice"
    val basef = new java.io.FileInputStream("~/Downloads/"+s+".in")
    val f = new java.util.Scanner(basef)
    val out = new java.io.FileWriter(s+".out")
    val n = f.nextInt
    f.nextLine
    for (i <- 1 to n) {
      val nl = f.nextLine
      val l = Langs.fromLine(nl)
      out.write("Case #"+i+": "+Translate.lang2lang(l)+"\n")
    }
    out.close
  }
}

You should definitely use scala.io.Source for File-IO
I wouldn't consider String splitting a responsibility of a general-purpose class. This should be done in the main loop
For tuples you can write map{ case (one,two) => ... }, which is often clearer than using x._1 and x._2
You don't need to write return if it's the last statement of the block
You can use pattern matching when defining vals: val Array(x, y, z) = line.split(" ")

Here is my attempt:

case class Langs(num:String, srcLang:String, dstLang:String)

object Langs {
  def fromLine(line:String):Langs = {
    val Array(num, srcLang, dstLang) = line.split(" ")
    Langs(num, srcLang, dstLang)
  }
}

object Translate {
  def lang2int(lang:String,num:String):Long = {
    val dmap = lang.toList.zipWithIndex.toMap
    val digitsList = num map dmap
    val valueList = digitsList.reverse.zipWithIndex map {
      case (one, two) => one -> math.pow(dmap.size, two)}
    valueList.map{case (one,two) => one*two}.sum.toLong
  }

  def int2lang(lang:String, num:Long):String = {
    val dmap = (0.toLong to lang.size zip lang).toMap
    Iterator.iterate(num)( _/dmap.size).takeWhile(_ > 0).map(n =>
      dmap(n % dmap.size)).mkString.reverse
  }

  def lang2lang(l:Langs):String = int2lang(l.dstLang,lang2int(l.srcLang,l.num))
}

Eliminating the while loop isn't that straight-forward, maybe someone else has an idea how to avoid that Iterator train-wreck.

[Edit]

I asked in another forum for a better solution for int2lang, and got this answer:

def int2lang(lang: String, num: Long): String = {
  val dmap = (0L to lang.size) zip lang toMap
  val size = dmap.size
  def loop(num: Long, l: List[Char]): List[Char] =
    if (num == 0) l else loop(num/size, dmap(num%size) :: l)
  loop(num, Nil).mkString
}

The nice thing about this is that the reverse is gone.


LINQ method to create objects of different types I have the following code inside a method:

var addresses = new List<MutateAddressData>();

var yearsDB = (from tr in this.uow.TaxReturns
                where taxReturnIds.Contains(tr.Id)
                select new { Year = tr.TaxYear, TaxReturnTypeId = tr.TaxReturnType.Id }).ToList();

var ibAndVpb = yearsDB.Where(y => y.TaxReturnTypeId == (decimal)TaxReturnType.IB ||
                                  y.TaxReturnTypeId == (decimal)TaxReturnType.VpB);
if (ibAndVpb.Any())
{
    var address = new MutateAddressData
                    {
                    AddressType = AddressType.TaxReturn,
                    CityId = customer.PaCity.Id,
                    CountryId = customer.PaCity.Country.Id,
                    HouseNumber = customer.PaHouseNumber,
                    HouseNumberPostfix = customer.PaHouseNumberPostfix,
                    Street = customer.PaStreet,
                    ZipCode = customer.PaZipCode,
                    Years = string.Join(", ", ibAndVpb.Select(t => t.Year.ToString())),
                    TaxReturnType = (TaxReturnType)ibAndVpb.First().TaxReturnTypeId,
                    };

    addresses.Add(address);
}

var averaging = yearsDB.Except(ibAndVpb);
if (averaging.Any())
{
    var address = new MutateAddressData
                    {
                    AddressType = AddressType.TaxReturn,
                    CityId = customer.PaCity.Id,
                    CountryId = customer.PaCity.Country.Id,
                    HouseNumber = customer.PaHouseNumber,
                    HouseNumberPostfix = customer.PaHouseNumberPostfix,
                    Street = customer.PaStreet,
                    ZipCode = customer.PaZipCode,
                    Years = string.Join(", ", averaging.Select(t => t.Year.ToString())),
                    TaxReturnType = (TaxReturnType)averaging.First().TaxReturnTypeId,
                    };

    addresses.Add(address);
}

return addresses.ToArray();

I want to create max two different objects here. One object for types that er Ib or Vpb and the other object is for all other types.

My senses tell me that I can achieve the same more easily, but I can't figure out how. What do you think?

Considering that most of the options of MutateAddressData seem to be identical for both cases, I'd extract that into a method. But then you'd need yearsDB to contain items of the type TaxReturn.

So you'd basically end up with something like this:

var yearsDB = this.uow.TaxReturns.Where(x => taxReturnIds.Contains(x.Id)).ToList();

var ibAndVpb = yearsDB.Where(x => (TaxReturnType)x.TaxReturnType.Id == TaxReturnType.IB ||
                                  (TaxReturnType)x.TaxReturnType.Id == TaxReturnType.VpB);
if (ibAndVpb.Any())
{
    addresses.Add(CreateMutateAddressData( customer,  ibAndVpb));
}

var averaging = yearsDB.Except(ibAndVpb);
if (averaging.Any())
{
    addresses.Add(CreateMutateAddressData( customer,  averaging));
}

return addresses.ToArray();

This is the method that's called:

private MutateAddressData CreateMutateAddressData(Customer customer, List<TaxReturn> taxReturns)
{
    return new MutateAddressData
                    {
                    AddressType = AddressType.TaxReturn,
                    CityId = customer.PaCity.Id,
                    CountryId = customer.PaCity.Country.Id,
                    HouseNumber = customer.PaHouseNumber,
                    HouseNumberPostfix = customer.PaHouseNumberPostfix,
                    Street = customer.PaStreet,
                    ZipCode = customer.PaZipCode,
                    Years = string.Join(", ", taxReturns.Select(t => t.Year.ToString())),
                    TaxReturnType = (TaxReturnType)taxReturns.First().TaxReturnType.Id,
                    };
}


Project Euler #23 with Java8 Streams A perfect number is a number for which the sum of its proper divisors is exactly equal to the number. For example, the sum of the proper divisors of 28 would be 1 + 2 + 4 + 7 + 14 = 28, which means that 28 is a perfect number.

  A number n is called deficient if the sum of its proper divisors is less than n and it is called abundant if this sum exceeds n.

  As 12 is the smallest abundant number, 1 + 2 + 3 + 4 + 6 = 16, the smallest number that can be written as the sum of two abundant numbers is 24. By mathematical analysis, it can be shown that all integers greater than 28123 can be written as the sum of two abundant numbers. However, this upper limit cannot be reduced any further by analysis even though it is known that the greatest number that cannot be expressed as the sum of two abundant numbers is less than this limit.

  Find the sum of all the positive integers which cannot be written as the sum of two abundant numbers.

I used this as an excuse to play around with Java 8 Streams. It's my first time actually working with streams, so I'd like reviews covering my use of the Streams.

Any comments on code style, stream usage, potential optimizations (though it still runs in less than a second on my machine) of the basic logic are appreciated.

package complete;

import java.util.HashSet;
import java.util.stream.IntStream;

public class pe023 {
    public static final int MAX_POTENTIAL_NON_ABUNDANT_SUM = 28213;

    public static void main(String[] args) {
        int result = IntStream
                .rangeClosed(1, MAX_POTENTIAL_NON_ABUNDANT_SUM)
                .parallel()
                .filter(pe023::isNonAbundantSum)
            //  .peek(System.out::println)
                .sum();

        System.out.println(result);
    }

    /**
     * A number is a Non-abundant sum if it cannot be written as the sum of two abundant numbers.
     *
     * @param num the number to check
     * @return if the number is a non-abundant sum
     */
    public static boolean isNonAbundantSum(int num) {
        for (int low=1,high=num-1; low<=high; low++,high--) {
            if (isAbundant(low) && isAbundant(high)) {
                return false;
            }
        }
        return true;
    }

    static HashSet<Integer> isAbundantCache = new HashSet<>();
    static HashSet<Integer> notAbundantCache = new HashSet<>();

    /**
     * A number is abundant if the sum of its proper divisors is greater than the number.
     *
     * @param num the number to check
     * @return if the number is abundant
     */
    public static boolean isAbundant(int num) {
        if (isAbundantCache.contains(num)) {
            return true;
        }
        if (notAbundantCache.contains(num)) {
            return false;
        }

        if (properDivisors(num).sum() > num) {
            isAbundantCache.add(num);
            return true;
        } else {
            notAbundantCache.add(num);
            return false;
        }
    }

    public static IntStream properDivisors(int num) {
        IntStream stream = IntStream.of(1);
        for (int i=2; i<=num/2; i++) {
            if (num%i == 0) {
                stream = IntStream.concat(stream, IntStream.of(i));
            }
        }
        return stream;
    }
}

// 4179871

Wrapping individual values in single-value streams and concatenating the streams is very inefficient.  You'd be better off using IntStream.Builder to build your IntStream, like this:

public static IntStream properDivisors(int num) {
    IntStream.Builder builder = IntStream.builder();
    for (int i=2; i<=num/2; i++) {
        if (num%i == 0) {
            builder.accept(i);
        }
    }
    return builder.build();
}

Better yet, since you're experimenting with Streams, convert your for loop to an IntStream using IntStream.range(), like this:

public static IntStream properDivisors(int num) {
    return IntStream.range(2, num/2 + 1).filter(i -> num % i == 0);
}


Bitarr class optimization C++ I have implemented a bitarr class that does bit manipulations. I would love to know if there is any way to make the code more optimized. Any input would be much appreciated. Thank you! Note: int main() should not be modified.
    #include <iostream>
    #include <vector>
    #include <climits>
    #include <cstring>

    template<size_t NumBits>
    class bitarr
    {
    private:
        static const unsigned NumBytes = (NumBits + CHAR_BIT - 1) / CHAR_BIT;//find number of bytes to track least memory footprint
        unsigned char arr[NumBytes];
    public:
        bitarr() { std::memset(arr, 0, sizeof(arr)); } //initialize array to 0

        void set(size_t bit, bool val = true) {
            if (val == true)
            {
                arr[bit / CHAR_BIT] |= (val << bit % CHAR_BIT);//left shift and OR with masked-bit
            }
        }

        bool test(size_t bit) const {
            return arr[bit / CHAR_BIT] & (1U << bit % CHAR_BIT); //left shift and AND with masked-bit
        }
        const std::string to_string(char c1, char c2)
        {
            std::string str;
            for (unsigned int i = NumBits; i-- > 0;)
                str.push_back(static_cast<char>
                ('0' + test(i)));
            while (str.find("0") != std::string::npos) {
                str.replace(str.find("0"), 1, std::string{ c1 });
            }
            while (str.find("1") != std::string::npos) {
                str.replace(str.find("1"), 1, std::string{ c2 });
            }
            return str;
        }
        friend std::ostream& operator<<(std::ostream& os, const bitarr& b)
        {
            for (unsigned i = NumBits; i-- > 0; )
                os << b.test(i);
            return os << '\n';
        }
    };

    int main()
    {
        try
        {
            bitarr<5> bitarr;
            bitarr.set(1);
            bitarr.set(2);

            const std::string strr = bitarr.to_string('F', 'T');
            std::cout << strr << std::endl;
            if (strr != "FFTTF")
            {
                throw std::runtime_error{ "Conversion failed" };
            }
        }
        catch (const std::exception& exception)
        {
            std::cout << "Conversion failed\n";
        }
    }

Use size_t consistently
You use both size_t and unsigned for counting. Stick with size_t.
Use default member initialization
You can use default member initialization to ensure arr[] is initialized, without having to call memset():
class bitarr
{
    static const unsigned NumBytes = (NumBits + CHAR_BIT - 1) / CHAR_BIT;
    unsigned char arr[NumBytes] = {};
    ...
};

You can then also remove the constructor completely.
Unnecessary if-statement in set()
You don't need to check whether val is true in set(). If it is false, the body of the if-statement will still do the right thing. While it might look like that would do a lot of work for nothing, the processor might easily mispredict this condition, making it less efficient than not having the if at all.
Make all functions that do not modify arr[] const
You made the function test() const, but to_string() also does not modify the bit array, so you can make that function const as well.
Optimize to_string()
Your function to_string() is very inefficient. The caller provides you with the characters to use for the representation of one and zero bits, but you first ignore that and build a string of '0' and '1', and then replace those characters one by one. Why not build the string directly using c1 and c2? Also, since you know how long the string will be, you should reserve space for all the characters up front.
std::string to_string(char c1, char c2) const
{
    std::string str;
    str.reserve(NumBits);

    for (size_t i = NumBits; i-- > 0;)
        str.push_back(test(i) ? c2 : c1);

    return str;
}

You implementation also has bugs: to_string('0', '1') results in an infinite loop, and to_string('1', '0') always results in a string with all zeroes.


Passing around context.Context in Golang I am currently "gracefully" handling HTTP server net/http shutdown which works perfectly fine (I am not asking you to review server shutdown). As seen in the code below the context is created first time directly in server package when I run the application. What I've done afterwards is (this is the one I would like you to review please), I've created a context.Background() before calling the server and pass it to server. Is this acceptable/feasible practise in Golang?

Why I want to create a context beforehand while everything is already working fine?

It is because I need a few key-value pair in context so that I can use them throughout the application.

CURRENT IMPLEMENTATION WITH CONTEXT IN SERVER

cmd/auth/main.go

package main

import (
    "github.com/BentCoder/auth/internal/http"
)

func main() {
    srv := http.NewServer(":8011", 10)
    srv.Start()
}

internal/http/server.go

package http

import (
    "context"
    "log"
    "net/http"
    "os"
    "os/signal"
    "syscall"
    "time"
)

type Server struct {
    http    *http.Server
    timeout int64
}

func NewServer(adr string, tim int64) Server {
    return Server{
        http: &http.Server{
            Addr: adr,
        },
        timeout: tim,
    }
}

func (srv *Server) Start() {
    log.Infof("starting HTTP server")

    idle := make(chan struct{})

    go shutdown(srv.http, idle, srv.timeout)

    if err := srv.http.ListenAndServe(); err != http.ErrServerClosed {
        log.Fatalf("failed to start/close HTTP server [%v]", err)
    }

    <-idle

    log.Info("shutdown HTTP server")
}

func shutdown(srv *http.Server, idle chan<- struct{}, tim int64) {
    sig := make(chan os.Signal, 1)

    signal.Notify(sig, os.Interrupt, syscall.SIGINT, syscall.SIGTERM, syscall.SIGHUP)

    <-sig

    log.Infof("shutting down HTTP server in '%d' sec", tim)

    ctx, cancel := context.WithTimeout(context.Background(), time.Duration(tim)*time.Second)
    defer cancel()

    if err := srv.Shutdown(ctx); err != nil {
        log.Fatalf("failed to shutdown HTTP server [%v]", err)
    }

    close(idle)
}

output

INFO[0000] starting HTTP server                          source="server.go:42"
^C
INFO[0004] shutting down HTTP server in '10' sec         source="server.go:82"
INFO[0004] shutdown HTTP server                          source="server.go:54"

THIS IS WHAT I DECIDED TO DO

package main

import (
    "context"
    "github.com/BentCoder/auth/internal/http"
)

func main() {
    ctx := context.WithValue(context.Background(), "SomeKey", "SomeVal")

    srv := http.NewServer(":8011", 10)
    srv.Start(ctx)
}

And update server package (the relevant bit) as seen below.

func shutdown(ctx context.Context, .....
    ...
    ctx, cancel := context.WithTimeout(ctx, time.Duration(tim)*time.Second)
    ...
}

You've actually improved the design, because now unit tests can inject their own context and thus the shutdown() becomes testable. Pure gold.

But your stated reasons make me wary. I'd pass a parameter (e.g. struct) to Start() or NewServer()  There aren't many good uses of WithValue that make your code more readable and neater.

Also docs for WithValue warn that "The provided key must be comparable and should not be of type string or any other built-in type to avoid collisions between packages using context." So that's a big problem with your example.


Caesar Encryption Tool I'm newbie in Python and I wrote a tool called Caesar Encryption Tool. At the moment it only encrypts. It takes a plain text and a key from the user then encrypts the plain text using the key. How can this code be improved?

# This is version 1.0 Caesar Encryption Tool and it will be improved.
# This tool is written by Mahmoud Naguib.
# My facebook account is at : https:\\www.facebook.com\naguibarea
# This is letters table.
lettersTable = ['A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q',
            'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z']
# This line takes the user's key it's 3 by default.
key = int(input('Enter your key : '))
# This line takes the user's plain text and converts it
#  to uppercase to search for it in the English alphabet.
plainText = input('Enter your plain text : ').upper()
for letter in range(len(plainText)):
    # If there is a letter which isn't alphabet like numbers,
    #  special characters and spaces it will be appended to the list.
    if plainText[letter] not in lettersTable:
        lettersTable.append(plainText[letter])
    # This line locates the plain letter's numeric value in other words its    position.
    position = lettersTable.index(plainText[letter])
    # This line adds the key entered by the user to the plain letter's
    #  numeric value to reach the new encrypted letter.
    step = position + key
    # if the encrypted letter's numeric value is greater than 25
    #  then its new numeric value is the old numeric valuu %
    #  the number of English alphabet letters.
    if step > 25:
        step %= 26
    # If position is greater than 25 that means it's not an alphabet
    #  value and it will be printed without adding the key.
    if position > 25:
        step = position
    # This line locates the encrypted letter's numeric value in other words
    #  its position and prints the encrypted letter.
    # end is a keyword argument to combine all letters together.
    print(lettersTable[step], end='')

Welcome to Code Review.

I would be going through this code quickly.

Read PEP 8. It is de facto standard for coding in python. Generally, we name the variables in lower_case_with_underscore, and any constant used as CONSTANT_UNDERSCORE.
Use in-built functions. You are using your letters_table to find out whether the character is alphabet or not, use isalpha() instead. Similarly you can use ord and chr functions which would allow you to preserve case. Using all of these functions would remove the need of letters_table
Keep consistent comments. Your comments indicated that key default value is 3. It is not, and if I leave input blank, it would raise error.
Make your code modular. You can easily separate your I/O logic with implementation logic. (Use functions)

Since you indicate you want to improve this code yourself, I would refrain from posting refactored version of your code. (If you want me to as an example, I can do that).

Here are some more improvements which you could incorporate:

Allow user an option to select between encryption and decryption mode. (with functions, this would be done by having an extra parameter with default value)
Other encryption logic while maintaining the same I/O logic. (Would help you understand power of function modules)

Some Python code quality skills which might be handy in future:

Write useful docstring (see examples in link). While for this
program, your comments are more than enough, it is useful skill.
Learn to write some assertion tests and then unit tests.


A get_from_variant function in C++ This is the follow-up question for A recursive_transform Function For Various Type Nested Iterable With std::variant Implementation in C++. As G. Sliepen's answer mentioned, leaving only recursively transforming operation for recursive_transform() may be a better idea. As the result, the implementation of recursive_transform function is kept in the following form. Moreover, the forward declarations have been removed.
template<class T, class _Fn> requires is_iterable<T>
static inline T recursive_transform(const T input, _Fn func)
{
    T returnObject = input;

    std::transform(input.begin(), input.end(), returnObject.begin(), func);
    return returnObject;
}

template<class T, class _Fn> requires is_iterable<T> && is_element_iterable<T>
static inline T recursive_transform(const T input, _Fn func)
{
    T returnObject = input;
    std::transform(input.begin(), input.end(), returnObject.begin(),
        [func](const auto& element)
        {
            return recursive_transform(element, func);
        }
    );
    return returnObject;
}

However, I still want to handle the compound structure with ranges and std::variant, such as std::vector<std::variant<double>>. A new function get_from_variant comes up in my mind in order to focus on the operations with these things.
template<typename T_variant, typename T>
static inline auto get_from_variant(T_variant input_variant)
{
    T return_val;
    std::visit([&](auto&& arg)
        {
            return_val = static_cast<T>(arg);
            return arg;
        },
        input_variant);
    return return_val;
}

The tests of this get_from_variant function:
int main()
{
    //  get_from_variant function test
    std::variant<double> testNumber = 1;
    std::cout << get_from_variant<decltype(testNumber), double>(testNumber);

    //  The usage of recursive_transform function and get_from_variant function
    std::variant<double> variant_number = 3.14;

    std::vector<decltype(variant_number)> testVector1;
    testVector1.push_back(variant_number);
    testVector1.push_back(variant_number);
    testVector1.push_back(variant_number);
    std::cout << get_from_variant<std::variant<double>, double>(recursive_transform(testVector1, [](auto x){ return get_from_variant<std::variant<double>, double>(x) + 1; }).at(0)) << std::endl;

    return 0;
}

All suggestions are welcome.

Which question it is a follow-up to?
A recursive_transform Function For Various Type Nested Iterable With std::variant Implementation in C++

What changes has been made in the code since last question?
In order to handle the compound structure with ranges and std::variant, such as std::vector<std::variant<double>> in a better way, a new function get_from_variant has been created.

Why a new review is being asked for?
In my opinion, I am not sure whether the design of the function get_from_variant is good? Is the idea or the usage good or not? Any comment is welcome.

I haven't been following this thread from the beginning, so I'm more confused than you expect readers to be by this point. It would be a good idea for you to provide a complete compilable example each time — even just as a Godbolt link, if you want to keep the question's focus on some little piece of the code.
In fact, I prefer to see a Godbolt link (in addition to seeing the code in the question as you correctly have done), as it saves me the trouble of pasting your code into Godbolt myself. :) Here's a link to your code: Godbolt.

std::variant<double> testNumber = 1;

This doesn't compile in C++20. Did it use to? If so, yikes, that's a pretty big API break for C++... but not your problem. Anyway, change it to 1.0 and recompile.

template<typename T_variant, typename T>
static inline auto

Lose the static inline. Templates are effectively inline by definition, and you don't want this template to be static — you don't want to force each translation unit to keep its own unique copy (in the case that it's not optimized away by the inliner).
I'm not a fan of Giraffe_case. Template parameter names should be short and CamelCase; here I recommend V.
Your std::visit lambda has a useless return arg;. In fact, this entire function should look more like
template<class V, class T>
auto get_from_variant(V input) {
    return std::visit([&](auto&& arg) {
        return static_cast<T>(arg);
    }, input);
}

With the cruft removed, we have brain cells free to focus on the next level of pedantry: You're taking arg by forwarding reference (auto&&), but you're not actually forwarding it to the static_cast. Maybe we should use static_cast<T>(static_cast<decltype(arg)>(arg)) here, so that if arg is an rvalue reference, it'll get moved into T's constructor?
But wait; arg will never be an rvalue reference, because we're visiting an lvalue input! So maybe we shouldn't expect to modify the arg we visit — we could take it as const auto& arg. But if we don't expect to modify input, maybe it should be taken by— yeah, wait a minute, why are we making a copy of input here? Just take it by const reference to begin with!
template<class V, class T>
auto get_from_variant(const V& input) {
    return std::visit([](const auto& arg) {
        return static_cast<T>(arg);
    }, input);
}

I've dropped the [&] from the lambda, since it doesn't require any captures.
We should also look at the template parameters to get_from_variant. V can be deduced and T can't; it always always always makes sense to put non-deducible parameters first.
template<class T, class V>
auto get_from_variant(const V& input) {
    return std::visit([](const auto& arg) {
        return static_cast<T>(arg);
    }, input);
}

Now our main driver looks like this:
std::variant<double> testNumber = 1.0;
std::cout << get_from_variant<double>(testNumber);

std::vector testVector1 = {
    std::variant<double>(3.14),
    std::variant<double>(3.14),
    std::variant<double>(3.14),
};
std::cout << get_from_variant<double>(
    recursive_transform(testVector1, [](const auto& x){
        return get_from_variant<double>(x) + 1;
    }).at(0)
) << std::endl;

Meanwhile, in recursive_transform, you have a typo: const T input when you meant const T& input. You can mechanically grep for these typos, and you should!

Again, remove static inline from templates.

The name _Fn is reserved for the implementation; just use F.

Copying func into the lambda isn't necessary; you should use [&] as your default for every lambda you write (unless, as above, you can get away with plain []).

Honestly, unless you are rabid about following STL idioms, just pass the callback F by const reference and avoid ever copying it. There is a place in C++ for stateful, mutable callbacks, but transform is not that place.

Your base case is more complicated than it needs to be. Let's fix that.

Putting it all together:
template<class T, class F>
T recursive_transform(const T& input, const F& f) {
    return f(input);
}

template<class T, class F> requires is_iterable<T>
T recursive_transform(const T& input, const F& f) {
    T returnObject = input;
    std::transform(input.begin(), input.end(), returnObject.begin(),
        [&](const auto& element) {
            return recursive_transform(element, f);
        }
    );
    return returnObject;
}

And then, it really seems to me that using std::transform here is overkill: it reads from input twice, once to make the copy and again to do the transform. Suppose we just open-coded it, like this?
template<class T, class F> requires is_iterable<T>
T recursive_transform(const T& input, const F& f) {
    T output = input;
    for (auto&& elt : output) {
        elt = recursive_transform(elt, f);
    }
    return output;
}

Of course we could use C++20 Ranges to do something like this:
template<class T, class F> requires is_iterable<T>
T recursive_transform(const T& input, const F& f) {
    auto transformed = input | std::views::transform([&](auto&& x) {
        return recursive_transform(x, f);
    });
    return T(transformed.begin(), transformed.end());
}

That's slower to compile and generates bigger code — but it might indeed be faster at runtime, if T::value_type is expensive to copy, because we're eliminating the copy-assignments on T::value_type — we're just constructing directly in place.


One producer and multiple consumers wherein the producer has to wait until all consumers finish before adding more data I am trying to solve the scenario involving 1 producer and 5 consumers. The producer puts data to queue and waits till all consumers have finished before adding more data.

Please review and let me know if there is room for improvement.

import java.util.LinkedList;
import java.util.Queue;

public class ReadUpdate {

   static int cntr;
   static volatile int writeLck = 1;
   static int numSupplied;
   Queue queue = new LinkedList<Node>();

    public static void main(String arg[]) {
        ReadUpdate rU = new ReadUpdate();
        Producer p = new Producer(rU.queue);
        p.start();
        Consumer c1 = new Consumer(rU.queue, 1);
        c1.setName("1");
        c1.start();
        Consumer c2 = new Consumer(rU.queue, 2);
        c2.setName("2");
        c2.start();
        Consumer c3 = new Consumer(rU.queue, 3);
        c3.setName("3");
        c3.start();
        Consumer c4 = new Consumer(rU.queue, 4);
        c4.setName("4");
        c4.start();
        Consumer c5 = new Consumer(rU.queue, 0);
        c5.setName("5");
        c5.start();
    }
 }

class Consumer extends Thread {
    Queue queue;
    int turn;

    Consumer(Queue queue, int turn) {
        this.queue = queue;
        this.turn = turn;
    }

    public void run() {
        while (ReadUpdate.numSupplied <= 100) {
            synchronized (ReadUpdate.class) {
                while (queue.isEmpty() || ReadUpdate.cntr % 5 != this.turn) {
                    try {
                        ReadUpdate.class.wait();
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }
                System.out.println("THREAD " + Thread.currentThread().getName()
                        + " GOT " + queue.poll());
                if (ReadUpdate.cntr % 5 == 0) {
                    ReadUpdate.writeLck = 1;
                } else {
                    ReadUpdate.cntr++;
                }
                ReadUpdate.class.notifyAll();
            }
        }
    }
}

class Producer extends Thread {
    Queue queue;

    Producer(Queue queue) {
        this.queue = queue;
        ReadUpdate.numSupplied++;
    }

    public void run() {
        while (ReadUpdate.numSupplied < 100) {
            synchronized (ReadUpdate.class) {
                while (ReadUpdate.writeLck == 0) {
                    try {
                        ReadUpdate.class.wait();
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }
                for (int i = 0; i < 5; i++) {
                    queue.offer(ReadUpdate.numSupplied++);
                }
                ReadUpdate.writeLck = 0;
                ReadUpdate.cntr++;
                ReadUpdate.class.notifyAll();
            }
        }
    }
}

It's not recommended to inherit from Thread. Instead implement Runnable and pass it to a new Thread which you store in a member and provide accesses for what you need. This also lets you pass a ThreadFactory for some customization options for the threads used.

class Producer extends Runnable{

    private Thread thr;

    Producer(Queue queue, ThreadFactory fact) {
        if(fact == null)
            thr = new Thread(this, "producer thread");
        else
            thr = fact.newThread(this);
        //...
    }

    public void start(){
        thr.start();
    }

    //...
}

As for synchronizing if you only needed the consumers to run once you could have used a CountDownLatch but that cannot be reset. So you'll have to use a CyclicBarrier.

With a cyclic barrier you can make the producer the Runnable that gets invoked when the last thread calls its await. Then you can be sure all but one consumer threads are waiting for the producer code to finish.


Getting an array of certain objects within an object I'm trying to get an array of all "form" objects within a large "masterFormObject" object. Is this the cleanest way to do so, or is there a better way I can check if the form objects themselves are not undefined directly and check both the applicant and account portion of this form object without having two separate blocks of undefined checks / forEach loops?

vm.getAllForms = function (type) {

  var allForms = [];
  var accountForms = [];
  var applicantForms = [];

  if (undefined != vm.masterFormObject) {
    if (undefined != vm.masterFormObject.payload) {
      if (undefined != vm.masterFormObject.payload.accounts) {
        vm.masterFormObject.payload.accounts.forEach(function (account) {
          if (undefined != account.Forms) {

            account.Forms.forEach(function (Form) {

              allForms.push(Form);
              accountForms.push(Form);

            });
          }
        });
      }

      if (undefined != vm.masterFormObject.payload.accounts) {
        vm.masterFormObject.payload.applicants.forEach(function (applicant) {
          if (undefined != applicant.Forms) {
            applicant.Forms.forEach(function (Form) {
              allForms.push(Form);
              applicantForms.push(Form);
            });
          }
        });
      }
    }
  }

Your nested "ifs" make the code very hard to read.  I would think about changing your approach to something like

if (undefined === vm.masterFormObject) {
    console.log('Some error message.');
    return;
}
if (undefined === vm.masterFormObject.payload) {
    console.log('Some other message');
    return;
}
// and so on

// or if inside a loop
if (undefined === account.Forms) {
    console.log('Some error message.');
    // continue to next iteration
    continue;
}


Implementation of Observer Pattern in C++14 I have written a small class for implementing the observer pattern in C++(14). I would welcome any comments, corrections criticisms and suggestions.

Observable.h:

#include <functional>
#include <vector>

class Observable
{
public:
    using Callback = std::function<void(void)>;

    template<typename O,typename F>
    void AddObserver(O self, F f)
    {
        obs.push_back(Callback([self,f](){(self->*f)();}));
    }

    void NotifyObservers(){for(auto f: obs){f();}}

private:
    std::vector<Callback> obs{};
};

Sample Code

main.cpp

#include <iostream>

#include "Observable.h"

class A : public Observable
{
    private:
        int a;
        std::string s;
};

class B : public Observable
{
public:
    void EventHappened()
    {
        std::cout << "B has observed an event\n";
    }
};

void bar()
{
    std::cout << "This will not work";
}

class C
{
public:
    explicit C(A& a,B& b) : m_a(a), m_b(b)
    {
        a.AddObserver(this,&C::ReactToEventFromA);
        b.AddObserver(this,&C::ReactToEventFromB);

        //a.AddObserver(this,bar); //This will Fail.
        //b.AddObserver(this,&B::EventHappened); //This will Fail.
    }

    void ReactToEventFromA()
    {
        std::cout << "C has observed the event from A\n";
    }

    void ReactToEventFromB()
    {
        std::cout << "C has observed the event from B\n";
    }

private:
        A& m_a;
        B& m_b;
};

int main()
{
    A a;
    B b;
    C c(a,b);

    a.AddObserver(&b,&B::EventHappened);

    a.NotifyObservers();
    b.NotifyObservers();
}

The basic idea is that, to preserve simplicity, when you want a class to be able to notify observers, you just inherit publicly from Observable. And you use the AddObserver method to register any member function of any object to observe the class.

This is just a simple quick mock up, therefore the signature of the callback has been hardcoded to void(void). It shouldn't be too difficult to add the ability to register more complex functions. But this is it for now.

I'm not sure we really need a base Observable class to inherit from anymore, because with lambdas and function type erasure, this pattern has become more or less trivial.

But if you want to do it that way, I think you can simplify and generalize it a bit more, by relaxing constraints in addObserver's signature (which I renamed attach because it's the canonical name if I'm not mistaken):

#include <iostream>
#include <functional>

class Observable {
    using Callback = std::function<void(void)>;
    std::vector<Callback> notifications;
public:
    void attach(Callback c) { notifications.push_back(c); }
    void notify() { for (auto& c : notifications) c(); }
};

struct Buzzer {
    void go() { std::cout << "drrrr... drrrr...\n"; }
};

struct Rooster {
    void sing() { std::cout << "Cocorico!\n"; }
};

struct Sun : public Observable {
    void rise() { std::cout << "Waoooh! the sun's rising!\n"; notify(); }
};

int main() {
    Sun sun;
    Buzzer buzzer;
    Rooster rooster;

    sun.attach([&buzzer] {buzzer.go();});
    sun.attach([&rooster] {rooster.sing();});
    sun.rise();
}


JavaScript canvas bouncing objects I have been playing with JavaScript for some time now and made a simple canvas animation. I tried to make my code clean but I would like to hear your opinions about it.

GitHub

'use strict';

var Ball = function (options = {}) {
 var self = {};

 var x = options.x || 0;
 var y = options.y || 0;
 var width = options.width || 20;
 var height = options.height || 20;
 var color = options.color || '#aaaaaa';
 var verticalSpeed = options.verticalSpeed || 1;
 var horizontalSpeed = options.horizontalSpeed || 1;

 self.move = function () {
  x += horizontalSpeed;
  y += verticalSpeed;
 };

 self.verticalBounce = function () {
  verticalSpeed = -verticalSpeed;
 };

 self.horizontalBounce = function () {
  horizontalSpeed = -horizontalSpeed;
 };

 self.getX = function () {
  return x;
 };

 self.getY = function () {
  return y;
 };

 self.getWidth = function () {
  return width;
 };

 self.getHeight = function () {
  return height;
 };

 self.getColor = function () {
  return color;
 };

 self.getVerticalSpeed = function () {
  return verticalSpeed;
 };

 self.getHorizontalSpeed = function () {
  return horizontalSpeed;
 };

 return Object.seal(self);
};

var Board = function (options = {}) {
 var self = {};

 var objects = [];
 var canvas = options.canvas || document.getElementsByTagName('canvas')[0];
 var ctx = canvas.getContext('2d');

 var moveObjects = function () {
  objects.forEach(function (object) {
   object.move();
  });
 };

 var detectCollisions = function () {

  var wallCollision = function (object) {
   if (object.getX() <= 0 || (object.getX() + object.getWidth()) > canvas.width) object.horizontalBounce();
   if (object.getY() <= 0 || (object.getY() + object.getHeight()) > canvas.height) object.verticalBounce();
  };

  var areColliding = function (objectA, objectB) {
   return (
    objectA.getX() < objectB.getX() + objectB.getWidth() &&
    objectA.getX() + objectA.getWidth() > objectB.getX() &&
    objectA.getY() < objectB.getY() + objectB.getHeight() &&
    objectA.getHeight() + objectA.getY() > objectB.getY()
   );
  }

  objects.forEach(function (currentObject, index, objects) {
   wallCollision(currentObject);
   for (var i = index + 1; i < objects.length; i++) {
    var nextObject = objects[i];
    if (areColliding(currentObject, nextObject)) {
     if (Math.sign(currentObject.getHorizontalSpeed()) !== Math.sign(nextObject.getHorizontalSpeed())) {
      currentObject.horizontalBounce();
      nextObject.horizontalBounce();
     }
     if (Math.sign(currentObject.getVerticalSpeed()) !== Math.sign(nextObject.getVerticalSpeed())) {
      currentObject.verticalBounce();
      nextObject.verticalBounce();
     }
    }
   }
  });
 };

 var draw = function () {
  ctx.clearRect(0, 0, canvas.width, canvas.height);
  objects.forEach(function (object) {
   ctx.fillStyle = object.getColor();
   ctx.fillRect(object.getX(), object.getY(), object.getWidth(), object.getHeight());
  });
 };

 self.frame = function () {
  moveObjects();
  draw();
  detectCollisions();
 };

 self.addObject = function (object) {
  objects.push(object);
 };

 return Object.seal(self);
};

var ball1 = Ball();
var ball2 = Ball({horizontalSpeed: -1, x: 638, y: 123, color: '#ff0000', width: 25, height: 60, verticalSpeed: 2, horizontalSpeed: 2});
var ball3 = Ball({verticalSpeed: -1, x: 235, y: 453, color: '#00ff00', width: 75, height: 50});
var ball4 = Ball({horizontalSpeed: 1, x: 300, y: 300, color: '#0000ff', width: 50, height: 30});
var board = Board();

board.addObject(ball1);
board.addObject(ball2);
board.addObject(ball3);
board.addObject(ball4);

setInterval(board.frame, 1000/60);
body {
 background: #000;
 text-align: center;
}

canvas {
 background: #434343;
 border: 5px solid #323232;
}
<!doctype html>
<html>
<head>
 <link rel="stylesheet" href="css/main.css">
</head>
<body>
 <canvas width="800" height="600"></canvas>
</body>
</html>

Some observations:

Try not to define methods inside the Ball constructor, it uses more memory since a new function is created for each ball instance. Rather create them using ES2016 classes (if available) or defining them on the prototype.
Rather than defining getX, getY, etc, methods rather create properties (again using ES2016 syntax if possible or otherwise defineProperty)
I would add draw and checkCollision methods to the Ball class and move the code there.
I would add a class and objects for the walls. This has the advantage that they can draw themselves in the same way as a Ball (The main program just becomes a loop asking every object to draw itself) and that you can generalize collision detection as just being between two objects without worrying about their types. As you add different objects to your code this keeps things much simpler.


Reducing code complexity for UsergridClient constructor I'm initializing a class in JavaScript with a bunch of complex property checks. Codacy is showing that this is bad design and recommends reducing cyclomatic complexity.

I'm at a loss for how to improve/streamline this, though.

var ok = require('objectkit')

var UsergridClient = function(opts) {
    var self = this

    // required properties
    if (arguments.length === 2) {
        self.orgId = arguments[0]
        self.appId = arguments[1]
    } else {
        self.orgId = ok(opts).getIfExists('orgId') || ok(config).getIfExists('usergrid.orgId')
        self.appId = ok(opts).getIfExists('appId') || ok(config).getIfExists('usergrid.appId')
    }

    // optional initializer properites
    self.baseUrl = ok(opts).getIfExists('baseUrl') || ok(config).getIfExists('usergrid.baseUrl') || 'https://api.usergrid.com' // default: https://api.usergrid.com
    self.clientId = ok(opts).getIfExists('clientId') || ok(config).getIfExists('usergrid.clientId') // default: undefined
    self.clientSecret = ok(opts).getIfExists('clientSecret') || ok(config).getIfExists('usergrid.clientSecret') // default: undefined
    self.tokenTtl = ok(opts).getIfExists('tokenTtl') || ok(config).getIfExists('usergrid.tokenTtl') || 3600 // time in seconds, default:  3600
    self.authFallback = ok(opts).getIfExists('authFallback') || ok(config).getIfExists('usergrid.authFallback') || 'none' // ('none'|'app') default: 'none'

    // optional post-init properties
    self.paginationPreloadPages = 0 // number of pages to preload
    self.paginationCacheTimeout = 300 * 1000 // default: 300 seconds
    self.paginationCursors = [] // array of pagination cursors

    if (self.orgId !== undefined && self.appId !== undefined) {
        return self
    } else {
        throw new Error('"orgId" and "appId" parameters are required when instantiating UsergridClient')
    }
}

The variable self here is redundant.

The parameter name "opts" is significantly less commonly used than "options".

It is not clear immediately what can be supplied in opts. Inferring how the function can be called from the logic of the code is error-prone.

The code implies that the various optional properties might be in one of two locations, or missing. I would ensure that callers position these values in one location for the call, making testing simpler.

The objectkit library is so verbose as to obfuscate the code quite effectively.

Simply ensure the property exists and negate the need for the getIfExists at all.

Setting default values where none is supplied can be performed easily with Object.assign polyfills and similar libraries.

The following appear to always be the same, if so, do they need to be own-properties?

self.paginationPreloadPages = 0 // number of pages to preload
self.paginationCacheTimeout = 300 * 1000 // default: 300 seconds

The following check should be at the start of the function to short-circuit all the work:

if (self.orgId !== undefined && self.appId !== undefined) {

none and app are magic strings that should be pulled into a single enumeration object.

return self is redundant if the intention is to use this as a constructor function.

So applying these changes:

var AuthFallback = {
    APP: 'APP',
    NONE: 'NONE',
};

var defaultOptions = {
    orgId: null,
    appId: null,
    baseUrl: 'https://api.usergrid.com',
    clientId: null,
    clientSecret: null,
    tokenTtl: 3600,
    authFallback: AuthFallback.NONE,
};

function UsergridClient(options) {
    if (!options || !(options.orgId && options.appId)) {
        throw new Error('options.orgId and options.appId required.');
    }

    Object.assign(this, defaultOptions, options); // or polyfill
}

UsergridClient.prototype.paginationPreload = 0; // pages.
UsergridClient.prototype.paginationCacheLifetime = 300 * 1000; // ms.
UsergridClient.prototype.paginationCursors = []; // Not sure about this one...


Refactor multiple if condition in JavaScript/TypeScript I want to refactor this code but am wondering how I should start it. I'm not sure this code is totally against clean code or not. I'm thinking to use RxJS or a JavaScript builtin method. I'd like to have some help here to learn how to have clean code or how I can minimize the code to few line or chain multiple method of RxJS or JS to get rid of if conditions.
public loadChart(): void {
    this.chartDataSets = [];
    let allData = this.availableDataSet;
    if(this.selectedAttribute === Dashbaord.All && this.selectedComponent === Dashbaord.All) {
    this.prepareIntialData();
}
if (this.selectedAttribute === Dashbaord.All && this.selectedComponent !== Dashbaord.All) {
    const groupedByComponent = this.pendingActionDataSet.groupByArray(x => x.component);
    allData = groupedByComponent[this.selectedComponent];
}
if (this.selectedAttribute !== Dashbaord.All && this.selectedComponent === Dashbaord.All) {
    allData = this.groupedByAttribute[this.selectedAttribute].flattenArray();//["",""]
    const components = allData.groupByArray(x => x.component);//["key":["values"]]
    this.componentOptions = Mapper.mapToSelectItemWithAll(Object.keys(components));
}
if (this.selectedAttribute !== Dashbaord.All && this.selectedComponent !== Dashbaord.All) {
    const allDataByAttribute = this.groupedByAttribute[this.selectedAttribute].flattenArray();
    const groupedByComponent = allDataByAttribute.groupByArray(x => x.component);
    this.componentOptions = Mapper.mapToSelectItemWithAll(Object.keys(groupedByComponent));
    if (!this.componentOptions.filter(x => x.value === this.selectedComponent).length) {
        this.selectedComponent = Dashbaord.All;
        allData = allDataByAttribute;
    } else {
        allData = groupedByComponent[this.selectedComponent];
    }
}
this.prepareChartData(allData);
}

Indentation Your current indentation is a bit confusing - the function's opening block's { line is not balanced with the closing block } line. At a glance, it looks like the function is 4 lines long, and like there's an extra } at the very bottom of the snippet. Consider using an IDE which can prettily format code automatically, such as VSCode - it makes things much easier when you and anyone who reads the code can identify each block and its nesting level at a glance.
Spelling Spelling matters in programming - misspellings are a very common cause of bugs. Dashbaord should probably be renamed to Dashboard, rather than using the misspelled word everywhere (which would probably eventually cause a headache or few, especially if other developers have to go over the code).
Prefer const - don't use let unless you must reassign the variable name. Using const makes code more readable when you know there's no chance of the variable name being reassigned. Here, rather than reassigning the allData variable name, you may refactor to const by calling prepareChartData immediately, rather than reassign allData. For example:
if (this.selectedAttribute === Dashbaord.All && this.selectedComponent !== Dashbaord.All) {
    const groupedByComponent = this.pendingActionDataSet.groupByArray(x => x.component);
    allData = groupedByComponent[this.selectedComponent];
}

can turn into
if (this.selectedAttribute === Dashbaord.All && this.selectedComponent !== Dashbaord.All) {
    const groupedByComponent = this.pendingActionDataSet.groupByArray(x => x.component);
    this.prepareChartData(groupedByComponent[this.selectedComponent]);
}

If you don't like repeating this.prepareChartData everywhere, you could make it a bit shorter with:
// replace Data with the appropriate type below:
const prepareData = (data: Data) => {
  this.prepareChartData(data);
};

Comparisons As for your if statements, you can save the two comparisons being done in variables first, then use those variables:
const { All } = Dashboard; // note spelling
const attribIsAll = this.selectedAttribute === All;
const selectedIsAll = this.selectedComponent === All;
if (attribIsAll && selectedIsAll) {
  // ...
} else if (attribIsAll && !selectedIsAll) {
  // ...

Another option would be to group one of the truthy comparisons together, like so:
if (attribIsAll) {
  if (selectedIsAll) {
    this.prepareIntialData();
  } else {
    const groupedByComponent = this.pendingActionDataSet.groupByArray(x => x.component);
    prepareData(groupedByComponent[this.selectedComponent]);
  }
  return; // return early; avoid dense indentation later
}
if (selectedIsAll) {
  // ...
} else {
  // ...
}

Array test You have
if (!this.componentOptions.filter(x => x.value === this.selectedComponent).length) {

In general, if you want to check that no elements in an array satisfy a condition, it would be more semantically appropriate to use .some or .every:
const componentOptionSelected = this.componentOptions.some(
  option => option.value === this.selectedComponent
);
if (componentOptionSelected) {
  prepareData(groupedByComponent[this.selectedComponent]);
} else {
  this.selectedComponent = Dashboard.All; // note spelling
  prepareData(allDataByAttribute);
}


Too many conditions in the method I am new to node.js and I am just wondering what you think about this code. For me, it seems there are too many conditions in the code. Is there any way to refactor it?

  var removeQuestions = function(questions) {
    if (questions.length < 1)
      return;

    var group_name = questions[0].group;
    var metagroup_name = questions[0].metagroup;

    // update count in groups
    QuestionGroupModel.find({group: group_name}).exec(function(err, groups) {
      if(!err) {
        groups[0].cnt = groups[0].cnt - questions.length;

        if(groups[0].cnt > 0) {
          groups[0].save(function(err2) {
            if(err2)
              console.info('error while updating group info');
            });
        } else {
          QuestionGroupModel.find({group: groups[0].group}).remove(function(err3) {
            if(!err3) {
              console.info('groups %s was deleted', groups[0].group);
            } else {
              console.info('error while deleting group');
            }
          });
        }
      } else {
        console.info('error while lookup group info');
      };
    });

    // delete questions
    for (var i = 0; i < questions.length; i++) {
      QuestionModel.remove({ _id: questions[i]._id }, function(err) {
        if(!err) {
          log.info("question removed");
        }
      });
    }
  };

Almost all of your conditions are error handling, so there's not a lot to clean up.  For clarity's sake, I would make your initial error handler a guard condition, which makes it much more obvious what's going on.  I would then break out all your inner functions into labeled functions, which makes it clear what you're trying to achieve:

QuestionGroupModel.find({group: group_name}).exec(function(err, groups) {
    if (err) {
        console.info("Error while lookup group info", err);
        return;
    }

    var deleteGroup = function(group) {
        QuestionGroupModel.find({group: groups[0].group}).remove(function(err) {
            console.info.apply(null, (err) ? ['error while deleting group', err] : ['groups %s was deleted', group]);
        });
    };

    var saveGroup = function(group) {
        group.save(function(err2) {
            if(err2)
                console.info('error while updating group info');
        });
    };

    groups[0].cnt = groups[0].cnt - questions.length;

    if (groups[0].cnt > 0) {
        saveGroup(groups[0]);  // That you have two different arguments for similar operations is a code smell.
    } else {
        deleteGroup(groups[0].group);
    }
});

I don't have a clear picture of your QuestionGroupModel, but that they take different arguments for similar operations shows that the model itself has problems, and may require refactoring.


Multiple attempts to find the best icon I have a series if statements where I am trying to find the highest resolution file in an set of files. Is there a way that I can refactor this code to be shorter and or cleaner?

begin
  icon = icons.find { |name, _| name.downcase.include? 'xxxhdpi' }
  if icon.nil?
    icon = icons.find { |name, _| name.downcase.include? 'xxhdpi' }
  end
  if icon.nil?
    icon = icons.find { |name, _| name.downcase.include? 'xhdpi' }
  end
  if icon
    File.open(tmp_icon.path.to_s, 'wb') do |f|
      icon { |_, data| f.write data }
      s3_icon = bucket.objects[icon_path].write(Pathname.new(tmp_icon.path),
                                              content_type: 'image/png')
      icon_url = s3_icon.public_url(secure: true)
    end
  end
ensure
  tmp_icon.close
  tmp_icon.unlink
end

In order to properly give precedence to higher DPI images, you need to do three separate finds like your code already does. However, this part can be cleaned up, as well as getting rid of the unnecessary icon.try do ... end call.

begin
  icon = icons.find { |name, _| name.downcase.include? 'xxxhdpi' }
      || icons.find { |name, _| name.downcase.include? 'xxhdpi' }
      || icons.find { |name, _| name.downcase.include? 'xhdpi' }

  return unless icon.defined?

  File.open(tmp_icon.path.to_s, 'wb') { |f| f.write icon[:data] }
  s3_icon = bucket.objects[icon_path].write(Pathname.new(tmp_icon.path),
                                            content_type: 'image/png')
  icon_url = s3_icon.public_url(secure: true)
ensure
  tmp_icon.close
  tmp_icon.unlink
end


Speed up two for-loops? The code below is slow. Any thoughts on speeding up?

dict1 = {}
dict2 = {}
list_needed  = []

for val in dict1.itervalues():
    for k,v in d1ct2.iteritems():
        if val == k:
            list_needed.append([val,v])

Perhaps,

for val in dict1.itervalues():
    if val in dict2:
       list_needed.append([val,dict2[val]])

This is similar to

list_needed = [[val,dict2[val]] for val in dict1.itervalues() if val in dict2]

My preference would be to make [val,v] a tuple - i.e (val,v) which is an immutable D.S and hence would be faster than a list. But it may not count for much.


Java enum containing a hash map for look up I have the below enum. I need to get the description by code. It is working but can it be improved still?

public enum Maps {

    COLOR_RED("ABC", "abc description");

    private final String code;
    private final String description;
    private static Map<String, String> mMap;

    private Maps(String code, String description) {
        this.code = code;
        this.description = description;
    }
    public String getCode() {
        return code;
    }

    public String getDescription() {
        return description;
    }

    public static String getDescriptionByCode(String code) {
        if (mMap == null) {
            initializeMapping();
        }
        if (mMap.containsKey(code)) {
            return mMap.get(code);
        }
        return null;
    }

    private static void initializeMapping() {
        mMap = new HashMap<String, String>();
        for (Maps s : Maps.values()) {
            mMap.put(s.code, s.description);
        }
    }
}

Your code is very neat and clean, there is only one thing that I would change:

private static Map<String, String> mMap;

to

private static final Map<String, String> mMap = Collections.unmodifiableMap(initializeMapping());

The reasons:

By not declaring it final and not using a call to unmodifiableMap it is mutable, and it would be possible to modify the reference using reflection or to use .remove("ABC") on the map. Declaring it as final makes sure that the referenced map cannot change and unmodifiableMap makes sure that no changes can be done to the map itself.
Multi-threading issues. As it currently stands, if two threads would call the getDescriptionByCode method at the same time you would initialize the mapping twice, which is not needed.

This also of course requires a slight change in your initializeMapping():

private static Map<String, String> initializeMapping() {
    Map<String, String> mMap = new HashMap<String, String>();
    for (Maps s : Maps.values()) {
        mMap.put(s.code, s.description);
    }
    return mMap;
}

Besides this, it all looks good. Well done!


A basic Object Orientated REPL Implementation Here is a very basic object orientated implementation of a read evaluate print loop AKA REPL. I encluded REShell to demonstrate how one could use Reple

#-*-coding:utf8;-*-
#qpy:3
#qpy:console

import re
import sre_constants

PS1 = '>> '

DEFAULT_EVALUATOR = print

class Repl(object):
    def __init__(self, evaluator=DEFAULT_EVALUATOR):
        self.ps1 = PS1
        self.evaluator = evaluator

    def read(self):
        return input(self.ps1)

    def evaluate(self):
        return self.evaluator(self.read())

    def run(self):
        while 1:
            try:
                self.evaluate()
            except KeyboardInterrupt:
                sys.exit(0)

class REShell(Repl):
    def __init__(self, data):
        self.data = data
        self.ps1 = PS1

    def evaluate(self):
        try:
            expression = re.compile(self.read())
            print(*expression.findall(self.data), sep='\n')
        except sre_constants.error as error:
            print(error)

def source_code():
    with open(__file__, 'r') as source:
        return source.read()

if __name__ == '__main__':
    data = source_code()
    print(data)
    shell = REShell(data)
    shell.run()

Looks quite good to me! There's some things I think you could improve:

When a user hits Ctrl-C, the program flow is suddenly interrupted. The call to sys.exit should use 1 as a status code to indicate that something went wrong.
The REPL prompt is currently hardcoded, but you could easily change Reple's signature to allow a custom prompt:

class Reple(object):
    def __init__(self, evaluator=DEFAULT_EVALUATOR, prompt=PS1):
        self.prompt = prompt

On that note, PS1 is a bit ambiguous, maybe rename it to DEFAULT_PROMPT.
This is more of a design argument, but in my opinion it would make more sense to have run call evaluate, making evaluator superfluous; in pseudocode:

while true
    read from stdin
    evaluate(expression)

And in Python:

class Reple(object):
    ...

    def run(self):
        while 1:
            try:
                expression = input(self.prompt)
            except KeyboardInterrupt:
                sys.exit(1)
            else:
                self.evaluate(expression=expression)

    def evaluate(self, expression):
        raise NotImplementedError("This method must be implemented in a derived class")

You can use abc.ABCMeta together with abc.abstractmethod to provide 'truly' abstract methods:

import abc

class Reple(object, metaclass=abc.ABCMeta):
    ...

    @abc.abstractmethod
    def evaluate(self, expression):
        return

And here's a Python 2 version:

import abc

class Reple(object):
    __metaclass__ = abc.ABCMeta
    ...

    @abc.abstractmethod
    def evaluate(self, expression):
        return

You don't have any docstrings, so it's hard to tell what certain methods do or how they should be implemented by subclasses of Reple.


Reading contents of a thousand CSV files I'm fairly new to VBA and just completed my first script. It's completing the task I want it to do just fine, but it's really slow in doing so. It has to open and read about 1000 CSV files for each CSV filter for specific rows and copy those into tabs in a newly created document. It then has to save and close that new document and open the next CSV.

Are there parts in the script that are not "good-practice" in the sense that they severely slow down execution time?

It takes about 3 seconds per loop iteration, so 50 minutes for all 1,000 files. The machine has also crashed halfway through, although I'm not 100% sure that's because of the script.

Sub createLists()

    Application.DisplayAlerts = False
    Application.ScreenUpdating = False

    Dim startDate As Date
    Dim endDate As Date
    Dim dateLooper As Date
    Dim currDate As String

    'Set date range for existing files
    startDate = #1/1/2012#
    endDate = #9/12/2014#

    'Array, which contains names for new worksheets as strings
    Dim tsN(1 To 12) As String
    tsN(1) = "AA11"
    tsN(2) = "AA22"
    tsN(3) = "AA33"
    tsN(4) = "AA44"
    tsN(5) = "AA55"
    tsN(6) = "AA66"
    tsN(7) = "BB11"
    tsN(8) = "BB22"
    tsN(9) = "BB33"
    tsN(10) = "BB44"
    tsN(11) = "BB55"
    tsN(12) = "BB66"

    Dim w1 As Workbook
    Dim w2 As Workbook
    Dim ws1 As Worksheet
    Dim ws2 As Worksheet

    Dim localPath As String
    localPath = ThisWorkbook.path

    'Check for folder "Lists", create if non-existend
    Dim fso, folderN
    Set fso = CreateObject("Scripting.FileSystemObject")
    folderN = localPath & "\Lists\"
    If fso.FolderExists(folderN) = False Then MkDir folderN

    For dateLooper = startDate To endDate

        currDate = Format(dateLooper, "yyyy-mm-dd")

        'Open / create workbook objects
        Set w1 = Workbooks.Open(Filename:=localPath & "\roh\daten" & currDate & ".CSV", Local:=True)
        Set w2 = Workbooks.Add()

        'Array, which contains worksheet-objects which will reference new worksheets
        Dim ts(1 To 12) As Worksheet

        'Create new file and add/name new worksheets, set references to array ts
        For i = 1 To 12
            With w2.Sheets.Add()
                .Name = tsN(13 - i)
                .Activate
            End With
            If i = 1 Then w2.Worksheets(2).Delete
            Set ts(13 - i) = ActiveSheet
        Next i

        'Copy data:
        Set ws1 = w1.Sheets(1)

        'Iterate through products and copy corresponding data to seperate sheets in prev. created new file
        For i = 1 To 12
            Set ws2 = ts(i)

            'Filter data for product
            ws1.Range("A1:H1").AutoFilter Field:=2, Criteria1:="=" & tsN(i)
            ws1.Range("A1:H1").AutoFilter Field:=7, Criteria1:="=ja"

            'Select range and copy
            Dim lastRow As Long
            lastRow = ws1.UsedRange.Rows.Count
            ws1.Range("A1:H" & lastRow).Copy ws2.Range("A1:H1")

            'Sort copied data
            With ws2
                .Range("A1").Sort Key1:=.Range("D1"), Order1:=xlAscending, DataOption1:=xlSortNormal, Header:=xlYes
            End With

            ws1.AutoFilterMode = False
        Next i

        'Save newly created file
        w2.SaveAs Filename:=localPath & "\Lists\Lists-" & currDate & ".xls", FileFormat:=xlNormal
        w2.Close

        w1.Close

    Next dateLooper

    Application.DisplayAlerts = True
    Application.ScreenUpdating = True
End Sub

First of all use early binding over late binding to slightly improve the performance of code and to take advantage of intelli-sense.

In VBE select Tools and then References. Scroll down to Microsoft Scripting Runtime and tick the checkbox to attach references to your project. That allows you to change

Dim fso, folderN
Set fso = CreateObject("Scripting.FileSystemObject")

to

Dim fso As FileSystemObject, folderN As String
Set fso = New FileSystemObject

Generally, you want to explicitly dim your variables and avoid using Variant type as it slows down the code by a lot believe or not! Bascially, don't rely on the runtime to workout the correct type for a variable as it takes its time to do so.

There is an .Activate in the loop that creates the worksheets. It's redundant there and just adds overhead -> remove it/comment it out and modify the assignment based on the ActiveSheet to

'Create new file and add/name new worksheets, set references to array ts
For i = 1 To 12
    With w2.Sheets.Add()
        .Name = tsN(13 - i)
    End With
    If i = 1 Then w2.Worksheets(2).Delete
    Set ts(13 - i) = w2.Sheets(tsN(13 - i))
Next i

The rest seems fine to me due to your current approach for reading the data. Your code seems rather clean, well indented and you have good comments in the right places.

Performance tip:

If performance is crucial to you consider another solution involving for example ADODB library and not opening the .CSV files just to get data out of them. ADODB allows you read the contents of a file into a Recordset object without actually opening the file (think of speed!). Then, you can use a very simple method of the Range class to copy the contents into the spreadsheet - Range.CopyFromRecordset.

You can see examples of how to use ADODB on my blog at http://vba4all.com - ADODB section is at the bottom of the article


Array of objects handler I've written this relatively simple class to help with managing arrays of anonymous objects in my scripts.

/**
* AoHandler - accepts an array of objects or creates a new array
* @param {array} ao
*/
function AOHandler(ao)
{
    this.ao = typeof ao === 'object' ? ao : [];

    /**
    * pluck()
    * @param {object} search
    * @param {boolean} retKey
    */
    this.pluck = function (search, retKey)
    {
        for (i = 0; i < this.ao.length; i++ )
        {
            var tmpObj = this.ao[i];

            if (tmpObj[search.key] == search.value)
            {
                return (retKey === true) ? i : tmpObj;
            }
        }
        return false;
    }
}

/**
* get()
* @param {object} search
* @return first matching object in ao array or false on failure
*/
AOHandler.prototype.get = function(search)
{
    return this.pluck(search);
}

/**
* add()
* @param {object} pushObj
* @return false on failure
*/
AOHandler.prototype.add = function(pushObj)
{
    return typeof pushObj === 'object' ? this.ao.push(pushObj) : false;
}

/**
* update()
* @param {object} search
* @param {object) updObj
* @param {boolean} merge
*/
AOHandler.prototype.update = function(search, updObj, merge)
{
    var key = this.pluck(search, true);

    if (merge === true && key !== false)
    {
        $.extend(this.ao[key], updObj);

        return true;
    }
    else if (key !== false)
    {
        this.ao[key] = updObj;

        return true;
    }

    return false;
}

/**
* remove()
* Finds the array item and removes it from the array
* @param {object} search
* @return first matching object in ao array
*/
AOHandler.prototype.remove = function(search)
{
    var key = this.pluck(search, true);

    return key !== false ? this.ao.splice(key, 1) : false;
}

Usage

var aoArray = new AOHandler();

aoArray.add({
    id : '50ef3ac',
    name : 'example',
});

aoArray.add({
    id : '50ef3ad',
    name : 'A new object'
});

var searchFor = {
    key : 'id',
    value : '50ef3ad'
}

console.log(aoArray.get(searchFor);

I'm unhappy with the one jQuery call with $.extend() but this proved the easiest way to accomplish an object merge and I use jQuery quite predominantly in the majority of my projects. Despite that can anyone suggest a different way to do this?

While this works fine I know it's far from perfect as it's limited to one search term. My thought was to add in another loop within pluck() but I'd prefer to avoid doing that, my second thought was to map the object and perform the lookup once I'd found an object with the first property match. Again any suggestions on how I might accomplish this whilst keeping the script clean and fast I'd really appreciate it.

Any other suggestions welcome, thank you for taking a look. :)

To answer your specific concern about jQuery-dependence, the jQuery extend function is basically this:

var key;
for(key in objB) {
  objA[key] = objB[key]; // copy each value from objB into objA
}

That's all there is to it really.

However, I see a number of other things in your code that I think deserve attention.

1. Why define pluck insides the AOHandler constructor, and everything else in its prototype? Just wondering, since it's an unnecessary mixing of style. pluck can just as easily go in the prototype.

2. You're using typeof ao === 'object' when determining whether to use the argument passed to the constructor or not. Use ao instanceof Array instead. Almost everything in JS is "typeof object", but what you're looking for is an array and only an array. (I'd probably be strict about it too and throw an exception if a non-array argument is passed).

3. Your code doesn't seem to handle multiple identical values very well. Say I do this:

var ao = new AOHandler(),
    obj = { foo: "bar" },
    dup = $.extend({}, obj);

ao.add(obj);
ao.add(dup);
ao.add(obj);

Now there are three items in the AOHandler's list, two of which are the exact same object, and the third being a copy. None of you functions seem prepared to deal with such a situation. For instance, calling update would update the first item, which happens to also be the third item, so in fact 2 items get updated. However, the 2nd item (which has the same values, but isn't the same object) would be untouched. Could get pretty confusing.

4. Your function names seems a little off to me. pluck usually means to pick out values from each item in a list (i.e. a map-like function); not the items themselves, and certainly not only the first one. filter, select, find, search, or fetch would all be better names, provided the function actually returns all the matches. filter is indeed the name used for the native JS function (available in ECMA 5) that, well, filters an array and return the subset that matches a comparison function.

However, your pluck can also return the index of an item. You call it key, but since this.ao is a normal numerically-indexed array index is the word to use. Key implies it won't change, but indices of an array do change when other items are added or removed.
Anyway, the usual JS name for a function that finds the index of a certain thing in an array is indexOf. So I'd suggest that you break the pluck function into an indexOf function and a filter function, since those are separate things.

You could also make the case for push instead of add, since AOHandler is pretty much a wrapper around an array, and you push items to arrays. But add is plain English (and the same that Java uses for its ArrayList), so it's fine to use.

update is more problematic though, because it really does 2 (very) different things: Merge or replace. Merge will update the object in-place, so whereever else you have a reference to it, the updates will be visible. Replacing the object is a different story altogether. Again, I'd suggest 2 functions, since the effects are so very different, and the implementations have little in common.

Here's me take:

function AOHandler(ao) {
  // if anything's passed, it must be an array
  if(typeof ao !== "undefined" && !(ao instanceof Array)) {
    throw new TypeError;
  }
  this.ao = ao || [];
}

AOHandler.prototype.indexOf = function (predicate, offset) {
  var i, l = this.ao.length;
  if(typeof predicate !== 'object') { throw new TypeError; } // type check
  offset = parseInt(offset, 10) || 0;
  // match native indexOf's handling of negative offset
  // see: https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Array/indexOf
  offset = (offset < 0 ? l - offset : offset);
  offset = offset < 0 ? 0 : offset;
  for( i = offset ; i < l ; i++ ) {
    if(this.ao[i][predicate.key] === this.ao[i][predicate.value]) {
      return i;
    }
  }
  return -1;
};

AOHandler.prototype.filter = function (predicate) {
  var offset = 0, index, results = [];
  while((index = this.indexOf(predicate, offset)) !== -1) {
    results.push(this.ao[index]);
    offset = index + 1;
  }
  return results;
};

// This, by the way, is a pluck function
AOHandler.prototype.pluck = function (key) {
  var i, l, results = [];
  for( i = 0, l = this.ao.length ; i < l ; i++ ) {
    results.push(this.ao[i][key]);
  }
  return results;
};

AOHandler.prototype.add = function (obj) {
  if(typeof obj !== 'object') { throw new TypeError; } // type check
  return this.ao.push(obj);
};

AOHandler.prototype.get = function (predicate) {
  var index = this.indexof(predicate);
   // return null - not false - if no obj is found
  return (index === -1 ? null : this.ao[index]);
};

// Simple jQuery-less merge function
AOHandler.prototype.update = function (predicate, source) {
  var matches = this.filter(predicate), i, l, key;
  if(typeof source !== 'object') { throw new TypeError; } // type check
  for( i = 0, l = matches.length ; i < l ; i++ ) {
    for(key in source) {
      // it may be a good idea to do a source.hasOwnProperty[key] check here
      matches[i][key] = source[key];
    }
  }
};

// replace all matches
AOHandler.prototype.replace = function (predicate, replacement) {
  var matches = this.filter(predicate), index, l, offset, key;
  if(typeof replacement !== 'object') { throw new TypeError; } // type check
  while((index = this.indexOf(predicate, offset)) !== -1) {
    this.ao[index] = replacement;
    offset = index + 1;
  }
};


Including the necessary polyfills for every combination of browser support My project uses a Promise polyfill and the fetch polyfill, that I am only including if the browser does not yet support it

// Browser doesn't have Promise
if (!window.Promise) {
  require(['es6-promise'], function(promise) {
    promise.polyfill();
    // Browser doesn't have Promise or fetch
    if (!window.fetch) {
      require(['fetch'], function(fetch) {
        run();
      });

    // Browser doesn't have Promise but does have fetch
    } else {
      run();
    }
  });
// Browser has Promise but doesn't have fetch
} else if (!window.fetch) {
  require(['fetch'], function(fetch) {
    run();
  });
// Browser has Promise and fetch
} else {
  run();
}

This accounts for every permutation of browser support for Promise and fetch, but it seems like a verbose way of doing things. Is there a better to dynamically load these polyfills?

Something tells me that requirejs probably has some features to help with this, but I'm not finding them at the moment.  Your situation is complicated by the fact that you have to run custom code after es6-promise is loaded before fetch is loaded and by the fact that a usual tool for these kinds of problems is promises, but you can't use them here because they may not yet be loaded.

Here's a solution that is not less overall code, but is more DRY and is more general purpose and could be reused in other projects:

The code to do the custom loading would look like this:

var loaders = [];
if (!window.Promise) {
    loaders.push({modules: 'es6-promise', fn: function(promise) {
        promise.polyfill();
    }});
}
if (!window.fetch) {
    loaders.push({modules: 'fetch'});
}
loadSequential(loaders, run);

And, the implementation of the reusable loadSequential() would look like this:

function loadSequential(list, done) {
    var i = 0;

    // load the next item in the list
    function next() {
        var item;
        if (i < list.length) {
            item = list[i++];
            require(item.modules, function() {
                if (item.fn) {
                    item.fn.apply(this, arguments);
                }
                next();
            });
        } else {
            done();
        }
    }
    // start first iteration
    next();
}

If you can allow fetch to load potentially before promises are available, then you could use the fact that fetch presumably can't exist without promises in a native browser environment (because fetch returns a promise) and could use this shortcut:

if (!window.Promise) {
    // if there's no promise, there can't be a fetch either
    require(['es6-promise', 'fetch'], function(promise) {
        promise.polyfill();
        run();
    });
} else if (!window.fetch) {
    require('fetch', run);
} else {
    run();
}


Checking if a string contains vowels I'm just learning C# and wrote a little program to check if a word contains vowels. It's fairly simple and not to exciting but I would like to know if there's easier ways to do this:

using System;

namespace check_vowels
{
    class Program
    {
        static void Main(string[] args)
        {
            string word;

            Console.WriteLine("We will check if your string contains any vowels or not.");
            Console.Write("Enter a word: ");
            word = Console.ReadLine();
            if (word.ToLower().Contains("a") || (word.ToLower().Contains("e")))
            {
                Console.WriteLine("Your word contains the vowel 'A' or the Vowel 'E', or both.");
            }
            else if ((word.ToLower().Contains("i")) || (word.ToLower().Contains("o")))
            {
                Console.WriteLine("Your word contains the vowel 'I', or 'O', or both.");
            }
            else if (word.ToLower().Contains("u"))
            {
                Console.WriteLine("Your word contains the vowel 'U'.");
            }
            else
            {
                Console.WriteLine("Your word contains no vowels.");
            }

            Console.ReadLine();
        }
    }
}

What can I do better? What did I do well? How can I shorten this?

You could potentially do:

var vowels = Console.ReadLine()
     .Where(c => "aeiouAEIOU".Contains(c))
     .Distinct();

foreach(var vowel in vowels)
     Console.WriteLine("Your phrase contains a vowel of {0}", vowel);

if(!vowels.Any())
    Console.WriteLine("No vowels have been detected.");

So you understand the code a bit:

Where() : Will test each character to see if it is indeed a vowel or not.
Distinct() : Will remove duplicate vowels found.

Then you loop through the vowels stating what exist inside, then the if will simply return when loop is empty.


Overriding operator delete/new to check correct pairs of (non-)array variants A legacy project I am maintaining in my freetime uses overrides of operator delete / operator new. These overrides over-allocate by some bytes using malloc (and free) and treat said bytes as a prefix filled with a signature different between new and new[]. Checking the signature on delete allows to discover accidental cases of "new ... delete[]" or "new[] ... delete". Returned is the malloc'ed pointer offset by the size of the prefix.

These overrides are put behind an #ifdef that does not evaluate in release mode.

I recently rewrote this part, using guidance from the relevant cppreference page and find that I am left with three questions I have trouble to satisfyingly answer. These are my questions:

Is this code even still necessary or could I replace it with some builtin feature or plugin? (Currently using latest non-preview MSVC++ toolchain, might switch to MinGW-w64 at some indeterminate time in the future... currently the project does not compile on GCC though.)
Did I avoid UB at least in the non-pathological use cases? (And even if, examples of such cases are welcome.)
The original legacy code implements both operator delete and delete[] by calling free, essentially meaning their implementation (prefix handling aside) is identical... if that must be so, then why bother in the first place?

Of course, considerations beyond those pertaining to this list are also welcome.

Code to be reviewed:

#include <cstring>

constexpr char PREFIX_ARRAY [] = "STR2MED";
constexpr char PREFIX_OBJECT[] = "str1med";
constexpr auto OFFSET_ARRAY  = std::size(PREFIX_ARRAY);
constexpr auto OFFSET_OBJECT = std::size(PREFIX_OBJECT);
using OFFSET_TYPE = decltype(OFFSET_ARRAY + OFFSET_OBJECT);

template<OFFSET_TYPE OFFSET>
inline void * guardDelete(void * pointer, const char PREFIX[OFFSET])
{
  if(pointer == nullptr)
    return pointer;
  else
  {
    char * prefix = reinterpret_cast<char*>(pointer) - OFFSET;
    ASSERT(std::memcmp(prefix, PREFIX, OFFSET) == 0);

    return prefix;
  }
}

void __cdecl operator delete  (void * pointer) { std::free(guardDelete<OFFSET_OBJECT>(pointer, PREFIX_OBJECT)); }
void __cdecl operator delete[](void * pointer) { std::free(guardDelete<OFFSET_ARRAY> (pointer, PREFIX_ARRAY)); }

template<OFFSET_TYPE OFFSET>
inline void * guardNew(void * pointer, const char PREFIX[OFFSET])
{
  ASSERT(pointer != nullptr); // Out of virtual memory?
  std::memcpy(pointer, PREFIX, OFFSET);

  return reinterpret_cast<char*>(pointer) + OFFSET;
}

void * __cdecl operator new  (std::size_t size) { return guardNew<OFFSET_OBJECT>(std::malloc(size + OFFSET_OBJECT), PREFIX_OBJECT); }
void * __cdecl operator new[](std::size_t size) { return guardNew<OFFSET_ARRAY> (std::malloc(size + OFFSET_ARRAY),  PREFIX_ARRAY); }

External dependency (ASSERT implementation; still legacy code; not to be reviewed):

// This spawns a handy Windows error message allowing one to start VS
// and attach the debugger while the context is still intact;
// with debugger already attached, it acts as a breakpoint.
void _AssertDebug(char * strAssertion, char * strFile, unsigned int uLine)
{
  static char str[1024];
  sprintf(str, "\n---  Assertion failed: %s file:%s line:%u  ---\nDo you still want to continue?", strAssertion, strFile, uLine);
  switch(MessageBox(GetActiveWindow(), str, "Fatal error!", MB_ABORTRETRYIGNORE|MB_DEFBUTTON3|MB_TOPMOST))
  {
    case IDABORT:
      std::abort();
      break;
    case IDRETRY:
      DebugBreak();
      break;
  }
}

#define ASSERT(f) \
  if (f)          \
  {}              \
  else            \
    _AssertDebug((#f), __FILE__, __LINE__)

#include <cstring>

Also include <cstdlib> (for std::malloc), and I'd include <new> out of paranoia anytime I'm messing with operator new, even if it might be unnecessary in this case.

constexpr char PREFIX_ARRAY [] = "STR2MED";
constexpr char PREFIX_OBJECT[] = "str1med";
constexpr auto OFFSET_ARRAY  = std::size(PREFIX_ARRAY);
constexpr auto OFFSET_OBJECT = std::size(PREFIX_OBJECT);

This strikes me as needlessly complicated. Also, such similar names invite accidental typo-bugs (e.g. mistyping OFFSET_ARRAY as OFFSET_OBJECT). Also, the use of the C++14 library algorithm std::size is unnecessary — (sizeof will do exactly what you want); and the use of auto is obfuscatory (the type of sizeof x is invariably size_t).

If you really wanted to use std::size, you should have included at least one of <array>, <deque>, <forward_list>, <iterator>, <list>, <map>, <regex>, <set>, <string>, <unordered_map>, <unordered_set>, <vector>.

Also, if you need these variables at all, you might consider putting them in a namespace, so that they don't conflict with the user's own global variable or function names.

using OFFSET_TYPE = decltype(OFFSET_ARRAY + OFFSET_OBJECT);

This is super duper confusing, or else it's one of those typo-bugs I was warning you about. Here you're adding together OFFSET_ARRAY (i.e. size_t(8)) and OFFSET_OBJECT (i.e. size_t(8)), taking the decltype of that (i.e. size_t), and declaring a typedef named OFFSET_TYPE. If you really wanted this typedef, you should write it as

using OFFSET_TYPE = size_t;  // or std::size_t if you enjoy typing

Related: I never write std:: for the stuff that I know is coming from libc anyway. That's a personal quirk, though. I get the impression that a lot of people do write std:: everywhere, even where it's not technically needed.

template<OFFSET_TYPE OFFSET>
inline void * guardDelete(void * pointer, const char PREFIX[OFFSET])

Here I would worry that the programmer didn't know that arrays in C and C++ are passed "by decay to pointer". I would prefer to write this as

template<size_t Offset>
inline void *guardDelete(void *ptr, const char *prefix)

so that it is obvious that Offset is not deducible. Notice also that I'm consistently using snake_case for local variable names, and CamelCase for template parameter names (such as Offset); I find that this makes code easier to decipher than using a mix of lower and UPPER case.

void __cdecl operator delete  (void * pointer)

If I cared about portability to non-MSVC targets, I'd preemptively factor out this __cdecl into a macro that I could easily turn off. However, it occurs to me that __cdecl itself is an identifier that could be defined away on non-MSVC platforms; so sure, let's leave it alone.

So putting it all together, I'd have:

#include <string.h>
#include <stdlib.h>
#include <new> // maybe

template<size_t N>
inline void *guardNew(void *pointer, const char (&guard)[N])
{
    ASSERT(pointer != nullptr); // Out of virtual memory?
    memcpy(ptr, guard, N);
    return reinterpret_cast<char*>(ptr) + N;
}

template<size_t N>
inline void *guardDelete(void *ptr, const char (&guard)[N])
{
    if (ptr == nullptr) {
        return ptr;
    } else {
        char *prefix = reinterpret_cast<char*>(ptr) - N;
        ASSERT(memcmp(prefix, guard, N) == 0);
        return prefix;
    }
}

void * __cdecl operator new(size_t size) {
    return guardNew(malloc(size + 8), "str1med");
}
void * __cdecl operator new[](size_t size) {
    return guardNew(malloc(size + 8),  "STR2MED");
}
void __cdecl operator delete(void *ptr) {
    free(guardDelete(ptr, "str1med"));
}
void __cdecl operator delete[](void *ptr) {
    free(guardDelete(ptr, "STR2MED"));
}

However, then I start worrying... is this code even doing a sensible thing? Notice that if I try to new an object of some type T where alignof(T) > 8, then the pointer returned from our operator new will not be properly aligned for that T. And the fact that it's properly aligned for alignments <= 8 feels like an accident. If we changed our guard strings from str1med and STR2MED (not obvious why we picked those strings, by the way — and again the unfortunate mix of lowercase and UPPERCASE) to, let's say, hello and world, this code would break in several different ways. So maybe we want to force the compiler to diagnose that breakage for us:

inline void *guardNew(void *pointer, const char (&guard)[8])
{
    ASSERT(pointer != nullptr); // Out of virtual memory?
    memcpy(ptr, guard, 8);
    return reinterpret_cast<char*>(ptr) + 8;
}

Now if we change the string in operator new from str1med to hello, we'll get a compiler error.

But we've still got two instances of the magic number 8 separated by a fair number of lines of code. Let's inline this guardNew helper so that we're malloc'ing and memcpy'ing in the same place:

#include <stdlib.h>
#include <string.h>

void * __cdecl operator new(size_t size) {
    void *p = malloc(size + 8);
    ASSERT(p != nullptr);  // Out of memory?
    memcpy(p, "str1med", 8);
    return (char *)p + 8;
}
void * __cdecl operator new[](size_t size) {
    void *p = malloc(size + 8);
    ASSERT(p != nullptr);  // Out of memory?
    memcpy(p, "STR2MED", 8);
    return (char *)p + 8;
}
void __cdecl operator delete(void *ptr) {
    if (ptr == nullptr) return;
    char *p = (char*)ptr - 8;
    ASSERT(memcmp(p, "str1med", 8) == 0);
    free(p);
}
void __cdecl operator delete[](void *ptr) {
    if (ptr == nullptr) return;
    char *p = (char*)ptr - 8;
    ASSERT(memcmp(p, "STR2MED", 8) == 0);
    free(p);
}

From 36 lines down to 27 — and with no pollution of the global namespace, no templates, and — mainly as a result of flattening and inlining things with complicated dependencies on each other — it's just a little bit harder to break accidentally.


Project Euler Problem #102 in F#: counting triangles that contain the origin I've recently decided to get into functional programming with F#, and decided to learn the language through Project Euler. The following is my implementation of problem 102:

Problem:

  Three distinct points are plotted at random on a Cartesian plane, for
  which -1000 ≤ x, y ≤ 1000, such that a triangle is formed.

  Consider the following two triangles:

  A(-340,495), B(-153,-910), C(835,-947)

  X(-175,41), Y(-421,-714), Z(574,-645)

  It can be verified that triangle ABC contains the origin, whereas
  triangle XYZ does not.

  Using triangles.txt (right click and 'Save Link/Target As...'), a 27K
  text file containing the co-ordinates of one thousand "random"
  triangles, find the number of triangles for which the interior
  contains the origin.

  NOTE: The first two examples in the file represent the triangles in
  the example given above.

Implementation:

type Coord = { x: int; y: int }

let origin = { x = 0; y = 0 }

let intToCoord [x; y] = { x = x; y = y }

let twiceTriangleArea a b c =
    abs (a.x * (b.y - c.y) + b.x * (c.y - a.y) + c.x * (a.y - b.y))

let containsOrigin [a; b; c] =
    twiceTriangleArea a b c = twiceTriangleArea origin a b +
        twiceTriangleArea origin b c +
        twiceTriangleArea origin a c

let solution =
    System.IO.File.ReadLines "triangles.txt"
    |> Seq.map (fun line ->
        line.Split ',' |> Array.map int |> List.ofArray |> List.chunkBySize 2 |> List.map intToCoord
    )
    |> Seq.filter containsOrigin
    |> Seq.length

printfn "%i" solution

Explanation:

This code iterates through each line of triangles.txt.
It then splits each line (by comma), takes elements in groups of 2, and converts them to coordinates.
For each of these three coordinates, it checks if they make a triangle containing the origin by checking if the sum of the areas of all triangles made from two of the passed points and the origin is equal to the total triangle area (with all three points). Note that the area isn't divided by 2 in order to prevent floating point errors. All triangles that do not meet this criteria are filtered out.
The number of remaining triangles are summed and printed to the user.

While making this, I tried to maintain a functional style as much as possible.

Here is what mainly I want to get from a review:

Is my code idiomatic and functional? Where can it be improved in this regard?
Are there any areas where I am unnecessarily doing extra computation and the code can be simplified?

It looks OK to me. You can avoid the List.ofArray call, if you change the arguments to intToCoord and containsOrigin to arrays instead of lists:

let intToCoord [|x; y|] = { x = x; y = y }

let containsOrigin [|a; b; c|] =
    twiceTriangleArea a b c = twiceTriangleArea origin a b +
        twiceTriangleArea origin b c +
        twiceTriangleArea origin a c

let solution =
    System.IO.File.ReadLines "p102_triangles.txt"
    |> Seq.map (fun line ->
        line.Split ',' |> Array.map int |> Array.chunkBySize 2 |> Array.map intToCoord
    )
    |> Seq.filter containsOrigin
    |> Seq.length

Update:

You could though "rearrange" the mapping from string to a "triangle" in this way:

let toCoord [|x; y|] = { x = int x; y = int y}

let toTriangle line = (string line).Split ',' |> Array.chunkBySize 2 |> Array.map toCoord

And then change the main function to:

let solution =
    System.IO.File.ReadLines "p102_triangles.txt"
    |> Seq.map toTriangle
    |> Seq.filter containsOrigin
    |> Seq.length

In this way you gain more even abstraction levels in the pipe chain in the main function.


Scroll to top and addClass onClick - then removeClass on .mouseout The function is to click a button, the browser window then scrolls to the top of the page, then a class is added to a menu item (with CSS to make the sub menu appear). Then when the cursor leaves the sub menu, the class is removed to allow the sub menu to disappear.

Can I make the following code shorter or more efficient? I have written it to work with Joomla and jQuery with no conflict.

jQuery(document).ready(function(){
    jQuery("#instruct-us-readmore").click(function(){
        jQuery('html, body').animate({scrollTop:0}, 800);
        setTimeout(
        function() {
            jQuery("#iceMenu_144 ul.sub_level_1").addClass("instruct-button-click");
        },950);

     });
});

  jQuery(document).ready(function(){
        jQuery("#iceMenu_144 ul.sub_level_1").mouseleave(function(){
            jQuery("#iceMenu_144 ul.sub_level_1").removeClass("instruct-button-click");
    });
      });

jQuery provides itself as first argument in the ready callback. You can assign it the $ name so you can use it shorthand inside the callback.

jQuery(function($){
  // Use jQuery as $ inside here
});

Also, you don't need multiple ready handlers. Whenever possible, just bunch them up into one.

jQuery(function($){
  // Click handler
  // Mouse handler
});

Next is that we don't know for certain how jquery.animate schedules animation. This might affect how sooner or later the callback for setTimeout is executed.

However, jquery.animate accepts a function as the last argument which is run when the animation is complete. Use that instead of doing a timeout.

$('html, body').animate({scrollTop:0}, 800, function(){
  // done animating
});