Find longest Palindrome substring I am preparing for technical interview and I found this question on geeksforgeeks, but I did not understand their solution. So, I have written my own solution. I want to optimize this code.

#include <iostream>
#include <string>

bool isPalindrome(std::string& str)
{
    for (int i = 0, j = str.length()-1; i<j; ++i, --j)
    {
        if (str[i] != str[j])
        {
            return false;
        }
    }
    return true;
}

int longestPalindrome(std::string& str, std::string& palindromeStr)
{
    int max = 0, start = 0, end = 0;
    for (int i = 0; i < str.length(); ++i)
    {
        for (int j = i+1; j < str.length(); ++j)
        {
            std::string sub = str.substr(i, j);
            if (isPalindrome(sub) && max < sub.length())
            {
                max = sub.length();
                start = i;
                end = j;
            }
        }
    }
    palindromeStr = str.substr(start, end);
    return max;
}

int main()
{
    std::string str = "forgeekskeegfor";
    std::string palindromeStr;
    std::cout << longestPalindrome(str, palindromeStr) << '\n';
    std::cout << palindromeStr << '\n';
}

The code is reasonably clear and obvious, but has some severe inefficiencies.

First, let's pick up some simple oversights.  Both isPalindrome() and longestPalindrome() ought to have internal linkage (using either the static keyword or the anonymous namespace), and the str arguments should be reference to const:

namespace {
    bool isPalindrome(const std::string& str);
    int longestPalindrome(const std::string& str, std::string& palindromeStr);
}

In passing, we can simplify the interface of longestPalindrome().  It doesn't need to return the string and its length; if we simply return the longest palindrome, then obtaining the length is trivial:

    std::string longestPalindrome(const std::string& str);

    // main() can now look like:
    //   std::string palindromeStr = longestPalindrome(str);
    //   std::cout << palindromeStr.size() << '\n';
    //   std::cout << palindromeStr << '\n';

The next oversight is that std::string::length() returns a std::size_t, so don't compare it with (signed) int:

for (std::size_t i = 0, j = str.length()-1; i<j; ++i, --j)
//   ^^^^^^^^^^^

Note that I've left a bug there (that's neatly missed because we always call this with a non-empty string): if str.length() is zero, then j starts at a very large positive value (because the subtraction is unsigned, and wraps).

BTW, there's a neat way to test a string for symmetry (at the expense of repeating the initial comparisons), using <algorithm>:

static bool isPalindrome(const std::string& str)
{
    return std::equal(str.begin(), str.end(), str.rbegin());
}

Now to the matter of efficiency.  We're creating new string objects for every possible substring of the input.  That's a lot of copying.  We could reduce that by using std::string_view.

That's only part of the way towards an efficient solution, though.  We really need to change the algorithm.  My recommendation is to iterate over each character as a possible mid-point of an embedded palindrome, and at each position, determine what's the longest palindrome possible from there (in most cases, it will be 1 or 2 chars).  There's no need to consider longer substrings centred on that position once you have a failing case, so that eliminates much of the unnecessary work we're doing here.

Hint: for this we can use std::make_reverse_iterator() and std::mismatch().

Finally, the single test we have in main() isn't really enough.  At a minimum, we want examples of odd- and even-length palindromes, and also check that we handle the trivial case of empty string as input.

Update - using iterators

I've developed the idea I hinted at in the second section; there's probably a little more scope for reducing duplication:

#include <algorithm>
#include <iostream>
#include <iterator>
#include <string>
#include <string_view>

namespace
{
    template<typename Iter>
    // requires BidirectionalIterator(Iter)
    void updateBest(Iter forward_start,
                    Iter forward_end,
                    std::reverse_iterator<Iter> backward_start,
                    std::reverse_iterator<Iter> backward_end,
                    std::string_view& best_so_far)
    {
        auto span = std::mismatch(forward_start, forward_end,
                                  backward_start, backward_end);
        auto start = span.second.base();
        auto end = span.first;
        std::string_view candidate{ &*start, static_cast<std::size_t>(std::distance(start, end)) };
        if (candidate.size() > best_so_far.size()) {
            best_so_far = candidate;
        }
    }

    std::string_view longestPalindrome(const std::string& str)
    {
        std::string_view best_so_far;

        // Work out from the middle of the string
        auto const halfway = (str.size() + 1) / 2;

        // first, loop from midpont to end of string (but we can stop
        // when there's no room for a bigger palindrome)
        for (auto i = str.begin() + halfway;  i + best_so_far.length()/2 < str.end();  ++i) {
            // test for odd-length palindrome
            updateBest(i, str.end(),
                       std::make_reverse_iterator(i), str.rend(),
                       best_so_far);
            // test for even-length palindrome
            updateBest(i + 1, str.end(),
                       std::make_reverse_iterator(i), str.rend(),
                       best_so_far);
        }

        // then, loop from midpont to beginning of string (but stop
        // when there's no room for a bigger palindrome)
        for (auto i = str.rbegin() + halfway;  i + best_so_far.length()/2 < str.rend();  ++i) {
            // test for odd-length palindrome
            updateBest(i.base(), str.end(),
                       i, str.rend(),
                       best_so_far);
            // test for even-length palindrome
            updateBest(i.base(), str.end(),
                       i + 1, str.rend(),
                       best_so_far);
        }

        return best_so_far;
    }

}

int main()
{
    for (std::string s: { "",
                "forgeekskeegfor",
                "abc abc",
                "forgeeksskeeg",
                "geeksskeegfor" }) {
        auto palindromeStr = longestPalindrome(s);
        std::cout << "Found palindrome of length " << palindromeStr.size()
                  << " in " << s << ": " << palindromeStr << '\n';
    }
}


Java function that blocks until a specific file is deleted Created for a console application that will be run in the background on a linux system, giving me a nice way to gracefully shut it down by simply deleting a file (which can be done via script/etc.)

public class BlockOnRunFile {
    private File watchedFile;
    private Path path;
    private WatchService watcher;

    public void end() {
        watchedFile.delete();
    }

    public BlockOnRunFile(String runFilePath) throws IOException {
        watcher = FileSystems.getDefault().newWatchService();
        path = FileSystems.getDefault().getPath(
                runFilePath.substring(0, runFilePath.lastIndexOf(FileSystems
                        .getDefault().getSeparator())));
        watchedFile = new File(runFilePath);
        watchedFile.createNewFile();
    }

    public void block() {
        try {
            WatchKey key;
            key = path.register(watcher, StandardWatchEventKinds.ENTRY_DELETE);

            // stall until the game is supposed to end
            // reset key to allow new events to be detected
            while (key.reset()) {

                // key = watcher.take();
                try {
                    for (WatchEvent<?> event : key.pollEvents()) {
                        WatchEvent.Kind<?> kind = event.kind();
                        if (kind == StandardWatchEventKinds.OVERFLOW) {
                            Common.log.logMessage("File watcher overflow",
                                    LogLevel.INFO);
                            if (!watchedFile.exists()) {
                                // do nothing
                                ;
                            }
                            break;
                        }
                        if (kind == StandardWatchEventKinds.ENTRY_DELETE) {
                            @SuppressWarnings("unchecked")
                            WatchEvent<Path> ev = (WatchEvent<Path>) event;
                            Path filename = ev.context();
                            if (filename
                                    .toAbsolutePath()
                                    .toString()
                                    .equals(watchedFile.getAbsolutePath()
                                            .toString())) {
                                watcher.close();
                                break;
                            }
                        }
                    }
                    Thread.sleep(1000);   // prevent CPU burn, worst-case: 1 second delay on shutdown
                } catch (Exception e) {
                    watcher.close();
                    Common.log.logMessage(e, LogLevel.INFO);
                    continue;
                }
            }// end while loop
        } catch (IOException e1) {
            Common.log.logMessage(e1, LogLevel.ERROR);
        }
    }
}

It works, and I don't expect there to be any large amount of file operations in the directory where the file exists, but I would still like to know of any possible issues (I intend to automate it via some scripts/crontab and make sure it's robust enough)

This is an interesting concept. I would link it in with, say, a pid file, and have the program terminate itself when the pid file is deleted.... and also delete it's own pid if it terminates early. I think that's why you have your end() method....
General

public class BlockOnRunFile {
    private File watchedFile;
    private Path path;
    private WatchService watcher;

The above variables should all be final too. Also, there's an uncomfortable mix of the "old" File-based system, and the "new" Path based one. I recommend that you use Path, and stick to it.
Simplifications
This code is.... ugly.

path = FileSystems.getDefault().getPath(
        runFilePath.substring(0, runFilePath.lastIndexOf(FileSystems
                .getDefault().getSeparator())));
watchedFile = new File(runFilePath);
watchedFile.createNewFile();

It can be simplified a lot:
Path watchedFile = Paths.get(runFilePath).toAbsolutePath();
Path runDir = watchedFile.getParent();
Files.deleteIfExists(watchedFile);
Files.createFile(watchedFile);

Note, there is no actual File instance in there, all just paths.... Also, there is no Watcher in there. There is no need to create the Watcher in the constructor, it is only used in the block() method, so use it there.
In your block() method, the try/catch is good, but I would add a finally:
} finally {
    Files.deleteIfExists(watchedFile);
}

The Watcher should be added as a resource to the top of that try.... and it auto-closes that way...
Also, about simplification, this code should be 1 line:

WatchKey key;
key = path.register(watcher, StandardWatchEventKinds.ENTRY_DELETE);

like:
WatchKey key = runDir.register(watcher, StandardWatchEventKinds.ENTRY_DELETE);

Then, your code body, the guts, is a little more complicated than necessary, and has a bug....
The first thing is that the inner try/catch is unnecessary, and buggy. You cannot close() the watcher and then try to reset the key, and wait for more. That will just fail. The code will simply terminate the loop, yet you have a continue statement. It's unintuitive. I would simply remove the try/catch entirely.
The second bug is in the "OVERFLOW" code. Your code tests for if the file does not exist, but it should only continue if it does exist.
Finally, why do you have the take() commented out... and use a Thread.sleep()? The take() is a blocking operation, and will 'sleep' until there is an event ready. Use the logic in the take(), and trust it...
Oh, and your handling of InterruptedException is not good.....
Finally, you go to a lot of effort to make sure that every event in the directory is 'handled', but, there is no need to handle the events. All we need to do is to make sure our sentry file is still there. If it is gone, then we die. We don't care about any other files.... this essentially removes the need for the Overflow and other conditions....
Suggestions
With the 'finally' block, the above fixes, and a little bit of "return" instead of "break", your code becomes:
import java.io.IOException;
import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.StandardWatchEventKinds;
import java.nio.file.WatchKey;
import java.nio.file.WatchService;

public class BlockOnRunFile {
    private final Path watchedFile;

    public BlockOnRunFile(String runFilePath) throws IOException {
        watchedFile = Paths.get(runFilePath).toAbsolutePath();
        Files.deleteIfExists(watchedFile);
        // create entire directory tree, if possible, to create our watch file
        // in.
        Files.createDirectories(watchedFile.getParent());
        Files.createFile(watchedFile);

    }

    public void end() throws IOException {
        Files.deleteIfExists(watchedFile);
    }

    public void block() {
        try (WatchService watcher = FileSystems.getDefault().newWatchService()) {
            final WatchKey key = watchedFile.getParent().register(watcher,
                    StandardWatchEventKinds.ENTRY_DELETE);

            // stall until the game is supposed to end
            // reset key to allow new events to be detected
            while (key.reset()) {

                // wait for a file to be deleted (or an overflow....)
                if (key != watcher.take()) {
                    throw new IllegalStateException(
                            "Only our key is registered, only it should be taken");
                }

                // now, we know something has changed in the directory, all we
                // care about though, is if our file exists.
                if (!Files.exists(watchedFile)) {
                    return;
                }

            }
        } catch (IOException e1) {
            Common.log.logMessage(e1, LogLevel.ERROR);
        } catch (InterruptedException e) {
            // propogate an interrupt... we can't handle it here.....
            // just let the file be removed, and we die....
            Thread.currentThread().interrupt();
            Common.log.logMessage(e, LogLevel.WARN);
        } finally {
            try {
                Files.deleteIfExists(watchedFile);
            } catch (IOException e) {
                // unable to delete the sentry file.....
                Common.log.logMessage(e, LogLevel.WARN);
            }
        }
    }
}


Storing values or calculating values I am trying to decide between two designs of a class. The basic difference is the one calculates properties when they are called, and the other when the object is instantiated.

I would usually just take the first one, but the calculations call each other, so when calling all of them, they get recalculated multiple times.

I, however, still prefer the first code. It is cleaner. Easier, for me, to read. I also feel the calculations are probably small enough to not worry about as opposed to storing extra values.

Is there anything else I should be considering, or looking out for? Or any reason one is amazingly better than the other?

public class Angle1
{
    private decimal angle;

    public int Degree => (int)Math.Floor(angle);
    private decimal MinuteSecond => (angle - Degree) * 60;
    public int Minute => (int)Math.Floor(MinuteSecond);
    public decimal Second => (MinuteSecond - Minute) * 60;

    public Angle1(decimal angle)
    {
        this.angle = angle;
    }
}

public class Angle2
{
    private decimal angle;

    public int Degree { get; private set; }
    public int Minute { get; private set; }
    public decimal Second { get; private set; }

    public Angle2(decimal angle)
    {
        this.angle = angle;
        Degree = (int)Math.Floor(angle);
        var minSec = (angle - Degree) * 60;
        Minute = (int)Math.Floor(minSec);
        Second = (minSec - Minute) * 60;
    }
}

I have also thought of a "hybrid" option. Store values in private values. When a property is called the first time, the value is calculated, and the private value is set. When it is called again, the private value is returned.

This hybrid option is the one I naturally go to when properties are larger objects, datasets, or binary data. I feel in this instance, it would "dirty" the code more than necessary, for the size of the stored values (integers and decimals).

I'm leaning toward Angle2...

  I also feel the calculations are probably small enough to not worry about

I infer that doing the calcs in the constructor is trivial as well - no problematic user-perceived lag, hesitation, slowness.

Communicating through design

How would I interpret - make an initial guess about - design, intent, and use from what I see here?

... so when calling all of them, they get recalculated multiple times.

By doing something so obviously avoidable, there must be a reason. Somewhere.

Lazy loading, generally, suggests

the operation is time, CPU, whatever consuming to some degree
doing it all at once certainly is an issue
instantiation and/or re-calculation are not intrinsically inter-dependent.
angle is being mutated because there is no logic avoiding re-calc on every call.
an object is in a valid state with all, some, or none of the calculations done.

Client doesn't need guard code all over the place
Client does not have to know any particular call order.

If the client must know call order & all the involved properties to do it, why is Angle1 even a class?

Doing it all in the constructor suggests

This object is immutable
I'm guaranteeing valid state.
angle is not mutated
client code will be just that much cleaner, easier

  however, still prefer the first code. It is cleaner. Easier, for me, to read.

You should write code that is "cleaner, and easier" to use.

Conclusion: Angle2 is better.


Loop over whole array starting from any index I recently had to repeatedly iterate over an array but increment the index to start from each time. Since this is quite cumbersome in comparison to a normal for loop I made a small template function that does this for me:

#include <algorithm>
#include <iostream>

template<typename TSize, typename TFunc>
void for_start(const TSize start, const TSize size, TFunc&& fn)
{
    for(TSize c{}, i{c + start}; c < size; ++c, ++i)
    {
        if(i >= size)
            i = 0;
        fn(i);
    }
}

template<typename TRAIter, typename TFunc>
void for_start(TRAIter start, TRAIter begin, TRAIter end, TFunc&& fn)
{
    for(TRAIter c{begin}, i{c + std::distance(begin, start)}; c != end; ++i, ++c)
    {
        if(i == end)
            i = begin;

        fn(i);
    }
}

// usage
int main()
{
    const unsigned ARR_SIZE{10};
    int arr[ARR_SIZE]{
        0, 1, 2, 3, 4, 5, 6, 7, 8, 9
    };

    for_start(5u, ARR_SIZE, [arr](unsigned i)
    {
        std::cout << arr[i] << ' ';
    });
    std::cout << '\n';

    for_start(std::begin(arr) + 5, std::begin(arr), std::end(arr), [](const int* i)
    {
        std::cout << *i << ' ';
    });
}

// prints:
// 5 6 7 8 9 0 1 2 3 4
// 5 6 7 8 9 0 1 2 3 4

I'd like to get some feedback on things like usability (function name, order of parameters) and reuseability. Note that I'd like to use this function with signed & unsigned integers.

There is no reason to initialize i as {c + std::distance(begin, start)}. Upon such initialization you are guaranteed that i == start. A much simpler i{start} suffices.
Testing for i == size, or i == end inside the loop, as well as tracking two iterators, feels wasteful. Consider splitting the loop into two, e.g:

    for (i = start; i != end; ++i) {
        fn(i);
    }

    for (i = begin; i != start; ++i) {
        fn(i);
    }

I am not sure that for_start is a good name. I am also not sure what name would be good. iterate_rotated perhaps?


Script for saving top wallpapers from wallbase.cc into a directory I am newish to Python and I am looking for some extra eyes on a script I wrote. I wrote this script to learn more about web-scraping and using requests and Beautiful Soup. I use it to connect to the website wallbase.cc and save the top 32 wallpapers from the last 3 days into a directory where I later cycle through displaying them on my desktop.

I'm really looking for some best practices suggestions, whatever you think is appropriate.  I'm open to any constructive criticism.

#!/usr/bin/env python

import os
import requests
import sys
import urllib.request
from bs4 import BeautifulSoup
import hashlib

def get_pic(number_imgs, timespan):
    """Returns a list of the specified number of top images over the specified
    timeframe from wallbase.cc
    """
    url = "http://wallbase.cc/toplist"
    opts = {
        'section':'wallpapers', 'q':'', 'res_opt':'gteq',
        'res':'1920x1080', 'aspect':'1.77', 'purity':'100',
        'board':'21', 'thpp':number_imgs, 'ts':timespan
    }
    htmltext = requests.get(url, params = opts)
    page_urls = []
    img_urls = []
    soup = BeautifulSoup(htmltext.content)
    results = soup.findAll("a")

    for r in results:
        if "http://wallbase.cc/wallpaper/" in r['href']:
            page_urls.append(r['href'])

    for p in page_urls:
        wp_page = requests.get(p)
        wp_soup = BeautifulSoup(wp_page.content)
        wp_results = wp_soup.findAll("img")
        for res in wp_results:
            if "http://wallpapers.wallbase.cc/" in res['src']:
                img_urls.append(res['src'])

    return img_urls

def save_pic(url):
    """Saves a file to disk when given a URL"""
    save_path = sys.argv[1]
    hs = hashlib.md5(url.encode('UTF-8')).hexdigest()
    file_ext = url.split(".")[-1]
    to_save = (save_path + hs + "." + file_ext)
    if to_save != "":
        if os.path.isfile(to_save):
            print(hs + "." + str(file_ext) + "\texists, skipping...")
        else:
            print(hs + "." + str(file_ext) + "\tdownloading...")
            urllib.request.urlretrieve(url, to_save)

if __name__ == "__main__":
    for img in get_pic(32, "3d"):
        save_pic(img)

Your code is nice, well documented, properly formatted and easy to understand so I won't have to much to say.

You can have a read at PEP 8 as your code is not exactly perfect for details like spacing (you should have whitespaces after : in your dicts, you shouldn't have trailing whitespaces at the end of the line, etc). You'll find cool tools to check stuff for you automatically : pep8, pylint, pyflakes, pychecker.
In save_pic, there is no way to_save != "" can be false. Indeed, because of the way you build to_save, I know for sure that it will contain a "." making it a non-empty string. If you want to be sure that the property keeps being True, you can use assert like this : assert to_save != "". Otherwise, you can just get rid of the test.
The fact that you are calling str() on file_ext in save_pic is a bit confusing : if this wasn't to behave like a string in the first place, I assume to_save = (save_path + hs + "." + file_ext) would fail.
save_pic as a function probably shouldn't rely on sys.argv[1] : I think it would be much better if this information was to be provided as a parameter. Also, to join filename, you can use os.path.join().
in save_pic (again), I think you should store the value of hs + "." + file_ext in a variable instead of repeating this in 3 different places. Also, you do not need parenthesis when assigning to to_save.
in get_pic, you could use list comprehension to define page_urls : page_urls = [r['href'] for r in soup.findAll("a") if "http://wallbase.cc/wallpaper/" in r['href']].


Symfony controller that registers a user via HTTP-POST This is the code I have written for one of my API routes. I feel like my controller is too big and the function does too much. What can I do to make my adhere to SOLID principles and be more maintainable/readable?

/**
 * @Route("/registration", name="registration")
 * @Method({"POST"})
 * @throws UniqueConstraintViolationException
 * @param Request $request
 * @return JsonResponse
 */
public function registrationAction(Request $request, UserPasswordEncoderInterface $encoder)
{
    // Build base response
    $response = [
        "success"   => false,
        "message"   => "",
        "user_id"   => null,
        "errors"    => []
    ];

    // Put blanks for keys not present in the request
    // This is so we can validate this using Symfony's validator class
    foreach(self::REQUIRED_KEYS as $key) {
        if (!array_key_exists($key, $request->request->all())) {
            $request->request->set($key, "");
        }
    }

    // Get validator and build array of constraints
    $validator = Validation::createValidator();
    $constraint = new Assert\Collection([
        "username"  => new Assert\NotBlank(['message' => "Username is required."]),
        "email"     => [
            new Assert\Email(["message" => "Email must be a valid email address."]),
            new Assert\NotBlank(["message" => "Email is required."])
        ],
        "address1"  => new Assert\NotBlank(['message' => "Address1 is required."]),
        "city"      => new Assert\NotBlank(['message' => "City is required."]),
        "state"     => new Assert\NotBlank(['message' => "State is required."]),
        "zip"       => new Assert\NotBlank(['message' => "Zip is required."]),
    ]);
    $constraint->allowExtraFields = true;

    $errors = $validator->validate($request->request->all(), $constraint);

    // If there are errors, return the errors as a response
    // If there are no errors, register the user and return the ID
    if (count($errors) > 0) {
        foreach($errors as $e) { $response['errors'][] = $e->getMessage(); }
        $response['message'] = "Submitted user failed validation.";
    } else {
        $user = new User($request->request->all());
        $encodedPassword = $encoder->encodePassword($user, 'password');
        $user->setPassword($encodedPassword);

        $em = $this->getDoctrine()->getManager();
        try {
            $user->setEnabled(true);
            $em->persist($user);
            $em->flush();
            $response['success'] = true;
            $response['user_id'] = $user->getId();
        } catch (UniqueConstraintViolationException $e) {
            preg_match('/(?<=Duplicate entry \')[^\']*/', $e->getMessage(), $matches);
            $response['message'] = "Unique constraint violation exception";
            $response['errors'][] = sprintf('%s already exists in the database.', $matches[0]);
        }
    }

    return new JsonResponse($response);
}

My initial hunch is that I should put this into a service and break it up. But it seems like I would just be copying and pasting this code into class.

I feel like my controller is too big and the function does too much.

I think you're right there. Your validation rules shouldn't be in the controller, but in separate classes. That makes re-using them much easier. I

  My initial hunch is that I should put this into a service and break it
  up. But it seems like I would just be copying and pasting this code
  into class.

You're right here. Just moving code from your controller to a service makes no sense.

Why aren't you using Symfony Forms? It does exactly the job you're doing here: take the Request and some rules and tell you if the request matches your validation rules.

You can create an object (a Doctrine entity or a Data Transfer Object, DTO), add the constraints to it by using annotations.

See this to examples from https://stovepipe.systems/post/avoiding-entities-in-forms:

Your DTO:

class ChangeUsernameData
{
    /**
     * @Some\Custom\Assert\Unique(entity="App\Entity\Authentication", field="username")
     * @Assert\Length(5, 16)
     **/
    public $newUsername;
}

Your controller:

$changeUsername = new ChangeUsernameData();
    $form = $this->formFactory->create(ChangeUsernameType::class, $changeUsername);
    $form->handleRequest($request);

    if ($form->isSubmitted() && $form->isValid()) {
        $authentication->setUsername($changeUsername->newUsername);
        $this->em->flush($authentication);

        // redirect
    }


A more readable InStr: StringContains Consider the following:

If myString = "abc" Or myString = "def" [...] Or myString = "xyz" Then

In C# when myString == "abc" the rest of the conditions aren't evaluated. But because of how VB works, the entire expression needs to be evaluated, even if a match is found with the first comparison.

Even worse:

If InStr(1, myString, "foo") > 0 Or InStr(1, myString, "bar") > 0 [...] Then

I hate to see these things in code I work with. So I came up with these functions a while ago, been using them all over the place, was wondering if anything could be done to make them even better:

StringContains is used like If StringContains("this is a sample string", "string"):

Public Function StringContains(string_source, find_text, Optional ByVal caseSensitive As Boolean = False) As Boolean

    'String-typed local copies of passed parameter values:
    Dim find As String, src As String
    find = CStr(find_text)
    src = CStr(string_source)

    If caseSensitive Then
        StringContains = (InStr(1, src, find, vbBinaryCompare) <> 0)
    Else
        StringContains = (InStr(1, src, find, vbTextCompare) <> 0)
    End If

End Function

StringContainsAny works in a very similar way, but allows specifying any number of parameters so it's used like If StringContainsAny("this is a sample string", false, "foo", "bar", string"):

Public Function StringContainsAny(string_source, ByVal caseSensitive As Boolean, ParamArray find_strings()) As Boolean

    'String-typed local copies of passed parameter values:
    Dim find As String, src As String, i As Integer, found As Boolean
    src = CStr(string_source)

    For i = LBound(find_strings) To UBound(find_strings)
        find = CStr(find_strings(i))
        If caseSensitive Then
            found = (InStr(1, src, find, vbBinaryCompare) <> 0)
        Else
            found = (InStr(1, src, find, vbTextCompare) <> 0)
        End If
        If found Then Exit For
    Next

    StringContainsAny = found

End Function

StringMatchesAny will return True if any of the passed parameters exactly matches (case-sensitive) the string_source:

Public Function StringMatchesAny(string_source, ParamArray find_strings()) As Boolean

    'String-typed local copies of passed parameter values:
    Dim find As String, src As String, i As Integer, found As Boolean
    src = CStr(string_source)

    For i = LBound(find_strings) To UBound(find_strings)
        find = CStr(find_strings(i))
        found = (src = find)
        If found Then Exit For
    Next

    StringMatchesAny = found

End Function

My 2 cents,

the first function seems fine, you could make it a little DRYer by just setting the compareMethod in your if statement and then have only 1 complicated line of logic. And if you are doing that, you might as well put the Cstr's there.

Public Function StringContains(haystack, needle, Optional ByVal caseSensitive As Boolean = False) As Boolean

   Dim compareMethod As Integer

    If caseSensitive Then
        compareMethod = vbBinaryCompare
    Else
        compareMethod = vbTextCompare
    End If
    'Have you thought about Null?
    StringContains = (InStr(1, CStr(haystack), CStr(needle), compareMethod) <> 0)

End Function

Notice as well that I love the idea of searching for needles in haystacks, I stole that from PHP.

For StringContainsAny, you are not using the code you wrote for StringContains, you repeat it. If you were to re-use the first function, you could do this:

Public Function StringContainsAny(haystack, ByVal caseSensitive As Boolean, ParamArray needles()) As Boolean

    Dim i As Integer

    For i = LBound(needles) To UBound(needles)
        If StringContains(CStr(haystack), CStr(needles(i)), caseSensitive) Then
          StringContainsAny = True
          Exit Function
        End If
    Next

    StringContainsAny = False 'Not really necessary, default is False..

End Function

For the last one I wanted to you consider passing values that you will convert as ByVal, since you are going to make a copy anyway of that variable.

Public Function StringMatchesAny(ByVal string_source, ParamArray potential_matches()) As Boolean

  string_source = CStr(string_source)

  ... 'That code taught me a new trick ;)

End Function


How can I avoid unchecked cast warning in my generic recursive Iterator? It's somewhat odd that Java's collection framework has no iterator for recursive data structures.  Since I needed something like this, I wrote my own. First off, I need recursive elements:

public interface RecursiveElement<T>
{
    public Iterator<T> getChildrenIterator();
}

And then an Iterator:

public class RecursiveIterator<T> implements Iterator<T>
{
    private Deque<Iterator<T>> stack;
    private Iterator<T> currentStackItem;

    /**
     * Creates a new instance
     *
     * @param root
     *            all children of this node are iterated. The root node itself
     *            is not returned
     * @throws NullPointerException
     *             if root is null
     */
    public RecursiveIterator(final RecursiveElement<T> root)
    {
        if (root == null)
            throw new NullPointerException(
                    "root argument to this iterator must not be null");
        stack = new LinkedList<Iterator<T>>();
        currentStackItem = root.getChildrenIterator();
    }

    @Override
    public boolean hasNext()
    {
        return currentStackItem != null;
    }

    @Override
    public T next()
    {
        final T result = currentStackItem.next();
        if (result instanceof RecursiveElement)
        {
            stack.addLast(currentStackItem);
             // Here is the warning:
            currentStackItem = ((RecursiveElement<T>)result).getChildrenIterator();
        }
        while (currentStackItem != null && !currentStackItem.hasNext())
            currentStackItem = stack.pollLast();
        return result;
    }

    @Override
    public void remove()
    {
        currentStackItem.remove();
    }
}

That code works very well, but I do get a warning from the compiler in the next() method in the line I marked. It is clear to me why this warning occurs, but I have not come up with any solution on how to solve the problem without this warning (save suppressing the warning). Any ideas?

I don't think you can do anything about this. You have to cast here, and in the process you lose all information about the type parameter: The compiler can't know that if you have a RecursiveElement, it's always a RecursiveElement<T>, and "thanks" to type erasure it can't check the runtime type.


Depth First Search and Breadth First Search in C++ I am trying to learn DFS and BFS. However, I just want to make sure that I am not doing anything wrong. Would you please determine if the BFS and DFS functions are correct?

#include <iostream>
#include <vector>
#include <queue>
#include <stack>
using namespace std;

class Graph
{
private:
    int num_of_vertices;
    vector<int> *Adj;
public:
    Graph(int V);
    void addEdge(int from, int to);
    void BFS(int Start);
    void DFS(int Start);
};

Graph::Graph(int V)
{
    this->num_of_vertices = V;
    Adj = new vector<int>[V];
}
void Graph::addEdge(int from, int to)
{
    Adj[from].push_back(to);
}

void Graph::BFS(int Start)
{
    bool* visited = new bool[this->num_of_vertices]();
    queue<int> queue;
    queue.push(Start);
    vector<int>::iterator i;
    cout << "BFS: ";
    while(!queue.empty())
    {
        Start = queue.front();
        cout << Start << " ";
        visited[Start] = true;
        queue.pop();
        for (i = Adj[Start].begin(); i != Adj[Start].end(); i++)
        {
            if (!visited[*i])
                queue.push(*i);
        }
    }
    cout << endl;
}

void Graph::DFS(int Start)
{
    bool* visited = new bool[this->num_of_vertices]();
    stack<int> stack;
    stack.push(Start);
    vector<int>::iterator i;
    cout << "DFS: ";
    while(!stack.empty())
    {
        int top = stack.top();
        cout << top << " ";
        stack.pop();
        visited[top] = true;
        for(i = Adj[top].begin(); i != Adj[top].end(); i++)
        {
            if (!visited[*i])
                stack.push(*i);
        }
    }
    cout << endl;
}

The implementation of the BFS and DFS algorithms themselves seems correct(but it is a good practice to write unit-tests to make sure that it works as intended, so you should do it). However, your code in general is not correct. It definitely leaks memory:

In the constructor, Adj is allocated: Adj = new vector<int>[V];. However, it is never deleted. You can do it in a destructor.
The same is true for the visited array(you should delete it at the end of the DFS and BFS member-functions).

Actually, there is a much easier way to deal with this issue: do not use pointers and dynamic memory allocation. I do not see any point in visited being allocated dynamically. Using an std::vector is much better(std::vector<bool> visited(num_of_vertices)). The same holds true for the Adj member-variable.

One more thing: you should try to keep the scope of variables as narrow as possible. There is no need to declare the vector<int>::iterator i at the beginning of the function. You can do it just inside the for loop where it is used.

The last thing: using namespace std; is a bad practice. It pollutes the global namespace.


Shuffle a string in a repeatable way The following function is designed to accept a string of characters and an integer.  The function returns a string that's a reordered version of the original string.  The reordering process is controlled by the second variable.  If the function is called again with the same string and a different number, a different string will be returned.
However, if the string and the integer do not change, repeat calls to this function will always return the same string.  as the integer changes, the returned strings should not appear to be similar.  I have tested this function on the following string, with integer values between 0 and 40000 without finding a duplicate returned string.

"1234567890abcdefghijklmnopqrstuvwxyz"

Are there obvious improvements that can be made?
I am also curious about this function's limitations.
So far, it seems to perform rather well.

function shuffleString($inputString, $seed = 2000){

    $strLength = strlen($inputString);
    $newString = "";
    $factor = 3 + (int)($seed / $strLength);

    while($strLength > 0){

        $seed += $factor;

        if($seed >= $strLength){
            $seed = $seed % $strLength;
        }

        $newString .= $inputString[$seed];

        $copyStr = "";

        $strLength --;

        if($seed != 0){
            $copyStr = substr($inputString,0,$seed);
        }

        if($seed != $strLength){
            $copyStr .= substr($inputString,$seed+1);
        }

        $inputString = $copyStr;

    }

    return $newString.$inputString;

}

Example:

echo shuffleString("1234567890abcdefghijklmnopqrstuvwxyz", 1234), '
';
echo shuffleString("1234567890abcdefghijklmnopqrstuvwxyz", 1234), '
';
echo shuffleString("1234567890abcdefghijklmnopqrstuvwxyz", 1234), '
';
echo shuffleString("1234567890abcdefghijklmnopqrstuvwxyz", 1234), '
';
echo shuffleString("1234567890abcdefghijklmnopqrstuvwxyz", 4567), '
';
echo shuffleString("1234567890abcdefghijklmnopqrstuvwxyz", 4567), '
';
echo shuffleString("1234567890abcdefghijklmnopqrstuvwxyz", 4567), '
';
echo shuffleString("1234567890abcdefghijklmnopqrstuvwxyz", 4567), '
';

returns

beint19kw0q7sf4xrpv3dyjgl2ozh58m6cau
beint19kw0q7sf4xrpv3dyjgl2ozh58m6cau
beint19kw0q7sf4xrpv3dyjgl2ozh58m6cau
beint19kw0q7sf4xrpv3dyjgl2ozh58m6cau
g6yvx5fujbaitkdm2s3oqcnl074hr8wp19ze
g6yvx5fujbaitkdm2s3oqcnl074hr8wp19ze
g6yvx5fujbaitkdm2s3oqcnl074hr8wp19ze
g6yvx5fujbaitkdm2s3oqcnl074hr8wp19ze

also:

for($i=0; $i<5; $i++){
    $dataStr = "1234567890abcdefghijklmnopqrstuvwxyz";
    for($j=0; $j<2000; $j++){
        $dataStr = shuffleString($dataStr, ord($dataStr[0]));
    }
    echo $dataStr . '
';
}

returns:

gdwy63zmnjxharpovk50qulf2cei8t174bs9
gdwy63zmnjxharpovk50qulf2cei8t174bs9
gdwy63zmnjxharpovk50qulf2cei8t174bs9
gdwy63zmnjxharpovk50qulf2cei8t174bs9
gdwy63zmnjxharpovk50qulf2cei8t174bs9

Why does it need to be this complicated? PHP has builtins for changing the seed of the random number generator, and for shuffling strings.

function shuffleString($input, $seed = 2000) {
    srand($seed); // set the random generator seed
    $shuffled = str_shuffle($input);
    srand(); // to ensure that the seed is reset
    return $shuffled;
}

This might be less efficient (I haven't benchmarked them) but it's simpler and more readable. Sometimes it's fun to reinvent the wheel, but there are usually shortcuts that can be taken.


Attempt at a sorting algorithm I've been playing around and reading up on some sorting algorithms, and I've decided to try writing my own.  It turned out to be quite fast (compared to the others shown in the image below).

I'm very curious to know a little bit about it:

Does my algorithm already have a name?
What are the downsides to it (other than it being really stupid to sort { 900000, 1 } with it)?
Why is it not a good solution in general?
Any suggestions for optimisation?

static int[] MySort(int[] input)
{
    int minValue = input.Min() - 1;
    int[] tempArr = new int[input.Max() - minValue];
    //{1,3,3,5}   becomes:
    //{1,0,2,0,1} one 1's, two 3's, one 5's.
    for (int i = 0; i < input.Length; i++)
        tempArr[input[i] - minValue - 1] += 1;
    //AddNumbers to old array. and add minValue.

    int g = 0;
    for (int j = 0; j < tempArr.Length; j++)
    {
        if (tempArr[j] != 0)
        {
            for (int i = 0; i < tempArr[j]; i++)
            {
                input[g] = j + minValue + 1;
                g++;
            }
        }
    }
    return input;
}

Performance screenshot:

Does my algorithm already have a name?

It's called counting sort.

  What are the downsides to it? (Other than it is really stupid to sort { 900000, 1 } with it.)

That's exactly the major disadvantage of the algorithm: it doesn't work well if the range of values is large.

  Why is it not a good solution in general?

Because it can sort only integers. You can't use it to sort floating point numbers, strings or any other type that doesn't behave as an integer.

  Any suggestions for optimization?

The condition if (tempArr[j] != 0) is completely useless. If you leave it out, the code will work exactly the same.


CountdownLatch Thread-Safety Check I've written this class as an exercise for a synchronization construct to be used across multiple threads.

The intent is to have worker threads Increment() it, do some work, then finally Decrement()it.

Meanwhile, when a master thread decides to shut down the program, it signals the threads using a CancellationToken and Wait()s on this structure until all threads have ceased (signalled by _number hitting zero).

I am depending mainly on the fact that Interlocked operations imply full memory fences. I am concerned with the read on the Wait() method, see the comment there.

public class CountdownLatch
{
    int _number;
    readonly ManualResetEventSlim _event;

    public CountdownLatch()
    {
        this._event = new ManualResetEventSlim();
    }

    public void Increment()
    {
        Interlocked.Increment(ref this._number);
    }

    public void Decrement()
    {
        int currentNumber;
        bool firstAttempt = true;

        do
        {
            currentNumber = this._number;
            if (!firstAttempt)
            {
                var spinWait = new SpinWait();
                spinWait.SpinOnce();
            }
            else
            {
                firstAttempt = false;
            }

            if (currentNumber == 0)
            {
                throw new InvalidOperationException("Attempt to decrement past zero");
            }
        } while (Interlocked.CompareExchange(ref this._number, currentNumber - 1, currentNumber) != currentNumber);

        if (currentNumber == 1)
        {
            this._event.Set();
        }
    }

    public bool Wait(TimeSpan timeout)
    {
        // Should this be a volatile read: Volatile.Read(ref this._number) ?
        // AFAICT the memory barrier here would prevent instruction reordering
        // when the order of reads/writes matter, but here it doesn't. I just care
        // to have the latest value of this._number across all CPUs/cores.
        // Or does volatile read in C# specifically do both ?
        if (this._number == 0)
        {
            return true;
        }

        return this._event.Wait(timeout);
    }
}

The Good
I don't write multithreaded code very often, so other reviewers will probably have other comments (read: I'm a multithreading beginner myself), but I like what I'm seeing.

Interlocked.Increment(ref this._number);

I don't see anything that could go wrong with this. This is how I've read a thread-safe increment should be done, using the Interlocked class.

while (Interlocked.CompareExchange(ref this._number, currentNumber - 1, currentNumber) != currentNumber)

Looks good to me. currentNumber belongs to the thread that's running this, so no thread can mess with that currentNumber while currentNumber - 1 or CompareExchange is being evaluated, and again the Interlocked class at play has my full trust.

The Bad
Ignorance is bliss, nothing in sight.

The Ugly
The very first thing that jumped at me, is the abundance of redundant this qualifiers. They're not needed, you have underscores:

int _number;
readonly ManualResetEventSlim _event;

I like the underscore prefix, because it allows me to have a _number field and a number parameter or local variable, for example:
public CountdownLatch(ManualResetEventSlim event)
{
    _event = event;
}

The way I read currentNumber, "current" stands for "current thread's working copy of _number". I find the name number conveys that more succinctly.


php function for gathering includes...is there a better way than this? I am building a dynamic site. All data will be served through an index.php in my webroot, using jquery to update the content. In webroot, I have a folder 'i/' which contains the files I want to include. The main include files contain code which will be used on every page: globals.php, globals.css, globals.js, etc. plus library files, such as jquery.js. Then there are secondary includes that are used depending on the section or topic that has been loaded: topic1.php, topic1.css, topic1.js, etc.

I have written the following php function to call all the files that need to be declared in the head (stylesheets and javascript for now, but wanted to make it easy to add more filetypes in the future), allowing globals.php to be included before the html tag (and will probably include topics.php from inside globals.php). However, I'm not really sure if this is a very efficient function, or if there is a better way to do what I'm trying to:

function getIncludes($topic){
    $type=array("css","js");
    $library=array("js"=>array("jquery"));
    $scope=array("globals",$topic);
    foreach($type as $ext){
        $includes=array();
        if(array_key_exists($ext, $library)){
            foreach ($library[$ext] as $file){
                array_push($includes, $file);
            }
        }
        foreach($scope as $file){
            array_push($includes, $file);
        }
        foreach($includes as $file){
            if(file_exists('i/'.$file.'.'.$ext)){
                if($ext=="css"){
                    echo '<link rel="stylesheet" type="text/css" href="i/'.$file.'.'.$ext.'" />';
                }
                if($ext=="js"){
                    echo '<script type="text/javascript" src="i/'.$file.'.'.$ext.'"></script>';
                }
            }
        }
    }
}

Is this a good way of achieving my goal, or can I do better?

Don't make your directory structure so vague. Coming into this with no knowledge of your system how am I supposed to know what the "i" directory is? Just call it includes. Also, it is usually a good idea to separate your files based on their types so that they are easier to find. There are many common project directory structures out there. For instance I'm using something similar to Zend's. It looks similar to this.

/root
    /application
        /configs
        /models
        /views
        /controllers
    /data
        /cache
        /logs
    /public
        /js
        /css
        /images

I hope those file names don't indicate that you are actually using globals... There are much better ways to get "global" scope variables without ever using global. Sessions for instance. Globals, in my opinion, should eventually be deprecated. They are an old feature that has been proven faulty and replaced.

array_push() is another old function. Though there is nothing wrong with it, the preferred way to do it now is like so.

$includes[] = $file;

What you are trying to do is usually done with templates rather than PHP. For instance, you'd have a single HTML file with these includes in it already then dynamically change the contents with PHP. For those files that use the $topic variable, you would just perform a check to make sure it exists before including it via PHP. So a very basic template might look like this.

<html>
<head>
<title></title>
<script type="text/javascript" src="i/globals.js"></script>
<link rel="stylesheet" type="text/css" href="i/globals.css" />

<!-- SPECIFIC INCLUDES -->

<?php if( is_file( "i/$topic.js" ) : ?>
<script type="text/javascript" src="i/<?php echo $topic.js; ?>"></script>
<?php endif; ?>

<?php if( is_file( "i/$topic.css" ) : ?>
<link rel="stylesheet" type="text/css" href="i/<?php echo $topic.css; ?>" />
<?php endif; ?>

<!-- SPECIFIC INCLUDES -->

</head>
<body>
<?php if( is_file( "i/$topic.php" ) ) { include "i/$topic.php"; } ?>
</body>
</html>


Optimizing this method (HashSet.RemoveWhere performance issue) I have the following method:

public static HashSet<PathDetails> GetDeletedPaths(HashSet<PathDetails> old, HashSet<PathDetails> current)
{
    var deleted = new HashSet<PathDetails>(old);
    deleted.RemoveWhere(x => current.Any(y => y.Path == x.Path));
    return deleted;
}

PathDetails is a struct that, among other things, contains Path of type String. This method takes the longest in my application and I was wondering if there were any good ways of improving its performance. These hash sets are used elsewhere in a more hashset-friendly way, so the method must take in two sets and return a set.

Instead of using the RemoveWhere() method together with the Any() method you should consider to just use ExceptWith() method.

public static HashSet<PathDetails> GetDeletedPaths(HashSet<PathDetails> old, HashSet<PathDetails> current)
{
    var deleted = new HashSet<PathDetails>(old);
    deleted.ExceptWith(current);
    return deleted;
}

Your approach calls for each item in deleted the Any() of current whereas ExceptWith() just calls for each item in current the Remove() method of deleted.


Optimal way of skipping certain values in a for loop What would be the fastest way of skipping certain values in a for loop? This is what I intend to do in its most basic form, with about 20 more keys to omit.

for (var key in arr ) {
    if ( key != 'ContentType' && key != 'FileRef' ) {
        arrayPushUnknown( columns, formatTitle( key ), true, key )
    }
}

Tests across various browser has shown a reverse while loop is fastest.
Accessing array.length is slower than storing it in a variable to iterate on.
You can just "push" to an array by storing the value in the last element which is conveniently at .length
A switch may not be much faster in this case (though it can be with other data) than a bunch of &&s and ||s, but its much easier to read and the fallthrough characteristic allows you to write like code in groupings.  In this case we only have 2 groups: do nothing (break;) and the default "push"

For example:

var i=arr.length;
var newarr=[];
while (i--){
  switch (arr[i].key){
    case 'ContentType' :
    case 'FileRef' :
    case (other) :
    case (cases) :
    case (here) :
    break;
    default: newarr[newarr.length]=arr[i];
  }
}

On a few newer browsers (Chrome for one), push is faster than direct assignment.  Note that this will be reversed from the original array, but if you need to keep them in the same order using for(var i=0,len=arr.length;i<len;i++), I caution you not to be tempted to use newarr[i]=arr[i] or you will get empty slots where your keys match your blacklist.


Finding weight on various different planets I'm hoping I could get some help in understanding as well as optimizing code! I've been working on a few exercises on some online resources, and I decided to try and take it to the next level and start using methods.

import java.util.Scanner;

public class SpaceBoxing
{
    static Scanner keyboard = new Scanner(System.in);

    public static void main(String[] args)
    {
        double weight = getEarthWeight();
        int choice = getPlanetChoice();
        double realWeight = computeEffectiveWeight(weight, choice);
        System.out.println("Your weight would be " + realWeight + " on that planet. ");
    }

    static double getEarthWeight()
    {
        System.out.println("Please enter your current Earth weight: ");
        return keyboard.nextDouble();
    }

    static int getPlanetChoice()
    {
        System.out.println("I have information on the following planets: ");
        System.out.println("1. Venus");
        System.out.println("2. Mars");
        System.out.println("3. Jupiter");
        System.out.println("4. Saturn");
        System.out.println("5. Uranus");
        System.out.println("6. Neptune");
        return keyboard.nextInt();
    }

    static double computeEffectiveWeight(double weight, int choice)
        {
            double realWeight = 0;
            if(choice == 1)
                realWeight = (weight * 0.78);

            else if(choice == 2)
                realWeight = (weight * 0.39);

            else if(choice == 3)
                realWeight = (weight * 2.65);

            else if(choice == 4)
                realWeight = (weight * 1.17);

            else if(choice == 5)
                realWeight = (weight * 1.05);

            else if(choice == 6)
                realWeight = (weight * 1.23);

            return realWeight;
        }
}

I've been working on calling methods and passing parameters between them, and so far I think I've got 2/3 of the methods completed except for the final one. Just a quick run down, I'm using the first two methods to return a typed input, and saving them into weight and choice. And the final method will take those two variables and computes a final result, which I hope to be able to print out. I'm not sure what to return the input values as, because I'm trying to pull the parameter and use it in another... instead of passing it in.

I'm not sure, but what am I missing? Are there some style things that I should do? Is there a better way in doing this (still with methods though)?

it's definitely WAY above what I've learned so far

So I try...

An answer for beginners

The code is mostly alright, sure, using an enum would help and so would Java 8 features. But let's state the simple things:

public class SpaceBoxing
{

The opening brace should be written on the same line and separated by a single space.

static Scanner keyboard = new Scanner(System.in);

You're creating a Scanner, which is Closeable and ideally should be closed. Usually, failing to do so means leaking resources and can make your program crash (e.g., you can't open a file as there are already 1000 open file descriptors).

But you're lucky: Your Scanner only wraps the underlying InputStream System.in and itself uses no system resources. So let's forget it for now.

static double computeEffectiveWeight(double weight, int choice)
    {
        double realWeight = 0;

Be consistent. Is the weight "real" or "effective"?

        if(choice == 1)

Space after if (to distinguish it visually from a method call).

You'd better always use braces, even around a single statement as it's less error-prone.

            realWeight = (weight * 0.78);

Needless parentheses. And also a needless variable. It'd clearer to write

        if (choice == 1) {
            return weight * 0.78;

        } else if (choice == 2) {
            return weight * 0.39;

        ...

        } else {
            return 0;
        }

But you probably should throw an IllegalArgumentException instead of returning 0 for an illegal argument.

Actually, this is a clear case for a switch:

       switch (choice) {
           case 1: return weight * 0.78;
           case 2: return weight * 0.39;
           ...
           default: return 0; // or better throw
       }

  I'll definitely look into it more, but I got the code working, and I guess "why break it if it ain't broke"

That's a good rule in general, but after applying it consequently for one month, you'll wish you never wrote a single line. ;) Don't hesitate to change code if it can get clearer and simpler. Learn writing tests so you don't have to be afraid of change.

Simple enum usage

As the next step, I'd go for enum Planet exactly like h.j.k. did. Modify getPlanetChoice to return it like

static Planet getPlanetChoice() {
    Planet[] planets = Planet.values();
    while (true) {
        System.out.println("I have information on the following planets: ");
        System.out.println("1. Venus");
        System.out.println("2. Mars");
        System.out.println("3. Jupiter");
        System.out.println("4. Saturn");
        System.out.println("5. Uranus");
        System.out.println("6. Neptune");
        int index = keyboard.nextInt() - 1;
        if (0 <= index && index < planets.length) {
            return planets[index];
        }
        System.out.println("\nPlease try again.\n");
    }
}

and then get the needed information out of the returned Planet.


Deserialize with LINQ query code in PageModel or PageView Today I do something like this, where I get, select and sort some data and pass it on to the view (which loads a partial view) as a list of custom objects

Page model

public PagesSettings PageSettings { get; set; }
public List<Reportage> itemListReportages { get; set; }

public void OnGet()
{
    PageSettings = (PagesSettings)HttpContext.Items["pagesettings"];

    if (PageSettings == null)
    {
        return;
    }

    var fileContent = _fileService.GetFileContentCached(PageSettings.DataFile);

    if (fileContent?.Length > 0)
    {
        itemListReportages = (JsonConvert.DeserializeObject<List<Reportage>>(fileContent))
            .Where(l => l.Published)
            .OrderBy(l => l.Name)
            .ToList();
    }
}

Page view

@page
@model ListReportagesModel

@if (Model.PageSettings?.Data?.Count > 1 && Model.PageSettings.Data[1].Html?.Length > 0)
{

            @if (Model.PageSettings.Data[1].Values?.Length > 0)
                {
                    @Html.Raw(String.Format(Model.PageSettings.Data[1].Html, Model.PageSettings.Data[1].Values))
                }
                else
                {
                    @Html.Raw(@Model.PageSettings.Data[1].Html)
                }

}

        <partial name="/Pages/Partial/@Model.PageSettings.PartialView" />

Partial view

@for (var i = 0; i < Model.itemListReportages.Count; i++)
{

            @Model.itemListReportages[i].Text

}

It now start to become a lot of PageModel's, so I started to wonder where the proper location to run C# code is. In above the data loading/select/sort is done in the PageModel and the rest in the view's.

I was planning to split it up like below instead, moving the select/sort from Pagemodel to the partial view, and with that reduce the amount of Pagemodel's and make it simpler to add data by simply create a new class + data file + partial view.

The question, which way is recommended (if any), perform select/sort in PageModel or in View?

Page model

public PagesSettings PageSettings { get; set; }
public string fileContent { get; set; }

public void OnGet()
{
    PageSettings = (PagesSettings)HttpContext.Items["pagesettings"];

    if (PageSettings == null)
    {
        return;
    }

    fileContent = _fileService.GetFileContentCached(PageSettings.DataFile);
}

Partial view

if (fileContent?.Length > 0)
{
    var itemListReportages = (JsonConvert.DeserializeObject<List<Reportage>>(fileContent))
        .Where(l => l.Published)
        .OrderBy(l => l.Name)
        .ToList();

    @for (var i = 0; i < Model.itemListReportages.Count; i++)
    {

                @Model.itemListReportages[i].Text

    }
}

I find you should keep the library code in the model and only parts that are relevant to views inside them.

In your case there is actually a third option. Let OnGet only handle the file. Remove OrderBy as this is a view's matter how it's displayed. Additionally initialize itemListReportages to an empty list if a file was empty. It'll save you the trouble of null checks later.

public void OnGet()
{
    PageSettings = (PagesSettings)HttpContext.Items["pagesettings"];

    if (PageSettings == null)
    {
        return;
    }

    var fileContent = _fileService.GetFileContentCached(PageSettings.DataFile);

    itemListReportages =
        fileContent?.Length > 0
            ? JsonConvert.DeserializeObject<List<Reportage>>(fileContent).Where(l => l.Published).ToList()
            : new List<Reportage>();
}

We don't see whether you initialize itemListReportages but this view could crash because you don't use the null propagation operator here: ?.Count

@for (var i = 0; i < Model.itemListReportages.Count; i++)
{

            @Model.itemListReportages[i].Text

}

With a non-null list you can turn this into a foreach loop and put OrderBy here. It'll make the code shorter too as you now have an item variable and no longer have to acces them by index like Model.itemListReportages[i]:

@foreach (var item in Model.itemListReportages.OrderBy(x => x.Name))
{

            @item.Text

}


Improving CollectionView Filter Speed I have around 350 People loading into a CollectionView and am already noticing some lag when filtering on their Name. This concerns me as we could easily reach > 1000 People quickly and I need filtering to be as quick as possible.

I'm trying to avoid calling Refresh() as I know this is expensive but I am not quite sure how to do this here. Here is my code:

C#

public PeopleViewModel()
{
    using (var context = new DashboardContext())
    {
        People = new ObservableCollection<Person>(context.People.ToList());
        SearchPeople = new ObservableCollection<SearchPerson>(
            People.Select(c => new SearchPerson
            {
                Id = c.Id,
                FullName = c.FullName.ToLower(),
                DisplayName = c.FullName
            }).ToList().OrderByDescending(x => x.Id));
    }

    SearchPeopleView = CollectionViewSource.GetDefaultView(SearchPeople) as CollectionView;
    if (SearchPeopleView != null) SearchPeopleView.Filter = ViewFilter;
}

C# Filter

private void FilterPeople()
{
    SearchPeopleView.Refresh();
}

private bool ViewFilter(object obj)
{
    var user = obj as SearchPerson;
    return user != null && user.FullName.Contains(_searchString);
}

XAML

<DataGrid Grid.Row="1" Margin="5"
            SelectedItem="{Binding SelectedSearchPerson}"
            ItemsSource="{Binding SearchPeopleView}"
            Style="{StaticResource HomeDataGrid}">
    <DataGrid.Columns>
        <DataGridTextColumn Binding="{Binding DisplayName}" Width="*"/>
    </DataGrid.Columns>
</DataGrid>

I use a DataGrid here as I couldn't get the styling I was looking for in a ListBox. I also created a minimal version of a Person, SearchPerson so I can search on fewer properties.

Reading around there are solutions but many are < 5 years old. I'm also aware that Contains() can be slow so I suppose I should be looking at a different approach to the Filter itself.

I'd appreciate any help and advice, thanks!

Take a look at ReactiveExtensions (or specifically ReactiveUI). Then use something like in your VM constructor

this.WhenAnyValue(x => x.SearchString).Throttle(TimeSpan.FromMiliseconds(50).Subscribe(_ => Fitler.Refresh())

This will wait a little between each keystroke before starting the filter.

On the other hand, checkout DerivedCollection - it can move the filtering out of the UI thread, only pushing additions and removes of single items (or calling the full reset or ReactiveList if changes are big, you can set the threshold) to UI.

Still, your solution should be fast enough. Remember that it vastly depends on your DataTemplate for your filtered items - if it is complicated, most cost will be in displaying, not in filtering - especialy that it has to be done on UI thread.


Function wrapper like std::function that uses "small buffer" allocation My Function class is similar to std::function with the small-buffer optimization. However, it always uses  a small buffer and static asserts if the required size exceeds it.

I would like a review for:

correct use of the low-level features
general class design and interoperability with standard library

Here is my code:

#include <cassert>
#include <functional>
#include <iostream>
#include <new>
#include <utility>
#include <array>

template<typename Signature>
struct Function;

template<typename R, typename ...Args>
struct Function<R(Args...)>
{
    Function() : storage()
    {}

    template<typename F>
    Function(const F& f)
    {
        static_assert(alignof(F) <= alignof(Function), "");
        static_assert(sizeof(f) <= sizeof(storage), "");

        new (storage.data()) Impl<F>(f);
    }

    Function(const Function& rhs) :
        storage(rhs.storage)
    {
        if (rhs.valid())
        {
            rhs.getImpl().clone(data());
        }
    }

    Function(Function&& rhs) noexcept :
        storage(rhs.storage)
    {
        rhs.storage = Storage();
    }

    Function& operator=(Function rhs) noexcept
    {
        std::swap(storage, rhs.storage); // not sure if safe
        return *this;
    }

    ~Function()
    {
        if (valid())
        {
            getImpl().~Base();
        }
    }

    R operator()(Args&& ...args) const
    {
        if (!valid())
        {
            throw std::bad_function_call();
        }
        return getImpl().call(std::forward<Args>(args)...);
    }

private:
    struct Base
    {
        virtual ~Base() {}
        virtual R call(Args&& ...args) const = 0;
        virtual void clone(void* where) const = 0;
    };

    template<typename F>
    struct Impl : Base
    {
        Impl(const F& f) : f(f) {}

        R call(Args&& ...args) const override final
        { return f(std::forward<Args>(args)...); }

        void clone(void* where) const override final
        { new (where) Impl<F>(*this); }

        F f;
    };

    // convenience methods
    bool valid() const
    { return storage != Storage(); }

    const void* data() const
    { return static_cast<const void*>(storage.data()); }

    void* data()
    { return static_cast<void*>(storage.data()); }

    const Base& getImpl() const
    { assert(valid()); return *static_cast<const Base*>(data()); }

    Base& getImpl()
    { assert(valid()); return *static_cast<Base*>(data()); }

    typedef std::array<long, 4> Storage; // long is probably max-align
    Storage storage;
};

int main()
{
    Function<int(int)> increment = [](int n) {
        return n + 1;
    };
    Function<int(int)> decrement = [](int n) {
        return n - 1;
    };

    std::cout << increment(3) << std::endl;
    std::cout << decrement(3) << std::endl;

    increment = std::move(decrement);
    std::cout << increment(3) << std::endl;

    // calling the moved-from decrement
    try
    {
        decrement(3);
        assert(false);
    }
    catch (std::bad_function_call& e)
    {
        std::cout << e.what() << std::endl;
    }
}

Generally, I can't see any real problems, but there are a few things that I'm going to point out:

In the assignment operator, you use std::swap(storage, rhs.storage) and declare it as noexcept. This should probably be conditionally noexcept, based on whether the swap implementation for the given array type (in this case, array<long>) is noexcept. That being said, I'd be really, really surprised if this wasn't always true.

The only other issues I can see are to do with alignment and storage size, which you've basically dealt with thanks to the two static_asserts in the constructor for Function (although they should both have a string detailing exactly what happened if either of them fails). As you're probably aware, there are precious few guarantees about function pointer sizes (especially when you start dealing with a combination of member functions, virtual functions, and multiple inheritance). 4 * sizeof(long) should be enough space to hold anything like this, but this is implementation defined; there are no guarantees.

With regards to alignment, I'm less confident. long is only guaranteed by the standard to be at least 4 bytes, and this is true even for 64-bit platforms. Since storage is actually holding the function itself, the alignment should be made more explicit, and shouldn't rely on the (potentially less stringent) alignment of long. I'd personally suggest:

alignas(std::intptr_t) Storage storage;

which (as far as I can see) should ensure storage will be aligned on the correct boundary.


Making a rectangular-shaped pattern I'd like someone to suggest a better way to create this pattern in Java which I'm sure is possible:

*********
*       *
*       *
*       *
*       *
*       *
*       *
*       *
*********

I'm working my way through a new Java book and am examining string patterns.

public static void drawRectangle() {

    // y axis
    for( int y = 0; y <= 8; y++ )
    {
        if( y == 0 || y == 8 )
        {
            System.out.print( "*********\n" );
            if( y == 8 )
            {
                // Leave loop
                break;
            }
        }

        // x axis
        for( int x = 0; x <= 8; x++ )
        {
            if( x == 0 || x == 8 )
            {
                System.out.print( "*" );
                if( x == 8 )
                {
                    System.out.println();
                }
            }
            else
            {
                System.out.print( " " );
            }
        }
    }
}

Off the top of my head, I'd say that I could replace the values in the loops with constants to work towards. For example:

final static int END_POINT = 8;

Any thoughts?

It's ok for a first iteration.

But your 2nd iteration should be drawRectangle(int width, int height). That will force you to not hard-code your numbers.
Your 3rd iteration should be noticing that your rectangle only has 2 different rows (one row fills up the width with * and the other only has * in the beginning and end).  You might want to make a method drawHorizontal to draw the top and bottom and drawEnds to draw the sides.  For example

    void drawHorizontal(int width) {
      for (int i=0 ; i<width ; i++) {
         System.out.print("*");   /* notice I'm using print() not println() */
      }
      System.out.println("");     /* notice I'm using println() */
    }

    void drawEnds(int width) {
      System.out.print("*");
      for (int i=1 ; i<width-1 ; i++) {
         System.out.print(" ");
      }
      System.out.println("*");
    }


Functor to compare two floats with tolerance What do you think about this implementation of 2 floats comparison functor considering how tolerance is introduced?

class Less
{
private:
    float m_tolerance;

public:
    Less(const float tolerance)
        : m_tolerance(tolerance)
    {

    }

    bool operator()(const float f1, const float f2) const
    {
        const bool toCloseToCompareSmaller = (std::abs(f2 - f1) < m_tolerance);
        const bool isSmaller = (f1 < f2);
        return !toCloseToCompareSmaller && isSmaller;
    }

    ~Less()
    {
    }
};

A few remarks:

Seems appropriate to add a default value of 0.0f in the constructor:

Less(const float tolerance = 0.0f)
    : m_tolerance(tolerance)
{

}

This class could most certainly be used as a base class.

For example:

class Less1 : public Less
{
public:
    Less1():Less(1.0f)
    {
        ...
    }
    ...
}

So you may as well make it suitable to serve as such:

Declare virtual ~Less()
Change private to protected where needed

The operation f1 < f2 is most likely less expensive than abs(f2 - f1).

So you may as well check it first:

bool operator()(const float f1, const float f2) const
{
    return f1 < f2 && std::abs(f2 - f1) >= m_tolerance;
}

You should add an assertion on the value of m_tolerance being non-negative.

Alternatively, you could use std::abs(m_tolerance), but it seems a bit hacky.


More DRY way of conditionally filling variables? I'm building a chart component and it behaves differently if data has been passed in or not.
Here are two ways I can think of to conditionally fill chart variables.
(data will be empty {} or have non-empty values)
// helper
function isEmptyObject(obj) {
  for (const i in obj) return false;
  return true;
}

const gridInterval = 5;

let yTopTickValue;
let yBottomTickValue;
let yTickCount;
let xTopTickValue;
let xBottomTickValue;
let xTickCount;

if (!isEmptyObject(data)) {
  yTopTickValue = Math.ceil(data.yMax / gridInterval) * gridInterval;
  yBottomTickValue = Math.floor(data.yMin / gridInterval) * gridInterval;
  yTickCount = (yTopTickValue - yBottomTickValue) / gridInterval + 1;

  xTopTickValue = Math.ceil(data.yMax / gridInterval) * gridInterval;
  xBottomTickValue = Math.floor(data.yMin / gridInterval) * gridInterval;
  xTickCount = (xTopTickValue - xBottomTickValue) / gridInterval + 1;
}

and
const gridInterval = 5;
const isDataEmpty = isEmptyObject(data);

const yTopTickValue = isDataEmpty ? null: Math.ceil(data.yMax / gridInterval) * gridInterval;
const yBottomTickValue = isDataEmpty ? null: Math.floor(data.yMin / gridInterval) * gridInterval;
const yTickCount = isDataEmpty ? null: (yTopTickValue - yBottomTickValue) / gridInterval + 1;

const xTopTickValue = isDataEmpty ? null: Math.ceil(data.yMax / gridInterval) * gridInterval;
const xBottomTickValue = isDataEmpty ? null: Math.floor(data.yMin / gridInterval) * gridInterval;
const xTickCount = isDataEmpty ? null: (xTopTickValue - xBottomTickValue) / gridInterval + 1;

Are there any other clever versions I'm missing?

Since you have a reasonable number of variables that all relate to one particular thing (chart settings), rather than having lots of standalone variables, you might consider using a single object instead. Regardless of any other issues with the code, I'd consider a single object to be slightly more appropriate than multiple similar standalone variables regardless.
Using a single object can also let you exploit a shortcut: if data doesn't exist, just use an empty object. Otherwise, type out the object literal using your formulas:
const getChartSettings = (gridInterval, data) => {
  if (isEmptyObject(data)) {
    return {};
  }
  const yTopTickValue = Math.ceil(data.yMax / gridInterval) * gridInterval;
  const yBottomTickValue = Math.floor(data.yMin / gridInterval) * gridInterval;
  const xTopTickValue = Math.ceil(data.yMax / gridInterval) * gridInterval;
  const xBottomTickValue = Math.floor(data.yMin / gridInterval) * gridInterval;
  return {
    yTopTickValue,
    yBottomTickValue,
    yTickCount : (chartSettings.yTopTickValue - chartSettings.yBottomTickValue) / gridInterval + 1,
    xTopTickValue,
    xBottomTickValue,
    xTickCount : (chartSettings.xTopTickValue - chartSettings.xBottomTickValue) / gridInterval + 1,
  };
};

Then you can call the function, reference chartSettings.yBottomTickValue, and either get a number or undefined.
YMMV on whether that's worth using or not.
It's unfortunate that the TickCounts depend on the prior calculated values - if they didn't, it could be made to look a lot simpler by just returning the single object literal instead of defining variables beforehand.
You could still return an object alone without prior defined variables by making the TickCounts getters instead, but that'd be weird-looking, I wouldn't use it:
return {
  yTopTickValue: Math.ceil(data.yMax / gridInterval) * gridInterval,
  yBottomTickValue: Math.floor(data.yMin / gridInterval) * gridInterval,
  get yTickCount() {
    return (this.yTopTickValue - this.yBottomTickValue) / gridInterval + 1;
  },
  // ...

Operator position You also might consider putting the + 1 at the beginning, rather than at the end, eg:
xTickCount : 1 + (chartSettings.xTopTickValue - chartSettings.xBottomTickValue) / gridInterval,

I think that's a bit clearer than putting it right next to the / gridInterval. You could also consider using no-mixed-operators.


Local report RDLC to PDF Is there any way to improve the performance of local report or even an alternative if not? I have been looking for a solution for quite some time but the general consensus seems to be that it's just slow.

public byte[] genReportBytes(int id, string fromm,string too,string filetype)
{
    reportDetails repD = new reportDetails();
    repD = getOneReport(id);

    LocalReport report = new LocalReport();

    if (fromm != null)
        repD.ParametersCommandLine = "@startdate=" + fromm;

    if (too != null)
        repD.ParametersCommandLine += " @enddate=" + too;

    string RDLCPath = ConfigurationManager.AppSettings["RDLCPath"];
    string ReportOutputPath = ConfigurationManager.AppSettings["ReportOutputPath"];

    string RDLCName = repD.RDLCName;
    RDLCPath += @"\" + RDLCName;
    report.ReportPath = RDLCPath;

    string sqlGet = repD.SQLOfReport;

    report.DataSources.Add(new ReportDataSource(repD.DatasetName, getReportData(sqlGet, repD.ParametersCommandLine)));

    // export to byte array

    Warning[] warnings;
    string[] streamids;
    string mimeType;
    string encoding;
    string filenameExtension;
    string deviceInf = "";
    byte[] bytes;
    string extension;

    if (filetype == "pdf")
    {
        deviceInf = "<DeviceInfo><PageHeight>8.5in</PageHeight><PageWidth>11in</PageWidth><MarginLeft>0in</MarginLeft><MarginRight>0in</MarginRight></DeviceInfo>";
        //fileName = ReportOutputPath + @"\" + repD.NameOfOutputPDF + ".PDF";
        bytes = report.Render("pdf", deviceInf, out mimeType, out encoding, out filenameExtension,
        out streamids, out warnings);
    }
    else
    {
        //fileName = ReportOutputPath + @"\" + repD.NameOfOutputPDF + ".XLS";
        bytes = report.Render(
              "Excel", null, out mimeType, out encoding,
               out extension,
              out streamids, out warnings);
    }

    return bytes;
}

Your naming conventions are hard to follow, or you're not following them; either way, you're not following the standard/recommended/established conventions.

public byte[] genReportBytes(int id, string fromm,string too,string filetype)

Public members should be PascalCase; locals and parameters should be camelCase. That would make the signature read like this:

public byte[] GenReportBytes(int id, string fromm, string too, string fileType)

getOneReport(id) would be GetOneReport(id), and getReportData(sqlGet, repD.ParametersCommandLine)) would be GetReportData(sqlGet, repD.ParametersCommandLine)), and streamids would be streamIds.

But if I had to pick only 1 name to change, it would be this one:

reportDetails repD = new reportDetails();

reportDetails is a TYPE and should be PascalCase like every single other type name in the framework. Notice how that one even gets the syntax highlighter confused?

The filetype parameter is stringly typed, and wants to be an enum. And I don't understand the doubling-up of m in fromm and o in too - if that's just to make from and to usable names, you have two options:

Prefix with a @ verbatim specifier, making them @from and @to. Seriously though, don't do that.
Give them a meaningful name; I'd match them with the parameters they're mapped to, so that would be startDate and endDate.

There are other points to add, but I have to go... will come back and edit this answer if no one else points them out by then.


ImmutableList implementation in C# I tried to implement an immutable list.

public class ImmutableList<T> : IEnumerable<T>
{
    public T Head { get; set; }
    public ImmutableList<T> Tail { get; set; }

    protected ImmutableList()
    {

    }

    public static ImmutableList<T> WithItems(IEnumerable<T> items)
    {
        if (items.Count() == 1)
        {
            return new ImmutableList<T>()
            {
                Head = items.Single(),
                Tail = new EmptyList<T>()
            };
        }

        return new ImmutableList<T>()
        {
            Head = items.First(),
            Tail = WithItems(items.Skip(1))
        };
    }

    public ImmutableList<T> Add(T item)
    {
        if (this is EmptyList<T>)
        {
            return new ImmutableList<T>() {Head = item, Tail = new EmptyList<T>()};
        }

        return new ImmutableList<T>()
        {
            Head = Head,
            Tail = Tail.Add(item)
        };
    }

    public IEnumerator<T> GetEnumerator()
    {
        if (this is EmptyList<T>) yield break;
        else
        {
            yield return Head;
            foreach (var elem in Tail)
                yield return elem;
        }
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }
}

public sealed class EmptyList<T> : ImmutableList<T>
{

}

public static class ImmutableListExtensions
{
    public static ImmutableList<T> ToImmutableList<T>(this IEnumerable<T> sequence)
    {
        return ImmutableList<T>.WithItems(sequence);
    }
}

Simple example of how to use:

private static void Main(string[] args)
{
    ImmutableList<int> list = new[] {1, 2, 3}.ToImmutableList();

    ImmutableList<int> list2 = new EmptyList<int>().Add(1).Add(2).Add(3).Add(4);

    foreach (var n in list)
    {
        Console.WriteLine(n);
    }
}

Is there a better and/or more efficient way to do this?

First of all, this class is not immutable at all. You can modify both the head and the tail at any given node in the list from calling code. Here are some quick fixes to make the list immutable:

public class ImmutableList<T> : IEnumerable<T>
{
    private static readonly ImmutableList<T> _Empty = new EmptyList<T>();

    private readonly T _Head;

    private readonly ImmutableList<T> _Tail;

    public static ImmutableList<T> Empty
    {
        get
        {
            return _Empty;
        }
    }

    public T Head
    {
        get
        {
            return this._Head;
        }
    }

    public ImmutableList<T> Tail
    {
        get
        {
            return this._Tail;
        }
    }

    protected ImmutableList(T head, ImmutableList<T> tail)
    {
        this._Head = head;
        this._Tail = tail;
    }

    public static ImmutableList<T> WithItems(IEnumerable<T> items)
    {
        return (items == null) || (!items.Any())
            ? _Empty
            : (items.Count() == 1
                ? new ImmutableList<T>(items.Single(), _Empty)
                : new ImmutableList<T>(items.First(), WithItems(items.Skip(1))));
    }

    public ImmutableList<T> Add(T item)
    {
        return this == _Empty
            ? new ImmutableList<T>(item, _Empty)
            : new ImmutableList<T>(this._Head, this._Tail.Add(item));
    }

    public IEnumerator<T> GetEnumerator()
    {
        if (this == _Empty)
        {
            yield break;
        }

        yield return this._Head;
        foreach (var elem in this._Tail)
        {
            yield return elem;
        }
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return this.GetEnumerator();
    }

    private sealed class EmptyList<TU> : ImmutableList<TU>
    {
        public EmptyList() : base(default(TU), null)
        {
        }
    }
}

The calling code needs to change ever so slightly to accommodate:

var list2 = ImmutableList<int>.Empty.Add(1).Add(2).Add(3).Add(4);

You'll also note I added a null and empty sequence check in the WithItems static method. This simply allows any null/empty sequence call to return an empty list.


Program to convert binary to hex I wrote this program to convert binary to hex, it ignores other characters between the binary digits. I would like to receive any recommendations on how to improve it.

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

const char table[] = "0123456789abcdef";

int main(int argc, char* argv[])
{
  int flip = argc - 2;
  unsigned char* ubin = argv[1];
  unsigned int nibble = 0;
  int i = 0;
  int nothing = 1;

  if((argc != 2) && (argc != 3 || strcmp(argv[2], "-f") != 0)){
    fprintf(stderr, "Usage: %s 00001111 [-f]\n", argv[0]);
    return EXIT_FAILURE;
  }

  printf("%s", "0x");

  while(*ubin){
    if((*ubin != '0') && (*ubin != '1')){
      ++ubin;
      continue;
    }

    nibble <<= 1;
    nibble |= *ubin - '0';

    if(++i == 4){
      i = 0;
      if(flip)
        nibble = ~nibble & 0xf;
      putchar(table[nibble]);
      nibble = 0;
      nothing = 0;
    }

    ++ubin;
  }

  if(nothing)
    putchar('0');

  putchar('\n');

  if(i)
    fprintf(stderr, "Error: %d bits were discarded, min unit is a nibble\n", i);

  return 0;
}

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

const char table[] = "0123456789abcdef";

This constant should be static const char table[]. Just as a general rule, so that it doesn’t conflict with other variables in other files that are also called table.

int main(int argc, char* argv[])
{
  int flip = argc - 2;
  unsigned char* ubin = argv[1];

Using argc and argv before range checking them is dangerous in general. In this case it works, but it means that ubin could be NULL here. This code looks suspicious.

  unsigned int nibble = 0;
  int i = 0;

Bad variable name i. It should rather be nbits, since it contains the number of bits in the current nibble.

  int nothing = 1;

  if((argc != 2) && (argc != 3 || strcmp(argv[2], "-f") != 0)){
    fprintf(stderr, "Usage: %s 00001111 [-f]\n", argv[0]);
    return EXIT_FAILURE;
  }

  printf("%s", "0x");

A simple printf("0x") would be clearer.

  while(*ubin){

Instead of this while loop, write a for loop: for (ubin = argv[1]; *ubin != '\0'; ubin++) {. Then you don’t need to write the ubin++ multiple times.

    if((*ubin != '0') && (*ubin != '1')){
      ++ubin;
      continue;
    }

    nibble <<= 1;
    nibble |= *ubin - '0';

    if(++i == 4){
      i = 0;
      if(flip)
        nibble = ~nibble & 0xf;

An alternative would be nibble ^= 0xf;.

      putchar(table[nibble]);
      nibble = 0;
      nothing = 0;
    }

    ++ubin;
  }

  if(nothing)
    putchar('0');

  putchar('\n');

  if(i)
    fprintf(stderr, "Error: %d bits were discarded, min unit is a nibble\n", i);

In case of an error, the return value should be EXIT_FAILURE, not 0.

  return 0;
}

But apart from all these comments, the program looks fine. You did the argument checking correctly and strictly. When your program gets more options, you should consider using the getopt library for parsing the command line. But for now, it is fine.

It is unusual to have the options after the remaining arguments; -f 00001111 is more common than 00001111 -f.

The main algorithm is clean and simple. I especially like the error handling at the end. There are too many other programs that just don’t do this.


Generate binary image using coordinate vectors I have a matrix M of size [n_rows, n_cols] and a list of x and y pixel coordinates. I use these coordinates to draw a binary image. Here is my code:
std::vector<std::vector<double>> x;
std::vector<std::vector<double>> y;

// fill x and y

std::vector<std::vector<unsigned int>> M(n_rows, std::vector<unsigned int>(n_cols));

std::vector<std::vector<double>>::const_iterator iter_row_x = x.begin();
std::vector<std::vector<double>>::const_iterator iter_row_y = y.begin();

for (; iter_row_x != x.end(); ++iter_row_x, ++iter_row_y) {
    std::vector<double>::const_iterator iter_col_x = iter_row_x[0].begin();
    std::vector<double>::const_iterator iter_col_y = iter_row_y[0].begin();
    for (; iter_col_x != iter_row_x[0].end(); ++iter_col_x, ++iter_col_y) {
        unsigned int i = unsigned int(*iter_col_x);
        unsigned int j = unsigned int(*iter_col_y);
        M[i][j] = 1;
    }
}

I would like to know if there is a more elegant way to achieve this? My code above looks a bit bulky and error-prone for such a simple task.

auto
Automatic type deduction is especially welcoming for iterators, as their types are bulky. We can rewrite to
std::vector<std::vector<double>> x;
std::vector<std::vector<double>> y;

// fill x and y

std::vector<std::vector<unsigned int>> M(n_rows, std::vector<unsigned int>(n_cols));

auto iter_row_x = x.cbegin(); //note cbegin to get const_iterator
auto iter_row_y = y.cbegin();

for (; iter_row_x != x.end(); ++iter_row_x, ++iter_row_y) {
    auto iter_col_x = iter_row_x->cbegin(); // iter_row_x[0].cbegin() = iter_row_x->cbegin()
    auto iter_col_y = iter_row_y->cbegin();
    for (; iter_col_x != iter_row_x[0].end(); ++iter_col_x, ++iter_col_y) {
        auto i = static_cast<unsigned int>(*iter_col_x); // be explicit about which cast you are using. Now you do not need to specify the type twice.
        auto j = static_cast<unsigned int>(*iter_col_y);
        M[i][j] = 1;
    }
}

But it's still wordy. Having separate vectors for x and y seems unnecessary, since it is expected that they should be of same length. Better create a structure for xy and create only one vector.
struct Point
{
    double x;
    double y;
}

Then we have
std::vector<std::vector<Point>> xy;

// fill xy

std::vector<std::vector<unsigned int>> M(n_rows, std::vector<unsigned int>(n_cols));

for (auto iter_row_xy = xy.cbegin() ; iter_row_xy != xy.end(); ++iter_row_xy) {
    for (auto iter_col_xy = iter_row_xy->cbegin(); iter_col_x != iter_row_x[0].end(); ++iter_col_x, ++iter_col_y) {
        auto i = static_cast<unsigned int>(*iter_col_xy.x);
        auto j = static_cast<unsigned int>(*iter_col_xy.y);
        M[i][j] = 1;
    }
}

But we can do even better by using for range:
std::vector<std::vector<Point>> xy;

// fill xy

std::vector<std::vector<unsigned int>> M(n_rows, std::vector<unsigned int>(n_cols));

for (const auto& elements : xy){
    for (const auto& point : elements){
        auto i = static_cast<unsigned int>(point.x);
        auto j = static_cast<unsigned int>(point.y);
        M[i][j] = 1;
    }
}


Finds all valid words by arranging 3 cubes Given 3 cubes, each cube has 3 different letters(other sides have
  pictures and numbers - they can be ignored for the purposes of this
  program). Find all valid words that can be made by arranging these
  cubes.

  Cube A = 'A', 'J' and 'N'; Cube B = 'B', 'K' and 'O'; Cube C = 'C',
  'L' and 'P'

import re
from itertools import permutations

a = ['A','J','N']
b = ['B','K','O']
c = ['C','L','P']

combinationsList = []#list of all possible variations of 3 letter words
for a_letter in range(len(a)):
    for b_letter in range(len(b)):
        for c_letter in range(len(c)):
            word = a[a_letter] + b[b_letter] + c[c_letter]
            combinationsList.append(word)
print len(combinationsList)

permutationsList = []#list of all permutations of the combinations list [changing positions of the cubes to form different words]
for word in combinationsList:
    perms = [''.join(p) for p in permutations(word)]
    permutationsList += perms

print len(permutationsList)

dictionList = []#list of all valid words found that match the length of number of cubes (in this scenario there are 3 cubes)
with open("/usr/share/dict/words", "r") as diction:
    for line in diction:
        word = line.strip()
        if re.match('.{3}$', word) != None:#improve regex search to only find words in the valid character setw
            dictionList.append(word.upper())
print len(dictionList)

match = []

for word in permutationsList:
    if dictionList.count(word) > 0:#compares all word permutations with all words in diction for matches
        match.append(word)
print match

It's just as easy to iterate over the characters in a string as over the elements in a list, so the initialization of the cubes can be simplified:

a, b, c = 'AJN BKO CLP'.split()

combinationsList can be computed using itertools.product:

combinationsList = list(itertools.product(a, b, c))

Instead of using a regular expression:

if re.match('.{3}$', word) != None:

just check the length:

if len(word) == 3:

(But once you have an efficient membership test, as discussed below, you can skip this check.)
The code checks for membership in a list by counting the number of occurrences:

if dictionList.count(word) > 0:

but it would be simpler to use the in operator:

if word in dictionList:

Testing for membership in a list is expensive — Python has to compare word with every member of dictionList in turn. It would be more efficient to use a set, which has a fast membership test.
The code in the post constructs a collection of dictionary words and checks each candidate word to see if it's in the dictionary. But because there are only 162 candidate words, and hundreds of thousands of entries in the dictionary, it would be more memory-efficient to do the operation the other way round — that is, to check each dictionary word to see if it's a candidate. That way, you wouldn't have to build a collection of dictionary words, you could just iterate over them one by one.

Revised code:

from itertools import permutations, product
cubes = 'AJN BKO CLP'.split()
candidate_words = {
    ''.join(permutation)
    for letters in product(*cubes)
    for permutation in permutations(letters)
}
valid_words = set()
with open('/usr/share/dict/words') as dictionary:
    for line in dictionary:
        word = line.strip().upper()
        if word in candidate_words:
            valid_words.add(word)
print(valid_words)


C program for 'Reverse DNS lookup' I have written the following code for doing a reverse dns lookup. I'm not sure if there are any errors in it. Please have a look:

#include <stdio.h> //for printf()
#include <stdlib.h> //for exit()
#include <arpa/inet.h> //for inet_pton()
#include <netdb.h> // for NI_MAXHOST, getnameinfo() and gai_strerror()
#include <errno.h> // for errno
#include <string.h> // for strerror()

int main(int argc, char** argv) {
  if(argc<2) {
    printf("\n%s [IP]\n",argv[0]);
    printf("For e.g. %s 10.32.129.77\n",argv[0]);
    exit(-1);
  }
  struct sockaddr_in sa;
  int res = inet_pton(AF_INET, argv[1] , &sa.sin_addr);
  switch(res) {
    case 0: printf("\nInput address is not a valid IPv4 address.\n");
    case -1: if(res == -1)
               printf("\nError(%s)\n",strerror(errno));
             int n_res = inet_pton(AF_INET6, argv[1] , &sa.sin_addr);
             switch(n_res) {
               case 0: printf("\nInput address is not a valid IPv6 address.\n");
               case -1: if(n_res == -1)
                          printf("\nError(%s)\n",strerror(errno));
                        exit(-1);
               case 1: sa.sin_family = AF_INET6;
             }
    case 1: sa.sin_family = AF_INET;
  }

  printf("\nsa.sin_addr.s_addr[%d]\n",sa.sin_addr.s_addr);

  char node[NI_MAXHOST];
  memset(node,0,NI_MAXHOST);
  res = getnameinfo((struct sockaddr*)&sa, sizeof(sa), node, sizeof(node), NULL, 0, 0);
  if (res) {
    printf("%s\n", gai_strerror(res));
    exit(1);
  }

  printf("\nIP[%s]\n",argv[1]);
  printf("HOSTNAME[%s]\n", node);

  return 0;
}

It's generally best to divide your code into separate sections (possibly separate functions) for argument handling and actual computation.

Errors should be printed to standard error rather than standard output.  Also, prefer small integer values for exit status (and since we're in main(), we can use simple return rather than exit() - note the useful EXIT_FAILURE macro for the return value).

It's worth documenting case fallthroughs.  This one is especially suspect:

               case 1: sa.sin_family = AF_INET6;
             }
    case 1: sa.sin_family = AF_INET;
  }

After we store AF_INET6, it's immediately overwritten with AF_INET - is that really intended?

This fallthrough would be clearer as two independent cases:

    case 0:
        printf("\nInput address is not a valid IPv6 address.\n");
        /* fallthrough */
    case -1:
        if (n_res == -1)
            printf("\nError(%s)\n",strerror(errno));
        exit(-1);

Compare:

    case 0:
        fprintf(stderr, "\nInput address is not a valid IPv6 address.\n");
        return 1;
    case -1:
        fprintf(stderr, "\nError(%s)\n", strerror(errno));
        return 1;

This line seems to be no use to a user:

printf("\nsa.sin_addr.s_addr[%d]\n",sa.sin_addr.s_addr);

We check for argv less than 2, but we neither complain about nor use any excess arguments.

There's no need to null out the node storage, as getnameinfo() will either fail (in which case we'll never access it) or write a valid string.

Minor (grammar): don't use "for" with "e.g." - that reads like, "for for example".

When I tried running the program, I found it wouldn't work at all with IPv6 addresses, because sockaddr_in is too small for IPv6 addresses.  I had to totally rewrite with a union of address types:

#include <arpa/inet.h> //for inet_pton()
#include <netdb.h> // for NI_MAXHOST, getnameinfo() and gai_strerror()

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

static int convert4(struct sockaddr_in *sa, const char *name)
{
    return inet_pton(sa->sin_family = AF_INET, name, &sa->sin_addr);
}

static int convert6(struct sockaddr_in6 *sa, const char *name)
{
    return inet_pton(sa->sin6_family = AF_INET6, name, &sa->sin6_addr);
}

int main(int argc, char** argv)
{
    if (argc != 2) {
        fprintf(stderr, "Usage: %s [IP]\nE.g. %s 10.32.129.77\n", argv[0], argv[0]);
        return EXIT_FAILURE;
    }

    union {
        struct sockaddr         sa;
        struct sockaddr_in      s4;
        struct sockaddr_in6     s6;
        struct sockaddr_storage ss;
    } addr;

    if (convert4(&addr.s4, argv[1]) != 1 && convert6(&addr.s6, argv[1]) != 1) {
        fprintf(stderr, "%s: not a valid IP address.\n", argv[1]);
        return EXIT_FAILURE;
    }

    char node[NI_MAXHOST];
    int res = getnameinfo(&addr.sa, sizeof addr, node, sizeof node, NULL, 0, NI_NAMEREQD);
    if (res) {
        fprintf(stderr, "%s: %s\n", argv[1], gai_strerror(res));
        return EXIT_FAILURE;
    }

    puts(node);
}