Setting a common background image on a dozen screens Is there something as "they have too little in common to use a superclass"?

In my case, doing iOS programming, in a project, every screen has the same background. So I created a superclass which only has:

.h

@property (nonatomic) IBOutlet UIImageView *backgroundImageView;

.m

- (void)viewDidLoad {
    [super viewDidLoad];

    UIImage *backgroundImage = ...;
    [self.backgroundImageView setImage:backgroundImage];
}

Is this too little to use a superclass for?

If I didn't add it here, I'd have about 10 view controllers where I would have to do this. (Obviously code duplication).

The short answer is, no, this is not too little code to "extract" into a super class.

If you intend for this bit of code to always be the same throughout every use of this class and its subclasses, then it's absolutely appropriate to put the code in the superclass.  The primary point is, here, if we ever decide to change something about this code, we can change it in once place and have this change applied across all instances.

With that said, however, I want to point out some problems.

As a start, we have no guarantee that this code will actually run.  If our subclass implements viewDidLoad and fails to call to super, now our code doesn't run.  There's still no way to make this guarantee, but there is a way to make a reminder for ourselves.

As this StackOverflow answer outlines, the NS_REQUIRES_SUPER directive will throw a warning when subclass implement the method without calling the super method.  So we should add the following to our .h file:

- (void)viewDidLoad NS_REQUIRES_SUPER;

Now, a subclass of this class which implements viewDidLoad will throw a compiler warning until [super viewDidLoad]; is added to its viewDidLoad implementation.

There's problems still however.  For starters, as a rule, I don't like IBOutlets in my header file.  There's absolutely no reason for these to be public.  But in this particular case, we shouldn't have an IBOutlet at all.  Using an image view set up in interface builder and requiring it be hooked up to this outlet will supremely complicate things.

First, not only do we have to remember to call to super, which I already address.  But we also have to remember to add the image view.  And we have to remember to hook the image view up as an outlet.  Trouble is though, we can't (or shouldn't) hook it up to our current class.  We should open this, the super class, in assistant editor and hook it up to that.  Problem is, assistant editor won't automatically load super class's in the assistant window... so that's kind of a pain.

I'm a huge proponent of using interface builder.  It does a lot to vastly simplify our written code, but in this case, it doesn't suffice for producing simplified results.

We should remove this outlet altogether and not worry in the slightest about relying on the subclass setting up the image view in interface builder and hooking it up properly.  The whole point of subclassing is minimizing redundancy, and so far, we're only half way there.

(Moreover, what guarantee do we have that every subclass of this class will even use interface builder at all?  View Controllers don't require a corresponding interface builder representation--the view can be set up entirely programmatically.)

We need to change our viewDidLoad code to manually create an image view with an image and load it as the background.

The number one problem we'll run into in manually creating and adding the image view to the view controller is getting it appropriately as the background.  We can't be sure whether the subclass will do it's set up first then call super, or call super then do its set up (in case you're wondering, in viewDidLoad, it's appropriate to call to super FIRST, then add your implementation).  Moreover, the view controller will almost certainly have views added on it in interface builder if the developer is using interface builder.

So, we need to add the view as the bottom-most view in the view controller's view's subviews.

This might be a good implementation:

- (void)viewDidLoad {
    [super viewDidLoad];

    // Set up image view
    UIImage *backgroundImage = [UIImage imageNamed:@"background"];
    UIImageView *backgroundView = [[UIImageView alloc] initWithImage:backgroundImage];
    [backgroundView setTranslatesAutoresizingMaskIntoConstraints:NO];
    // aspect fill may be preferred
    backgroundView.contentMode = UIViewContentModeScaleAspectFit;

    // Add background view as back-most view
    [self.view insertSubview:backgroundView atIndex:0];

    // Set up auto layout constraints so this view controller is applicable
    // on any device in any rotation
    NSDictionary *views = NSDictionaryOfVariableBindings(backgroundView);

    NSArray *verticalConstraints =
        [NSLayoutConstraint constraintsWithVisualFormat:@"V:|-0-[backgroundView]-0-|"
                                                options:0
                                                metrics:nil
                                                  views:views];
    NSArray *horizontalConstraints =
        [NSLayoutConstraint constraintsWithVisualFormat:@"H:|-0-[backgroundView]-0-|"
                                                options:0
                                                metrics:nil
                                                  views:views];

    [self.view addConstraints:verticalConstraints];
    [self.view addConstraints:horizontalConstraints];
}

I'm not particularly a fan of setting up UI in code (especially auto layout).  I much, much prefer interface builder.  But in some cases, it is absolutely necessary to get it right.

This auto layout code may seem bulky and unnecessary if your current app is just for iPads and just for a single orientation.  But as soon as you want to subclass this view controller into an app that rotates or is made for iPhones, or is a universal app, you'll be glad to have this auto layout code.

As one final comment, I might make a UIImage property in the .h file so that the background image could be changed.


Letter frequency analysis (Breaking Vigenere) I'm trying to improve my Perl and am doing some wargames online. I wrote a small utility to help me pass a stage.

  The main weakness of a simple substitution cipher is repeated use of a simple key. In the previous exercise you were able to introduce arbitrary plaintext to expose the key. In this example, the cipher mechanism is not available to you, the attacker.

  However, you have been lucky. You have intercepted more than one message. The password to the next level is found in the file ‘krypton4’. You have also found 3 other files. (found1, found2, found3)

  You know the following important details

  The message plaintexts are in English
  They were produced from the same key
  The cyphertexts are UPPERCASE
  The ciphertexts in blocks of 5

#!/usr/bin/perl

use strict;
use warnings;
use feature 'say';

use File::Slurp;

my @FILENAMES = ("found1", "found2", "found3");
my @LETTERS = ("A" .. "Z");
my @ENGLISH_FREQ = ("E", "T", "A", "O", "I", "N", "S",
                    "H", "R", "D", "L", "C", "U", "M",
                    "W", "F", "G", "Y", "P", "B", "V",
                    "K", "J", "X", "Q", "Z");

my $ENCRYPTED_STRING = read_file("krypton4");
$ENCRYPTED_STRING =~ s/\s+//g;

sub count_frequency {
    my ($string) = @_;
    my %occurences = count_occurence($string);
    return map { $_ => $occurences{$_} / length($string) } keys %occurences;
}

sub count_occurence {
    my ($string) = @_;
    my %frequency = map { $_ => 0 } @LETTERS;
    foreach (split //, $string) {
        $frequency{$_}++;
    }
    return %frequency;
}

sub get_single_key {
    my (%text_freq) = @_;
    my @text = ();
    foreach my $name ( sort { $text_freq{$b} <=> $text_freq{$a} } keys %text_freq ) {
        push @text, $name;
    }
    return map { $text[$_] => $ENGLISH_FREQ[$_] } (0 .. $#text);
}

foreach (@FILENAMES) {
    # Read the text
    my $text = read_file($_);
    $text =~ s/\s+//g;

    # Perform Letter Frequency Analysis
    my %cypher_key = get_single_key(count_frequency($text));

    # Try to decrypt the text
    foreach (split //, $ENCRYPTED_STRING) {
        print "$cypher_key{$_}";
    }
    say "";
}

It was some puzzling afterwards, but it did its job.

use File::Slurp;

File::Slurp is considered broken by a lot of people. A good alternative is Path::Tiny.

It's also good practice to import only what you need when you use a module. That helps others to see where a function is coming from (like your read_file, which I tried to find in your code), and it helps to keep your namespace clean.

use Path::Tiny 'path';

my $text = path('krypton4')->slurp;

my @FILENAMES = ("found1", "found2", "found3");

All caps names are typically used for constants only. You can create a constant with the use constants pragma. That works by installing a sub into your namespace that returns a scalar. As a function, there's no sigil, and you need to use references for arrays.

The qw() quoted words operator is a nice shrotand for creating lists of quoted words, as you don't have to bother with commas and quotes.

use constant FILENAMES => [ qw(found1 found2 found3) ];

my $ENCRYPTED_STRING = read_file("krypton4");
$ENCRYPTED_STRING =~ s/\s+//g;

You only use this variable once. It's not really configuration either, so making it a constant feels wrong. In Perl, we try to declare variables as close to where they are used as possible, and in the smallest scope possible. I would recommend simply using Path::Tiny's path down in your foreach loop.

You could however add a constant for the krypton4 file name if you wanted.

use constant ENCRYPTED_FILE => 'krypton4';

If your Perl is at least 5.14, you can use the /r modifier on regex substitution to return an altered version, instead of doing in-place edit.

foreach my $item (split //, path(ENCRYPTED_FILE)->slurp =~ s/\s+//gr) { ... }

If your Perl is older, you can also use map to do that in one line. Note that s///g returns the number of substitutions, you have to explicitly return $_ from the block to get the text.

foreach my $item (split //, map { s/\s+//g; $_ } path(ENCRYPTED_FILE)->slurp) { ... }

foreach (split //, $string) {
    $frequency{$_}++;
}

If you use $_ explicitly in a loop body, it's clearer to use a named variable instead. I would call this $letter.

foreach my $letter (split //, $string) {
    $frequency{$letter}++;
}

sub get_single_key {
    my (%text_freq) = @_;
    my @text = ();
    foreach my $name ( sort { $text_freq{$b} <=> $text_freq{$a} } keys %text_freq ) {
        push @text, $name;
    }

    return map { $text[uc($_)] => $ENGLISH_FREQ[$_] } (0 .. $#text);
}

The frequency you've calculated with count_frequency is the frequency of each letter in the text. Your variable %text_frequency should thus be named %letter_frequency. But that's long. Why not stick with %frequency? It worked above.

You don't need to initialize arrays and hashes with an empty list. Perl does that for you.

I didn't read the algorithm description, so I don't know exactly what this does, but @text feels like the name is wrong. That variable is not an array of texts, or ordered pieces of one text. I think it contains the letters ordered by frequency. So @letters might be better.

It's good that you used a named loop variable, but again, this is a letter, right? So $letter is way more fitting than $name.

You can just assign the list of keys that sort returns to @letters.

There's a bug in your map statement. The $_ is not the letter, but the index. Your iteration 0 .. $#text gives you all the indexes. You are ucing the index, not the value.

You don't need the parentheses around the range. map takes a list, and the range operator .. gives you a list.

sub get_single_key {
    my (%frequency) = @_;

    my @letters = sort { $frequency{$b} <=> $frequency{$a} } keys %frequency;

    return map { uc $letters[$_] => ENGLISH_FREQUENCIES->[0] } 0 .. $#letters;
}

Since you have that comment Perform Letter Frequency Analysis where you use this function, and you return a hash, which implies a bunch of things, the name get_single_key might not be optimal.

foreach (split //, $ENCRYPTED_STRING) {
    print "$cypher_key{$_}";
}

A postfix foreach (or for for brevity) will be easier to read here.

You don't have to quote a single variable. There is no need for interpolation.

print $cypher_key{$_}
    for split //, path(ENCRYPTED_FILE)->slurp =~ s/\s+//gr;

say "";

It's shorter than print "\n" (or print "$/" if you insist on being portable), but it just feels off. It's good though that you realized that you need an empty string so it does not output $_ instead.


Which of these two paren-matching functions is better? I am currently trying to learn Haskell (after taking a Scala course on Coursera); but while I can write functions that do what I want, I worry that I am not learning to write idomatic/clean/performant code. I am hoping that this is an acceptable place to receive feedback on how I am progressing. Please let me know if my examples are too small/'toy'/not appropriate for this area of the site.

Which of these two parenthesis matching functions is better - or how best should it be written? The function takes a String, and returns True if the parentheses are balanced, otherwise False.

"(hello)()" = True
")(test()" = False
"())" = False

Explicitly written:

balance :: String -> Bool
balance xs = bal 0 xs == 0
  where bal c [] = c
        bal c (x:xs)
          | c < 0     = c
          | x == '('  = bal (c + 1) xs
          | x == ')'  = bal (c - 1) xs
          | otherwise = bal c xs

Using foldl:

balance' :: String -> Bool
balance' xs = foldl bal 0 xs == 0
  where bal count char
          | count < 0   = count
          | char == '(' = count + 1
          | char == ')' = count - 1
          | otherwise   = count

I think that the explicitly-recursing one is better since it will terminate early if it detects the String is flawed, but the other one might be 'clearer' since it uses foldl. Both of them happen to be tail-recursive. How would I improve either?

How important is writing tail-recursive functions in Haskell? I have read somewhere that Haskell is very smart about making things tail recursive, but I do not understand enough to understand quite how.

Any help and suggestions would be greatly appreciated, thank you!

I would propose an even less performant solution, but maybe you can improve it...

balance xs = head cumulated == 0 && all (>= 0) cumulated where
  cumulated = scanr (+) 0 $ map depth xs
  depth '(' = -1
  depth ')' = 1
  depth _ = 0

Note that I work "backwards" (scanr and inverted parens values) in order to avoid a call to last cumulated.


Count number of gemstones in a set of minerals This is the "Gemstones" problem on Hackerrank.

  John has collected various rocks. Each rock has various minerals embeded in it. Each type of mineral is designated by a lowercase letter in the range a-z. There may be multiple occurrences of a mineral in a rock. A mineral is called a gemstone if it occurs at least once in each of the rocks in John's collection.
  Given a list of minerals embedded in each of John's rocks, display the number of types of gemstones he has in his collection.
  For example, the array of mineral composition strings [abc, abc, bc] The minerals b and c appear in each composite, so there are 2 gemstones.

This is my Python code:

def gemstones(arr):
    for i in range(len(arr)):
        arr[i] = "".join(set(arr[i]))

    long = max(arr, key=len)
    arrlen = len(arr)
    flag,count = 0,0
    for i in long:
        for j in range(arrlen):
            if i not in arr[j]: flag = 1
        if flag is 0: count += 1
        flag = 0
    return count

Are there ways to improve this code? I feel I'm not using the full functionality of Python.

A test case:

>>> arr = ['abcdde', 'baccd', 'eeabg']
>>> print(gemstones(arr))
2

You need to stop iterating over indexes and loop like a native. A first rewrite with that in mind would yield:

def gemstones(collection):
    collection = [''.join(set(rock)) for rock in collection]
    biggest_rock = max(collection, key=len)

    count = 0
    for mineral in biggest_rock:
        for rock in collection:
            if mineral not in rock:
                break
        else:
            count += 1
    return count

This version make use for the for...else construct. But the all() builtin would be more expressive:

def gemstones(collection):
    collection = [''.join(set(rock)) for rock in collection]
    biggest_rock = max(collection, key=len)

    count = 0
    for mineral in biggest_rock:
        if all(mineral in rock for rock in collection):
            count += 1

    return count

And counting a condition in a for-loop can be more efficient using sum():

def gemstones(collection):
    collection = [''.join(set(rock)) for rock in collection]
    biggest_rock = max(collection, key=len)

    return sum(
        all(mineral in rock for rock in collection)
        for mineral in biggest_rock
    )

Other than that, you won't have more gemstones than the amount of minerals in the smallest rock, so why doing extra work by using the biggest one to start with?

But you are going back and forth between strings and sets when the problem clearly calls for set intersection repeated on several elements of an array. Luckily, the set.intersection method accept a variable number of arguments. Just make sure to catch any error thrown in case the original collection is empty:

def gemstone(collection):
    rocks = map(set, collection)
    try:
        minerals = next(rocks)
    except StopIteration:
        return 0  # If the collection is empty, there is no gemstones
    return len(minerals.intersection(*rocks))


Better way to fetch JSON data from a server from iOS app I have successfully coded a network request to fetch JSON data from our server. The remote function is written in ColdFusion. However, the code is quite lengthy and involved. I also noticed in the API there are other seemingly very similar ways one can go about making network requests, such as NSURLSession. (I know NSURLConnection will be deprecated in the future, replaced by NSURLSession.) I would like to know what is the best way to make a network request - using modern code that is not going to be deprecated in the near future. Is NSURLSession the best way to go?

Here are my requirements:

Fetch JSON data from a function in a .cfc file on the server
Must be secure
Must be able to perform error checking
Only need to support iOS 7+
Preferably more concise code

Here is my current and lengthy code, using several variables, NSMutableURLRequest, and NSURLConnection:

dispatch_queue_t queue = dispatch_queue_create("Q", NULL);
dispatch_async(queue, ^{
    NSInteger success = 1;

    NSURL *url = [NSURL URLWithString:MYURL];
    NSData *postData = [[[NSString alloc] initWithFormat:@"method=methodName&username=%@&password=%@", usernameVar, passwordVar] dataUsingEncoding:NSUTF8StringEncoding];
    NSString *postLength = [NSString stringWithFormat:@"%lu", (unsigned long)[postData length]];

    NSMutableURLRequest *request = [[NSMutableURLRequest alloc] init];
    [request setURL:url];
    [request setHTTPMethod:@"POST"];
    [request setValue:@"application/x-www-form-urlencoded" forHTTPHeaderField:@"Content-Type"];
    [request setValue:postLength forHTTPHeaderField:@"Content-Length"];
    [request setValue:@"application/json" forHTTPHeaderField:@"Accept"];
    [request setHTTPBody:postData];

    NSError *requestError = [[NSError alloc] init];
    NSHTTPURLResponse *response = nil;
    NSData *urlData = [NSURLConnection sendSynchronousRequest:request
                                            returningResponse:&response
                                                        error:&requestError];

    //if communication was successful
    if ([response statusCode] >= 200 && [response statusCode] < 300) {
        NSError *serializeError = nil;
        NSDictionary *jsonData = [NSJSONSerialization
                                  JSONObjectWithData:urlData
                                  options:NSJSONReadingMutableContainers
                                  error:&serializeError];
        success = [jsonData[@"ERROR"] integerValue];

        if (success == 0) {
            //success! do stuff with the jsonData dictionary here
        }
        else {
            //handle ERROR from server
        }
    }
    else {
        //handle error for unsuccessful communication with server
    }
});

Could you help me clean this up to be implemented the most preferred way for modern apps of today?

NSURLSession was designed from the ground up to be as similar to NSURLConnection as possible, so that those accustomed to using the latter will have no problem transitioning to the former.  Meanwhile, NSURLSession has a lot of major advantages over NSURLConnection.

First of all, we can get rid of all the code for dispatching on a background thread.  NSURLSession works on background threads automatically unless we specify otherwise.

We still have to create our NSURLRequest.  I don't know any way around this.

But once we've got our request set up, and remember, all of this is outside the GCD block, we just do this:

NSURLSession *session = [NSURLSession sessionWithConfiguration:[NSURLSessionConfiguration defaultSessionConfiguration]];

[[session dataTaskWithRequest:request
            completionHandler:^(NSData *data, NSURLResponse *response, NSError *error) {

    // do stuff

}] resume];

As you can see, it's quite similar to NSURLConnection.  We replace // do stuff with the stuff we want to do when the request is complete (completion handler).  We have our data, our response, and our error all in this block.

A lot of the new power from NSURLSession comes from the NSURLSessionConfiguration object.  Using NSURLSessionConfiguration, we can easily set up downloads/uploads to happen in the background (when our app is not open).  We can specify to only run on Wi-Fi and not use cellular data as setting a BOOL.  We can set a BOOL to tell the OS to only try doing our background uploads/downloads when the device is on Wi-Fi and plugged into a power source.

NSURLSessionConfiguration also has an HTTPAdditionalHeaders property, which is an NSDictionary object containing additional headers that should be sent with requests using this configuration.

There's a lot more information about NSURLSessionConfiguration here.


Validation Extension method performance I'm writing a simple set/framework of extension methods for argument/parameter validation. What I'm mostly concerned about, ATM is the performance, against the simpliest boilerplate
if(obj == null)
   throw new ArgumentNullException()

The framework I came up with is like that:
The Validatable class packing the base object that will be validated through the ThrowIf() extension method.
public class Validatable<T>
{
    public T Value { get; }
    public Validatable(ref T argument)
    {
        Value = argument;
    }
}

public static Validatable<T> ThrowIf<T>(this T argument)
{
    return new Validatable<T>(ref argument);
}

And test methods themselves:
public static Validatable<T> IsNull<T>(this Validatable<T> argument) where T : class
{
    if (argument.Value == null)
        throw new ArgumentNullException();
    return argument;
}

public static Validatable<T> CollectionEmpty<T>(this Validatable<T> argument) where T: ICollection
{
    if (argument.Value.Count == 0)
        throw new ArgumentException();
    return argument;
}

This allows me to validate items with simple:
List<int> testList = new List<int>() { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };
testList.ThrowIf().IsNull().CollectionEmpty();

Everything was fine untill I decided to test, how it fares against the simple boilerplate.
int iNumber = 1000000000;
Stopwatch sw1 = new Stopwatch();
Stopwatch sw2 = new Stopwatch();
sw1.Start();
for (int i = 0; i < iNumber; ++i)
{
    if (testList == null)
        throw new ArgumentNullException();
    if (testList.Count == 0)
        throw new ArgumentException();
}
sw1.Stop();
sw2.Start();
for (int i = 0; i < iNumber; ++i)
{
    testList.ThrowIf().IsNull().CollectionEmpty();
}
sw2.Stop();

Console.WriteLine($"sw1: {sw1.ElapsedTicks} ticks");
Console.WriteLine($"sw2: {sw2.ElapsedTicks} ticks");

and the output was:

sw1: 10720019 ticks
sw2: 161726524 ticks

That's an order of magnitute difference. It was over 1,000,000,000 iterations, but still the difference is there.
I also tried with
public static void IsNullLite<T>(this T argument)
{
    if (argument == null)
        throw new ArgumentException();
}

public static void CollectionEmpty2Lite<T>(this T argument) where T : ICollection
{
    if (argument.Count == 0)
        throw new ArgumentException();
}

And it's result came somewhere in the middle with

sw3: 84165637 ticks

My questions are:

Should I be concerned with the performance loss. It was +- 3sec vs 47secs on my PC, at 1,000,000,000 iterations. I do not plan to run it in such loops, over so many objects, but I was going to use it my game engine, where I have about 16ms per Update tick, to do much more than just validate some variables.
Is there a better/faster way of doing it. Preferably, I'd like to pack the validation object into some sort of Validatable class, to not clutter the intelisense with dozens of validation methods.
Is there some error on my part, in the way I tested it? I ran the tests in Debug mode with code optimalization turned off.

public static Validatable<T> ThrowIf<T>(this T argument)
{
    return new Validatable<T>(ref argument);
}

This of course is creating a lot of objects. You could speed this a little bit up by making Validatable a struct but still, it'll be slower then the normal check. Convenience comes often with some kind of a performance hit.

  I was going to use it my game engine

You'd better not. Make it fast, not convenient.

There is one more thing about validating arguments. The only thing you need to validate is user input or something that you cannot predict like configurations. Internally most methods usually work without argument checking because it's nearly impossible to pass invalid values. Use Debug.Assert for this purpose during development to be absolutely sure if you want. It'll be removed in the release version so that it doesn't slow the application down.


Aggregation on list forEach and maps improvals I am using a function in a SpringBoot @RestController, with Lombok

This function is an intermediate result that permit me to do an aggregation between two classes.

This 2 classes are:

@Data
public class AchMatiereCmdCptAggDto implements Serializable {

    private static final long serialVersionUID = 4022154955808447185L;
    private String codService;
    private String codSociete;
    private String numCompte;
    private String codCommande;
    private Double mntCommandeHTLCY;
    private Double mntLivreHTLCY;
    private Double mntFactureHTLCY;
}

@Getter
@Setter
public class AchMatiereCompteCommandeAggFullDto extends AchMatiereCmdCptAggDto implements Serializable {

    private static final long serialVersionUID = 6357406952790844723L;
    private Date dateCommande;
    private String codFournisseur;
    private String libFournisseur;
    private Boolean booFournisseurIntragroupe;
    private Long idCommandeEtat;

}

public static String stringConcat(String ... elements) {
    return new StringBuilder().append(elements).toString();
}

I use them that way, and I want to improve forEach part, and any other improvable piece of code:

 public List<AchMatiereCompteCommandeAggFullDto> mergeAchat(List<AchMatiereCompteCommandeAggFullDto> achats,

List<AchMatiereCmdCptAggDto> achats2) {

    Set<AchMatiereCmdCptAggDto> setAchats = new HashSet<>(achats2);
    Map<String, AchMatiereCmdCptAggDto> map = setAchats.stream()
            .collect(Collectors.toMap(
                    p -> stringConcat(p.getNumCompte(), p.getCodCommande()),
                    p -> p, (ach1, ach2) -> ach2
            ));

    achats.stream().forEach(achat -> {
        String k = stringConcat(achat.getCodSociete(), achat.getCodService(), achat.getNumCompte(), achat.getCodCommande());

        if (map.containsKey(k)) {
            achat.setMntFactureHTLCY(map.get(k).getMntFactureHTLCY());
        }
    });
    return achats;
}

I was thinking about using a StringBuilder for the concatenation of attributes.

I would also like to avoid forEach, and use a Map or something like this.

Thanks for your help.

From java 8 it is available in the String class the method join that achieves the same result of your method stringConcat so instead of:

String k = stringConcat(achat.getCodSociete(), achat.getCodService(), achat.getNumCompte(), achat.getCodCommande());

You can use String.join with "" delimiter:

String k = String.join("", achat.getCodSociete(), achat.getCodService(), achat.getNumCompte(), achat.getCodCommande());

It is also possibile to avoid forEach and use Map instead but because you are modifying the achats list elements, you have to map every element of the list to a new instance of your class and after you obtain your new list. To achieve this you can define a new copy constructor for the class:

achats.stream()
    .map(original -> {
        String k = //omitted for brevity : see above the code for k
        AchMatiereCompteCommandeAggFullDto copy = new AchMatiereCompteCommandeAggFullDto(original);
        if (map.containsKey(k)) {
            copy.setMntFactureHTLCY(map.get(k).getMntFactureHTLCY());
        }
        return copy;
    }).collect(Collectors.toList()); //<- return the new list


Stack implementation using a linked list To understand the concept, I implemented the stack operations using a linked list. Please review the code and tell me your suggestions.

Node.java

public class Node {

public int data;
public Node next;

public Node(int data) {
    this.data = data;
}

public void displayNode() {
    System.out.print(data);
    System.out.print("  ");

 }
}

LinkList.java

public class LinkList {

private Node first = null;

public void insertFirst(int data) {
    Node n = new Node(data);
    n.next = first;
    first = n;
}

public Node deleteFirst() {
    Node temp = first;
    first = first.next;
    return temp;
}

public void displayList() {
    Node current = first;
    while (current != null) {
        current.displayNode();
        current = current.next;
    }
}

public boolean isEmpty() {
    return (first == null);
  }
}

LinkListStack.java

public class LinkListStack {

LinkList li = new LinkList();

public void push(int data) {
    li.insertFirst(data);
}

public void pop() {
    while(!li.isEmpty()){
    li.deleteFirst();
    }
}

public void displayStack() {
    System.out.println("  ");
    li.displayList();
  }
}

LinkListStackDemo.java

public class LinkListStackDemo {

public static void main(String[] args) {
    LinkListStack st = new LinkListStack();

    st.push(50);
    st.push(70);
    st.push(190);
    st.displayStack();
    st.pop();
    st.displayStack();

  }
 }

Time complexity

The time complexity of push and pop operations should be \$O(1)\$, and so it is in your case too. It doesn't matter how many elements you have, these operations should take constant time. (UPDATE: you've edited your original post, and made pop wipe out the entire stack. That's not normal! Normally, the pop operation on a stack should return the most recently added value. That's \$O(1)\$ time.)

Avoid printing to stdout

Instead of the display* methods that print to stdout, it would be better to override toString. That way your implementation would be more testable.

Generalize

Why limit the stack, linked list, node elements to int type? It would be trivially easy to rewrite to make it work with any type T.

The question is tagged "beginner", so I understand you might not be familiar with generics just yet. In that case, see this official tutorial. Or perhaps you can also learn from my example implementation further down.

Add an isEmpty method for the stack

Your linked list has an isEmpty method but the stack doesn't. It would be good to have such method for the stack too.

Reinventing the wheel

When reinventing the wheel (here, linked list), it's good to mimic what exists. For example, java.util.LinkedList uses the method names addFirst and removeFirst, instead of insertFirst and deleteFirst. It's good to follow the example.

Access modifiers and encapsulation

As @rolfl pointed out, Node should not be exposed to the outside. Users of the stack should not have to know its inner workings.

Also, the members of Node should be private, and the data and next fields can be final. Similarly in the stack, the linked list member should be private.

Naming

You use poor names in many places.

Instead of n for the new node when replacing the first item of a linked list, newFirst would be more intuitive
Instead of temp for the old first item removed from a linked list, oldFirst would be more intuitive
Instead of li for the linked list in the stack, linkedList would be more intuitive

Suggested implementation

class LinkList<T> {

    private static class Node<T> {

        private final T data;
        private final Node<T> next;

        public Node(T data) {
            this.data = data;
        }

        @Override
        public String toString() {
            return data.toString();
        }
    }

    private Node<T> first = null;

    public void addFirst(T data) {
        Node<T> newFirst = new Node<T>(data);
        newFirst.next = first;
        first = newFirst;
    }

    public T removeFirst() {
        Node<T> oldFirst = first;
        first = first.next;
        return oldFirst.data;
    }

    @Override
    public String toString() {
        StringBuilder builder = new StringBuilder();
        Node current = first;
        while (current != null) {
            builder.append(current).append(" ");
            current = current.next;
        }
        return builder.toString();
    }

    public boolean isEmpty() {
        return first == null;
    }

}

class LinkListStack<T> {

    private final LinkList<T> linkedList = new LinkList<>();

    public void push(T data) {
        linkedList.addFirst(data);
    }

    public T pop() {
        return linkedList.removeFirst();
    }

    public boolean isEmpty() {
        return linkedList.isEmpty();
    }

    @Override
    public String toString() {
        return linkedList.toString();
    }
}

Unit tests

@Test
public void testPushAndPop() {
    LinkListStack<Integer> st = new LinkListStack<>();
    st.push(50);
    st.push(70);
    st.push(190);
    assertEquals("190 70 50", st.toString());
    assertEquals(190, (int) st.pop());
    assertEquals("70 50", st.toString());
}

@Test
public void testPopUntilEmpty() {
    List<Integer> values = Arrays.asList(50, 70, 190, 20);
    LinkListStack<Integer> st = new LinkListStack<>();
    assertTrue(st.isEmpty());
    for (Integer value : values) {
        st.push(value);
    }
    assertFalse(st.isEmpty());
    for (int i = values.size(); i > 0; --i) {
        assertEquals(values.get(i - 1), st.pop());
    }
    assertTrue(st.isEmpty());
}


Printing human-readable number of bytes def as_size( s )
  prefix = %W(TiB GiB MiB KiB B)
  s = s.to_f
  i = prefix.length - 1
  while s > 512 && i > 0
    s /= 1024
    i -= 1
  end
  ("%#{s > 9 ? 'd' : '.1f'} #{prefix[i]}" % s).gsub /\.0/, ''
end

I'd like a clean and fast code to convert raw file size to human-readable format. Any hints?

Update (faster format):

def as_size( s )
  prefix = %W(TiB GiB MiB KiB B)
  s = s.to_f
  i = prefix.length - 1
  while s > 512 && i > 0
    s /= 1024
    i -= 1
  end
  ((s > 9 || s.modulo(1) < 0.1 ? '%d' : '%.1f') % s) + ' ' + prefix[i]
end

Update (freeze the constant):

PREFIX = %W(TiB GiB MiB KiB B).freeze

def as_size( s )
  s = s.to_f
  i = PREFIX.length - 1
  while s > 512 && i > 0
    i -= 1
    s /= 1024
  end
  ((s > 9 || s.modulo(1) < 0.1 ? '%d' : '%.1f') % s) + ' ' + PREFIX[i]
end

Possible solution: reduce your input to required precision. Returned result construction also reads cleaner with this approach:

def as_size(s)
  units = %W(B KiB MiB GiB TiB)

  size, unit = units.reduce(s.to_f) do |(fsize, _), utype|
    fsize > 512 ? [fsize / 1024, utype] : (break [fsize, utype])
  end

  "#{size > 9 || size.modulo(1) < 0.1 ? '%d' : '%.1f'} %s" % [size, unit]
end


CodeWars Deodorant Evaporator How can I make the solution to this https://www.codewars.com/kata/5506b230a11c0aeab3000c1f/train/fsharp more "functional"?
module Evaporator
    let evaporator (content: double) (evapPerDay: double) (threshold: double): int =
        let minUsefulAmount = content * (threshold / 100.)
        let evapAsPercentage = evapPerDay / 100.
        let rec solve (content: double) (dayCount: int) =
            let amountLost = content * evapAsPercentage
            let newContent = content - amountLost
            let stillUseful = newContent > minUsefulAmount
            let result =
                match stillUseful with
                | true -> solve newContent dayCount + 1
                | false -> dayCount
            result
        solve content 1

The first thing that comes into my mind is that if evapPerDay is <= 0 the function will continue infinitely - or more precise: until a stack overflow is encountered. So you have to guard against that:
let evaporatorReview (content: float) (evapPerDay: float) (threshold: float): int =
    if evapPerDay <= 0. then
        failwith "evapPerDay must be greater than 0.0"
    else
        // ... The original algorithm

The next is that by having the unnecessary local result variable solve() isn't tail recursive. You can fix that by simply return directly from the match-entries:
let evaporator (content: float) (evapPerDay: float) (threshold: float): int =
    let minUsefulAmount = content * (threshold / 100.)
    let evapAsPercentage = evapPerDay / 100.

    let rec solve (content: float) (dayCount: int) =
        let amountLost = content * evapAsPercentage
        let newContent = content - amountLost
        let stillUseful = newContent > minUsefulAmount

        match stillUseful with
        | true -> solve newContent (dayCount + 1)
        | false -> dayCount
    solve content 1

IMO all the temporary variables in solve() blur what actually is going on. By skipping them and do the calculations directly in the recursive call to solve() the picture is more clear:
let evaporator (content: float) (evapPerDay: float) (threshold: float): int =
    if evapPerDay <= 0. then
        failwith "evapPerDay must be greater than 0.0"
    else
        let limit = content * threshold / 100.
        let rec solve (content: float) (dayCount: int) =
            match content with
            | x when x <= limit -> dayCount
            | _ -> solve (content * (1. - evapPerDay / 100.)) (dayCount + 1)

        solve content 0

In fact you don't have to calculate on the content, you can do it percentage wise:
let evaporatorReview (content: float) (evapPerDay: float) (threshold: float): int =
    if evapPerDay <= 0. then
        failwith "evapPerDay must be greater than 0.0"
    else
        let limit = threshold / 100.
        let rec solve (remaining: float) (dayCount: int) =
            match remaining with
            | x when x <= limit -> dayCount
            | _ -> solve (remaining * (1. - evapPerDay / 100.)) (dayCount + 1)

        solve 1. 0

The mathematical "discipline" in question here is exponential growth (r > 0) or decay (r < 0) and there is a formula for that:
Xn = X0 * (1 + r)^n

Where Xn is threshold, X0 is 100 or content, r is evapPerDay / 100. and n is the number of days = the result.
This can be use in sequential calculations ending when the threshold is met:
let evaporatorSeq (content: float) (evapPerDay: float) (threshold: float): int =
    if evapPerDay <= 0. then
        failwith "evapPerDay must be greater than 0.0"
    else
        let limit = threshold / 100.0
        Seq.initInfinite (fun i -> i)
        |> Seq.takeWhile (fun n -> Math.Pow(1.0 - evapPerDay / 100.0, float n) > limit)
        |> Seq.last
        |> (+) 1

But even better, it can be solved in respect to n as :
n = log(Xn/X0) / log(1 + r)

which can be used in the function as an O(1) - solution:
let evaporator content evapPerDay threshold =
    match evapPerDay with
    | x when x = 100. -> 1
    | x when x <= 0.0 -> failwith "evapPerDay must be greater than 0.0"
    | _ -> int (Math.Ceiling(Math.Log((threshold / 100.) / 1.) / Math.Log(1. - evapPerDay / 100.)))

The division by 1. is of cause redundant, but it emphasize the relation of the expression to its origin.
if evapPerDay = 100. then 1 is necessary here because if evapPerDay = 100 then Math.Log(1.0 - evapPerDay / 100.) becomes Math.Log(0.) which isn't defined.


Guess Number Game - Console based game using Java This game is pretty common for beginner projects. I wrote a version of this before all in Main - so here I challenge myself to recreate it in a more OO style.
I wanted to take a more OO approach, so any feedback on where I could do better or am making some errors is appreciated. I also have some personal confusion if in some parts I'm coding this procedurally. I might be mistaken but I think this code is a bit of both styles, and if so, can someone help explain the distinctive differences? Is there too much recursion in some parts? How does the readability appear? What are your thoughts?
The game will generate a random secret number and ask the user to input their guesses within the given number of attempts Code below:
import java.util.InputMismatchException;
import java.util.Random;
import java.util.Scanner;

public class GuessGame {

    private static final Scanner SC = new Scanner(System.in);
    private static final Random R = new Random();
    private static int secret = R.nextInt(100) + 1;
    private static final int GIVEN_ATTEMPTS = 10;
    private int attempts = 0;

    public void playGame() {
        promptGuess();
    }

    private int promptGuess() {
        try {
        printLine("Guess a number from 1-100");
        int guess = SC.nextInt();
            if (guess < 1) {
                promptGuess();
            } else {
                return compare(guess);
            }
        } catch(InputMismatchException e) {
            printLine("Only numbers please");
            SC.nextLine();
            promptGuess();
        }
        return 0;
    }

    private int compare(int guess) {
        attempts++;
        if (attempts == GIVEN_ATTEMPTS) {
            return promptResult(4);
        }
        int difference = secret - guess;

        if (difference > 0) {
            return promptResult(1);
        } else if (difference < 0) {
            return promptResult(2);
        } else {
            return promptResult(3);
        }
    }

    private int promptResult(int n) {
        switch (n) {
            case 1:
                printLine("Guess was too low, try again");
                promptGuess();
                break;
            case 2:
                printLine("Guess was too high, try again");
                promptGuess();
                break;
            case 3:
                printLine("You got it! Nice guess! The number was " + secret);
                printLine("In " + attempts + " attempts");
                printLine("Game Over");
                attempts = 0;
                secret = R.nextInt(100) + 1;
                playAgain();
            case 4:
                printLine("Too many guesses! " + GIVEN_ATTEMPTS + "/" + GIVEN_ATTEMPTS + " used.");
                printLine("Secret number was: " + secret + " Game Over");
                attempts = 0;
                secret = R.nextInt(100) + 1;
                playAgain();

        }
        return 0;
    }

    private void printLine(String s) {
        System.out.println(s);
    }

    private void playAgain() {

        printLine("Play again? (Enter Y for yes or N for no)");
        String reply = SC.next();
        if (reply.equalsIgnoreCase("Y")) {
            playGame();
        } else if (reply.equalsIgnoreCase("N")) {
            printLine("Thanks for playing");
            System.exit(0);
        } else {
                System.out.println("Please enter a valid answer");
                playAgain();
            }
        }
    }

    public class Main {

    public static void main(String[] args) {
        GuessGame g = new GuessGame();

        g.playGame();
    }
}

Curious, this code looks a lot like the code in this question. I'm not saying that you have another account, but you don't happen to be in the same class or something?

Your formatting seems to be inconsistent, use an automatic code formatter (your IDE most likely has one).

private static final Scanner SC = new Scanner(System.in);

Don't shorten names just because you can, it makes the code harder to understand and maintain.
The usage of the scanner (or streams in general) should most likely be scoped. Streams are rather easily associated with native resources which must be destroyed explicitly to be freed.

    private static final Scanner SC = new Scanner(System.in);
    private static final Random R = new Random();
    private static int secret = R.nextInt(100) + 1;
    private static final int GIVEN_ATTEMPTS = 10;
    private int attempts = 0;

Some of your state is static, your methods are all instance. You most likely want to make all your state instance.

           if (guess < 1) {
                promptGuess();
            } else {
                return compare(guess);
            }

You're recursing here, so keeping to hit Return without entering something should at some point crash your game because of a stackoverflow.

if (attempts == GIVEN_ATTEMPTS) {

A check for greater-quals would be safer.

private int promptResult(int n) {

This does not prompt for a result, it displays whether the guess was correct or not, the method should be renamed.

        switch (n) {
            case 1:

Instead of using numbers here, you could either use constants or an enum. That way nobody would need to remember that 2 means "too high".

    private void printLine(String s) {
        System.out.println(s);
    }

Good idea, but you're gaining very, very little when it comes to readability with this method.

System.exit(0);

Something to keep in mind is that System.exit is not "exit the application" but "kill the JVM process". When invoking this not even finally blocks may run. In this case it does not matter, but something to keep in mind.

Your logic is all over the place. Instead of chaining the methods together like you did, you should consider having a main method which handles the flow and calls the methods as needed. That way you'd have the logic of your game in one place but the "details" are hidden away in functions, which would make it rather easy to not only figure out how the game works, but also makes it easier possible to change single functions.
Regarding on how to structure this more proper, there is not much to do except cleaning up the mixed state. What you can start with is to have the logic of the game separated from the logic to print to the terminal. So you'd have a Game class and that one would accept a GameUi interface as parameter, like this:
public interface GameUi {
    public abstract int getGuess();
    // ...
}

public class SimpleTerminalGameUi implements GameUi;

public class Game {
    public Game(GameUi gameUi);

    // Somewhere in your logic:
    int guess = gameUi.getGuess();
}

new Game(new SimpleTerminalGameUi());

That would allow you to decouple the presentation from the internal state of the game.
As further exercise, do the same with the secret provider.

I might be mistaken but I think this code is a bit of both styles, and if so, can someone help explain the distinctive differences?

Your code is not object-oriented at all, you're just using a single object as container. But to be fair, there isn't that much logic to shell out to different objects to begin with.


A large number of math type equations and functions in a python module I have roughly 40 or 50 functions that are similar to the one provided by the code below.

I kept all the common calculations between multiple functions separate such as service_coverage_area . I then kept all the specific calculations that are not repeated or needed by any other functions in the function that needs those calculations such as calculating f_sc.

My question is: Am I doing this right? as in is this the best practice to code utility functions? should I be separating all the mini calculations into separate functions but then have to deal with 200 functions instead of the current 50?

Note that the self is there cause this code is part of a class. I just copy and pasted 2 functions into this paste

import bisect

def service_coverage_area(self, mode):
  #simplified for codereview
  return 1

def some_calc(self, mode, street_type, grade, elderly_proportion, f_px):
    """ where:
          r = r_0 * f_sc * f_s * f_pop * f_px

          r_0 = ideal transit stop service radius
            400m for bus and 800m for rail / express bus and commuter
            needed variable: mode

          f_sc = street connectivity factor
            {'grid':1, 'hybrid':0.85, 'cul_de_sac':0.45}
            needed variable: street connectivity tye

          f_s = grade fator
            0.00 to 0.05 -> 1.00
            0.06 to 0.08 -> 0.95
            0.09 to 0.11 -> 0.80
            0.12 to 0.15 -> 0.65
            needed variable: grade

          f_pop population factor
            1 for normal population
            0.85 of 20% is elderly population
            needed variable:elderly_proportion

          f_px pedestrian crossing factor
            provided by another calculation
    """
    #calculate r_0
    r_0 = self.service_coverage_area(mode)

    #calculate f_sc
    if street_type == 'grid':
      f_sc = 1
    elif street_type == 'hybrid':
      f_sc = 0.85
    elif street_type == 'cul_de_sac':
      f_sc = 0.45

    #calculate f_s
    grades = [0.05, 0.08, 0.11]
    f_s_values = [1.00, 0.85, 0.80, 0.65]
    f_s_score = bisect.bisect(grades, grade)
    f_s = f_s_values[f_s_score]

    #calculate f_pop
    if elderly_proportion <= 0.2:
      f_pop = 1
    else:
      f_pop = 0.85

    return r_0 * f_sc * f_s * f_pop * f_px

Am I doing this right?

Almost.

  as in is this the best practice to code utility functions?

These aren't "utility" functions.  They appear to be central to your app.  There are rarely "utility" functions in any app.

What we used to call "utility" functions are almost always part of a standard library.

  should I be separating all the mini calculations into separate functions

Yes.

  but then have to deal with 200 functions instead of the current 50?

You still have the calculations.  You're just giving them names and keeping them separate.  Giving them separate names makes them more reusable, easier to test and easier to find.

The most important thing here is to avoid creating a (nearly) useless class who's only job is to contain a bunch of functions.

Your code uses no instance variables and simply uses another function that happens to be in the class.  Both of these could be @staticmethod method functions.

There's no reason -- in Python -- to create a class unless you have instance variables and a change in state of some kind.

[In Java, you must often create "all-static" classes because there's no other place to put stateless math like this.]

You can simply put these functions into a simple module and avoid the class definitions.


Wrapper around a Python API for creating user interfaces I am writing a wrapper around the GeUserArea class of Cinema 4D's Python API to enable creating user interfaces using an object orientated interface.

I've already written 2 prototypes, but I am not satisfied with my class-design.
They do work very well, but I decided to start over to make it even more efficient, more clean and better commented, etc.

The GeUserArea class provides drawing on it as it was a field of pixels, so it's a highly dynamic class to enable almost any type of interface you want, when you have the skills.

The baseclass for all objects being rendered is the Renderable class.
Two of the subclasses are View and Lable. The difference between these is, that View does hold a frame attribute that defines a rectangular area while Lable does only hold a position attribute that defines the position of the text being rendered.

The Renderable class implements a tree-like design, so you can add a Renderable instance below another, etc. When an input event (mouse/keyboard) appears, a chain starts that tells the full tree that this event occured. Any Renderable instance should be able to react on an input event. But the view-subclass should also call a method when the mouse clicked into it's area.

(Theese methods are to let the user override them while subclassing, enabling to react on those input events.)

The event itself comes from GeUserArea.InputEvent(self, msg).
From the msg parameter, I can read out if and where a mouse event took place.
After doing so, the event chain starts.

class Renderable:
    def InputEvent(self, msg):
        pass

class View(Renderable):
    def MouseEvent(self, msg, mouse):
        pass

Using the above layout, I'd need to implement a method that manages calling of theese methods, like this:

class Renderable:
    # ..
    def _private_ManageInput(self, msg):
        self.InputEvent(msg)
        for sub in self.subs:
            sub._private_ManageInput(msg)

class View(Renderable):
    # ..
    def _private_ManageInput(self, msg):
        if IsMouseEvent(msg):
            mouse = GetMouse(msg)
            if mouse in self.frame:
                self.MouseEvent(self, msg, mouse)
        Renderable._private_ManageInput(self, msg)

This does already seem a bit dirty to me.

As the mouse position doesn't change within the recursive chain, it makes more sense to give the mouse position as an argument to the Renderable._private_ManageInput() method, which gives a speedup of about 30%.

class Renderable:
    # ..
    def _private_ManageInput(self, msg, mouse):
        self.InputEvent(msg)
        for sub in self.subs:
            sub._private_ManageInput(msg)

class View(Renderable):
    # ..
    def _private_ManageInput(self, msg, mouse):
        if mouse is not None:
            if mouse in self.frame:
                self.MouseEvent(msg, mouse)
        Renderable._private_ManageInput(self, msg, mouse)

The Renderable class does not implement reacting on MouseEvents (as it does not define any area where the mouse could "hit" it), but a method does need arguments for this purpose?  It seems quite dirty to me.

Can you tell me how I can improve this design?

One thing you could do is ensure that events only reach those who are interested in that particular type of event. Right now you seem to be sending out a generic events that you then check to see what type they are.

I wrote a similar answer some time ago to another question on Stack Overflow. You can read it here:
► https://stackoverflow.com/questions/7249388/7294148#7294148


Linked list where user replace a chosen number in the list with a number chosen by user - how can it be improved? I'm quite new to C programming so I'm trying to learn the basics. I've made a program that creates a linked list of numbers, then ask user for which number he/she wants to replace and then what number should come instead.

I finally got it to work, but since I'm learning this on my own no one is there to say when my style is bad or if I use bad methods for solving my problems. So I kindly ask if you see something that's done in an unwise way to let me know.

So here's the code:

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

struct node {
    int val;
    struct node* next;
};

typedef struct node node_t;

// Function that evaluate the value in the nodes of the linked list and replace it.
void replace(node_t ** head, int replacement_value, int new_value) {
    node_t **tracer = head;

    while ((*tracer)->next != NULL && (*tracer)->val != replacement_value) {
        (*tracer) = (*tracer)->next;
    }
    (*tracer)->val = new_value;

}

// Function that prints out the linked list.
void printlist(node_t *head) {
    node_t *temporary = head;

    while (temporary != NULL) {
        printf("%d - ", temporary->val);
        temporary = temporary->next;
    }
    printf("\n");
}

// The following creates nodes in the linked list.
node_t *create_new_node(int val) {
    node_t *result = malloc(sizeof(node_t));
    result->val = val;
    result->next = NULL;
    return result;
}

int main(){

    node_t *head = NULL;
    node_t *tmp;

    // The following creates the linked list.
    for (int i=0; i<25; i++) {
        tmp = create_new_node(i);
        tmp->next = head;
        head=tmp;
    }

    printf("First print:\n");
    printlist(head);

    int replacement_value;
    int new_value;

    printf("Which number do you wish to replace?\n");
    scanf("%d", &replacement_value);
    printf("What number do you want to replace it with?\n");
    scanf("%d", &new_value);

    node_t *temphead;
    temphead = head;
    replace(&temphead, replacement_value, new_value);

    printf("Modified print:\n");
    printlist(head);

    return(0);
}

For a new user of C you really aren't doing too bad, you're not using global variables which is very good. The functions are small and well defined. The program as a whole is well organized so that function prototypes are not required.

This program could have a few more function and that would simplify main():

A function that asks the user for input and performs error checking on the input before returning values, this would also localize the variables for input in the function
There are some basic linked list operations that are not here that might be helpful insert_node(), delete_node() and append_node() find_node(), other functions such as replace() can utilize these basic functions
It might be good to have a function that prints a title and then calls printlist() which would reduce the amount of code in main()
A function to initialize the linked list

Missing Error Checking

There are two places where error checking in this program, first, it is generally a best practice to error check all user input before using the input values. Users can make mistakes when entering data, they may just hit the enter key without entering data, they may enter a string instead of a number, they may enter a number that is not in the list which could cause problems while searching the list.

The second place where error checking is necessary is after any memory allocation in the C programming language, some or most high level languages will throw exceptions if memory allocation fails. In C programming if memory allocation fails the call to malloc(), calloc() or realloc() returns NULL. It is very important to check the value returned to make sure it is not NULL before using the memory, access through a NULL pointer causes unknown behavior. This can cause a number of problems including the crashing of the program or the corruption of data in the program.

node_t *create_new_node(int val) {
    node_t *result = malloc(sizeof(node_t));
    if (result == NULL)
    {
        fprintf(stderr, "malloc failed in create_new_node()\n");
        return result;
    }
    result->val = val;  // Unknown behavior here if malloc() failed
    result->next = NULL;
    return result;
}

Include Files That Are Not Necessary

The code contains #include <string.h> however, no functions defined in this include file are utilized. The `#include  statement copies the contents of the file into the current file, it will increase the compile time and if you are using an editor that also creates a makefile it may add an unnecessary dependency to the makefile. There may also be collisions with functions defined in the code. Basically don't include files you don't need.

Return From Main

The C compiler is smart enough to add return 0; at the end of this program, if the code does contain multiple returns from main() such as program failure when malloc() fails the code will be more readable if return EXIT_FAILURE; or return EXIT_SUCCESS;. These macros are included from stdlib.h which is already included in this program.

Readability

The code would be more readable and easier to maintain if there were spaces between operators and operands for (int i=0; i<25; i++) { versus for (int i = 0; i < 25; i++) {. It would also make the code more readable and maintainable if the 25 in the for loop was a symbolic constant

#define LIST_SIZE 25

...

    for (int i = 0; i < LIST_SIZE; i++) {
        tmp = create_new_node(i);
        if (tmp == NULL)
        {
            return EXIT_FAILURE;
        }
        tmp->next = head;
        head=tmp;
    }


IQueryable left outer join linq extension I wanted to create an extension method to enable left outer joins in linq/lambda IQueryables, and I have come up with the following, but I am still not sure it's completely correct.

public static IQueryable<TResult> LeftOuterJoin<TOuter, TInner, TKey, TResult>(
    this IQueryable<TOuter> outer,
    IQueryable<TInner> inner,
    Expression<Func<TOuter, TKey>> outerKeySelector,
    Expression<Func<TInner, TKey>> innerKeySelector,
    Func<TOuter, TInner, TResult> resultSelector)
{
    return outer.GroupJoin(
        inner,
        outerKeySelector,
        innerKeySelector,
        (o, i) => new { outer = o, innerCollection = i })
    .SelectMany(
        a => a.innerCollection.DefaultIfEmpty(),
        (a, i)=>resultSelector(a.outer,i));
}

I am especially concerned with the resultSelector being a Func, not an Expression, but I cant call an Expression in the lambda on the last line. I assume it gets cast automatically, but it's only an assumption.

Is this correct? How can I improve this? Can it be optimized more?

Actually, it is way more complex than that, since Funcs<> cant't be translated to the database, so you'll have to "call" the expression. But since you can't call a expression, you'll have to change the expression into another that means what you need it to means.

Check out this question on StackOverflow. It has an good explanation on Linq inner workings and the accepted answer has a method that do the outer join. Be sure to read the second answer too, since it shows some issues that arise on more complex queries. And just as a warning, the method you are trying to write will easily gets 150 lines long (that's why I didn't copied it here)...


Simple IRC Bot in C# This was a simple IRC bot I threw together a long time ago, found recently, and was curious as to if there were any kind of significant improvements that could be made.

using System;
using System.Net;
using System.Net.Sockets;
using System.IO;
using System.Threading;

public class IRCbot
{
    // server to connect to (edit at will)
    public static string SERVER = "irc.changeme.com";
    // server port (6667 by default)
    private static int PORT = 6667;
    // user information defined in RFC 2812 (IRC: Client Protocol) is sent to the IRC server
    private static string USER = "USER IRCbot 0 * :IRCbot";
    // the bot's nickname
    private static string NICK = "IRCbot";
    // channel to join
    private static string CHANNEL = "#opers";

    static void Main(string[] args)
    {
        NetworkStream stream;
        TcpClient irc;
        string inputLine;
        StreamReader reader;
        StreamWriter writer;

        try
        {
            irc = new TcpClient(SERVER, PORT);
            stream = irc.GetStream();
            reader = new StreamReader(stream);
            writer = new StreamWriter(stream);
            writer.WriteLine("NICK " + NICK);
            writer.Flush();
            writer.WriteLine(USER);
            writer.Flush();

            while (true)
            {
                while ((inputLine = reader.ReadLine()) != null)
                {
                    Console.WriteLine("<- " + inputLine);

                    // split the lines sent from the server by spaces (seems to be the easiest way to parse them)
                    string[] splitInput = inputLine.Split(new Char[] {
                        ' '
                    });

                    if (splitInput[0] == "PING")
                    {
                        string PongReply = splitInput[1];
                        //Console.WriteLine("->PONG " + PongReply);
                        writer.WriteLine("PONG " + PongReply);
                        writer.Flush();
                        //continue;
                    }

                    switch (splitInput[1])
                    {
                        case "001":
                            writer.WriteLine("JOIN " + CHANNEL);
                            writer.Flush();
                            break;
                        default:
                            break;
                    }
                }

                // close all streams (to preserve memory)
                writer.Close();
                reader.Close();
                irc.Close();
            }
        }
        catch (Exception e)
        {
            // shows the exception, sleeps for a little while and then tries to establish a new connection to the IRC server
            Console.WriteLine(e.ToString());
            Thread.Sleep(5000);
            string[] argv = {};
            Main(argv);
        }
    }
}

you should separate the IRCBot from the Main into its own class with it's own fields/properties
you should specify all required parameters via the construtor so that it is not possible to create an invalid ircbot
I don't think it's a good idea to make the retry recursive - IMHO it would be better to create a loop and limit the retries so that you don't have an infite loop (ok, unless it was desired)
you really should use the using statement. In your current implementation the resources will be closed only if there was no execption - if an exception occurs the Closees won't be called. You also don't free the resources because you don't call the Dispose, you just close them and then create new resources on retry, this is a memory leak.

Here's an example:

public class IRCbot
{
    // server to connect to (edit at will)
    private readonly string _server;
    // server port (6667 by default)
    private readonly int _port;
    // user information defined in RFC 2812 (IRC: Client Protocol) is sent to the IRC server
    private readonly string _user;

    // the bot's nickname
    private readonly string _nick;
    // channel to join
    private readonly string _channel;

    private readonly int _maxRetries;

    public IRCbot(string server, int port, string user, string nick, string channel, int maxRetries = 3)
    {
        _server = server;
        _port = port;
        _user = user;
        _nick = nick;
        _channel = channel;
        _maxRetries = maxRetries;
    }

    public void Start()
    {
        var retry = false;
        var retryCount = 0;
        do
        {
            try
            {
                using (var irc = new TcpClient(_server, _port))
                using (var stream = irc.GetStream())
                using (var reader = new StreamReader(stream))
                using (var writer = new StreamWriter(stream))
                {
                    writer.WriteLine("NICK " + _nick);
                    writer.Flush();
                    writer.WriteLine(_user);
                    writer.Flush();

                    while (true)
                    {
                        string inputLine;
                        while ((inputLine = reader.ReadLine()) != null)
                        {
                            Console.WriteLine("<- " + inputLine);

                            // split the lines sent from the server by spaces (seems to be the easiest way to parse them)
                            string[] splitInput = inputLine.Split(new Char[] { ' ' });

                            if (splitInput[0] == "PING")
                            {
                                string PongReply = splitInput[1];
                                //Console.WriteLine("->PONG " + PongReply);
                                writer.WriteLine("PONG " + PongReply);
                                writer.Flush();
                                //continue;
                            }

                            switch (splitInput[1])
                            {
                                case "001":
                                    writer.WriteLine("JOIN " + _channel);
                                    writer.Flush();
                                    break;
                                default:
                                    break;
                            }
                        }
                    }
                }
            }
            catch (Exception e)
            {
                // shows the exception, sleeps for a little while and then tries to establish a new connection to the IRC server
                Console.WriteLine(e.ToString());
                Thread.Sleep(5000);
                retry = ++retryCount <= _maxRetries;
            }
        } while (retry);
    }
}

Usage:

void Main()
{
    var ircBot = new IRCbot(
        server: "irc.changeme.com",
        port: 6667,
        user: "USER IRCbot 0 * :IRCbot",
        nick: "IRCbot",
        channel: "#opers"
    );

    ircBot.Start();
}


How to model hierarchical json in Java I'm a front end developer who is brand new to backend development. My task is to model json in a Java object. It's just some mock data for now that my controller returns.

{
"data": {
    "objectId": 25,
    "columnName": [
        "myCategory",
        "myCategoryId"
    ],
    "columnValues": [
        [
            "Category One",
            1
        ],
        [
            "Category Two",
            2
        ],
        [
            "Category Three",
            3
        ],
        [
            "Category Four",
            4
        ],
        [
            "Category Five",
            5
        ]
    ]
}

}

And here's my attempt. The controller returns this json correctly. But isn't this too simple? What I believe should be done is extrapolate the columnName and columnValues arrays into separate classes but I'm not sure how.

package com.category;

import java.util.List;

public class MyObjectData {
    private int objectId;
    private List columnName;
    private List columnValues;

    public int getObjectId() {
        return objectId;
    }
    public void setObjectId(int objectId) {
        this.objectId = objectId;
    }

    public List getColumnName() {
        return columnName;
    }

    public void setColumnName(List colName) {
        this.columnName = colName;
    }

    public List getColumnValues() {
        return columnValues;
    }

    public void setValues(List values) {
        this.columnValues = values;
    }

}

Regarding the columnNames and columnValues, I feel like I should be doing something like this in the model instead:

private List<ColumnNames> columnNames;
    private List<ColumnValues> columnValues;

    public List<ColumnNames> getColumnNames() {
        return columnNames;
    }

    public void setColumnNames(List<ColumnNames> columnNames) {
        this.columnNames = columnNames;
    }

    public List<ColumnValues> getColumnValues() {
        return columnValues;
    }

    public void setColumnValues(List<ColumnValues> columnValues) {
        this.columnValues = columnValues;
    }

And then I'd have two separate classes for them like this:

package com.category;

import java.util.List;

public class ColumnName {

    private String columnName;

    public String getColumnName() {
        return columnName;
    }

    public void setColumnName(String columnName) {
        this.columnName = columnName;
    }

}

package com.category;

import java.util.List;

public class ColumnValue {

    private String columnValue;
    private int columnValueId;

    public String getColumnValue() {
        return columnValue;
    }

    public void setColumnValue(String columnValue) {
        this.columnValue = columnValue;
    }

    public String getColumnValueId() {
        return columnValueId;
    }

    public void setColumnValueId(int columnValueId) {
        this.columnValueId = columnValueId;
    }

}

I feel like I have all the right pieces but just not sure if this is a better approach than my initial attempt...which works. Just looking for input. Thanks in advance.

You implied Json -> Java object, but have you considered the other way around?  If so, check out Java object to Json serialization/deserialization using Gson.  Get more at https://en.wikipedia.org/wiki/Gson and https://static.javadoc.io/com.google.code.gson/gson/2.8.5/com/google/gson/Gson.html


Miller-Rabin Recursive Primality Test I'm working on a primality test and have written a recursive function that returns the value of the function

  \$b^{q-1} \bmod q\$

  where \$3<= q <= 32000\$

Is there any way to speed up my function? It works, but takes a while to return the answer as \$q\$ approaches 32000.

Variables:

  pow = \$q-1\$

  mod = \$q\$

  b is a variable ranging from \$1 < b < q \$

If q is prime, then b will be = to q if not, b will be a "strong" feature of non-primality. See Miller–Rabin primality test.

public int fF(int q)
    {
        int b = 2, v = 0;
        while(b < q)
        {
            v = operate(b, q-1, q);
            if (v != 1)
                break;
            b++;
        }
        return b;
    }

 int operate(int b, int pow, int mod)
    {
        if (pow == 2)
           return (b * b) % mod;
        return (pow % 2 != 0) ? (b * operate(b, pow - 1, mod)) % mod : (operate(b, pow / 2, mod) * operate(b, pow / 2, mod)) % mod;
    }

Naming

Oh my, what would Mr.Maintainer think if he would inherit the code... single letter variable names, methodnames like fF. He would have a hard time to figure out what is happening.

So let us clean this a little bit

public int fF(int possiblePrime)
{
    int baseNumber = 2, v = 0;

    while (baseNumber < possiblePrime)
    {
        int exponent = possiblePrime - 1;
        v = operate(baseNumber, exponent, possiblePrime);
        if (v != 1)
            break;
        baseNumber++;
    }
    return baseNumber;
}

int operate(int baseNumber, int exponent, int divisor)
{
    if (exponent == 2)
        return (baseNumber * baseNumber) % divisor;
    return (exponent % 2 != 0) ? (baseNumber * operate(baseNumber, exponent - 1, divisor)) % divisor : (operate(baseNumber, exponent / 2, divisor) * operate(baseNumber, exponent / 2, divisor)) % divisor;
}

Style

As many will agree, using braces {}, for single if statements also, is a have to.So let us use them and also let us remove the tenary expression and add an int result which we will return

public int fF(int possiblePrime)
{
    int baseNumber = 2, v = 0;

    while (baseNumber < possiblePrime)
    {
        int exponent = possiblePrime - 1;
        v = operate(baseNumber, exponent, possiblePrime);
        if (v != 1)
        {
            break;
        }
        baseNumber++;
    }
    return baseNumber;
}

int operate(int baseNumber, int exponent, int divisor)
{
    int result = 0;
    if (exponent == 2)
    {
        result = (baseNumber * baseNumber) % divisor;
    }
    else if (exponent % 2 != 0)
    {
        result = (baseNumber * operate(baseNumber, exponent - 1, divisor)) % divisor;
    }
    else
    {
        result = (operate(baseNumber, exponent / 2, divisor) * operate(baseNumber, exponent / 2, divisor)) % divisor;
    }
    return result;
}

Refactoring

Now let us focus on operate()

What you are doing, is always calling number * number % divisor so let us extract this to a method

private int calculateProductModulo(int firstValue, int secondValue, int moduloNumber)
{
    return (firstValue * secondValue) % moduloNumber;
}

The operate() method now looks

int operate(int baseNumber, int exponent, int divisor)
{
    int result = 0;
    if (exponent == 2)
    {
        result = calculateProductModulo(baseNumber, baseNumber, divisor);
    }
    else if (exponent % 2 != 0)
    {
        result = calculateProductModulo(baseNumber, operate(baseNumber, exponent - 1, divisor), divisor);
    }
    else
    {
        result = calculateProductModulo(operate(baseNumber, exponent / 2, divisor), operate(baseNumber, exponent / 2, divisor), divisor);
    }
    return result;
}

If we now extract the recursive calls out of the call to calculateProductModulo() we will see clearly what you have stated in your answer

int operate(int baseNumber, int exponent, int divisor)
{
    int result = 0;
    if (exponent == 2)
    {
        result = calculateProductModulo(baseNumber, baseNumber, divisor);
    }
    else if (exponent % 2 != 0)
    {
        int recursiveResult = operate(baseNumber, exponent - 1, divisor);
        result = calculateProductModulo(baseNumber, recursiveResult, divisor);
    }
    else
    {
        int recursiveResult1 = operate(baseNumber, exponent / 2, divisor);
        int recursiveResult2 = operate(baseNumber, exponent / 2, divisor);
        result = calculateProductModulo(recursiveResult1, recursiveResult2, divisor);
    }
    return result;
}

The code is calling 2 times the same method with the same arguements.

Let us eleminate the double calling

int operate(int baseNumber, int exponent, int divisor)
{
    int result = 0;
    int recursiveResult = 0
    if (exponent == 2)
    {
        result = calculateProductModulo(baseNumber, baseNumber, divisor);
    }
    else if (exponent % 2 != 0)
    {
        recursiveResult = operate(baseNumber, exponent - 1, divisor);
        result = calculateProductModulo(baseNumber, recursiveResult, divisor);
    }
    else
    {
        recursiveResult = operate(baseNumber, exponent / 2, divisor);
        result = calculateProductModulo(recursiveResult , recursiveResult , divisor);
    }
    return result;
}


Handling an IRController At the time I try to write some code to handle an IRController, I got this following classes:

public class UsbDataInputListener implements UsbPipeListener {

    @Override
    public void errorEventOccurred(UsbPipeErrorEvent upee) {
        throw new UnsupportedOperationException("Not supported yet."); //To change body of generated methods, choose Tools | Templates.
    }

    @Override
    public void dataEventOccurred(UsbPipeDataEvent upde) {
        final byte[] recData = upde.getData(); //Übertragene Bytes
        //Input wird überprüft
        //Aktion für die jeweilige Taste wird durchgeführt
        if(Arrays.equals(recData, Keys.POWER)){
            Key_Action.callShutdown();
        }
        ....
        ....
}

This class waits for a DataEvent (pressing a button on the controller):

public final class Keys {

    public static final byte[] POWER = {4,1};
    //Direkt Zugriff aus Musi,Spiele,Gallerie und Videos
    public static final byte[] MUSIC = {2,5,0,4,0,0,0,0,2,5,0,30,0,0,0,0};
    public static final byte[] GAME = {2,5,0,5,0,0,0,0,2,5,0,31,0,0,0,0};
    public static final byte[] GALLERY = {2,5,0,6,0,0,0,0,2,5,0,32,0,0,0,0};
    public static final byte[] VIDEO = {2,5,0,7,0,0,0,0,2,5,0,33,0,0,0,0};
    //Steurung für Film und Musik
    public static final byte[] BEFORE = {3,-74,0};
    public static final byte[] BEFORE2 = {2,0,0,0,0,0,0,0,3,-74,0};
    public static final byte[] NEXT = {3,-75,0};
    public static final byte[] NEXT2 = {2,0,0,0,0,0,0,0,03,-75,0};
    public static final byte[] BACKWARD = {3,-53,0};
    public static final byte[] BACKWARD2 = {2,0,0,0,0,0,0,0,03,-53,0};
    public static final byte[] FORWARD = {3,-54,0};
    public static final byte[] FORWARD2 = {2,0,0,0,0,0,0,0,03,-54,0};
    public static final byte[] PLAY = {3,-51,};
    public static final byte[] PLAY2 = {2,0,0,0,0,0,0,0,03,-51,0};
    public static final byte[] STOP = {3,-73,0};
    public static final byte[] STOP2 = {2,0,0,0,0,0,0,0,03,-73,0};
    //Nummernfeld und Navigation durch das Menü
    public static final byte[] NUMBER_ONE = {2,0,0,43,0,0,0,0,2,0,0,0,0,0,0,0};
    public static final byte[] UP = {2,0,0,82,0,0,0,0,2,0,0,0,0,0,0,0};
    public static final byte[] NUMBER_THREE = {2,-128,0,0,0,0,0,0,2,0,0,0,0,0,0,0};
    public static final byte[] LEFT = {2,0,0,80,0,0,0,0,2,0,0,0,0,0,0,0};
    public static final byte[] ENTER = {2,0,0,40,0,0,0,0,2,0,0,0,0,0,0,0};
    public static final byte[] RIGHT = {2,0,0,79,0,0,0,0,2,0,0,0,0,0,0,0};
    public static final byte[] NUMBER_SEVEN= {2,1,0,18,0,0,0,0,2,0,0,0,0,0,0,0};
    public static final byte[] DOWN = {2,0,0,81,0,0,0,0,2,0,0,0,0,0,0,0};
    public static final byte[] ESCAPE = {2,0,0,41,0,0,0,0,2,0,0,0,0,0,0,0};
    public static final byte[] ZERO = {2,0,0,0,0,0,0,0,2,4,0,43,0,0,0,0};
    //Lautstärkeregelung
    public static final byte[] VOL_UP = {3,-23,0};
    public static final byte[] VOL_DOWN = {3,-22,0};
    public static final byte[] MUTE = {3,-30,0};
}

This class represents the bytcode for every key

public class Key_Action {

    private static final Runnable SHUTDOWN = new Runnable(){
        @Override
        public void run() {
            try {
                ShutdownInfoController isc = new ShutdownInfoController();
                Window shtdnWin = new Window("/app/view/ShutdownInfoView.fxml","Info zum Herunterfahren",
                              isc,false);
            } catch (IOException ex) {
                Logger.getLogger(Keys.class.getName()).log(Level.SEVERE, null, ex);
            }
        }
    };

    //Ruft das neue Fenster auf
    public static void callShutdown(){
        Platform.runLater(SHUTDOWN);
    }

    //PC wird heruntergefahren
    public static void actShutdown() throws IOException{
        String cmdtext = "shutdown.exe /s /hybrid /t 0 "; // Befehl um PC herunterzufahren
        Runtime.getRuntime().exec(cmdtext); // Befehl wird ausgeführt;
    }
}

This class performs some action when a button is pressed.

My question is: Is this code design ok? Or is it a bad style?

I would try to create a data structure which stores both event data and required action. Something like this:

Map<byte[], Runnable> commands = new HashMap<>();
commands.put(Keys.POWER, SHUTDOWN);
...

Then you can iterate it over:

for (Map.Entry<byte[], Runnable> entry: commands) {
    if (Array.equals(entry.getKey(), recData)) {
        Platform.runLater(entry.getValue());
    }
}

It seems that the logger uses a wrong classname here:

Logger.getLogger(Keys.class.getName()).log(Level.SEVERE, null, ex);

It might mislead maintainers/bug hunters.
The comments is just a noise here, you should remove it:

throw new UnsupportedOperationException("Not supported yet."); //To change body of generated methods, choose Tools | Templates.

(Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)
Usually I try to avoid abbreviations like upde, shtdnWin, actShutdown. They are not too readable and I suppose you have autocomplete (if not, use an IDE, it helps a lot), so using longer names does not mean more typing but it would help readers and maintainers a lot since they don't have to remember the purpose of each variable - the name would express the programmers intent and would not force readers to decode the abbreviations every time they read/maintain the code.

See also: Clean Code by Robert C. Martin, Use Intention-Revealing Names, p18; Avoid Mental Mapping, p25

  If you can’t pronounce it, you can’t discuss it without sounding like an idiot. “Well,
  over here on the bee cee arr three cee enn tee we have a pee ess zee kyew int, see?” This
  matters because programming is a social activity.

Source: Clean Code by Robert C. Martin, Use Pronounceable Names, p21
Key_Action does not follow the usual Java naming conventions. Classnames should be CamelCase, without underscore.


Listing employees and their project tasks My database (SQL Server 2008) looks like this:

  People 1 -- * TaskPersons * -- 1 Tasks * -- 1 Project

The GROUP BY part of the query is quite long, all because of the COUNT aggregate.  This seems to perform reasonably well.  I would like to know if there is a more concise way of writing this.

use Projects
SELECT COUNT(t.id), ppl.Name, ppl.Birthdate, ppl.Title, ppl.Role, ppl.Status, ppl.Warehouse, ppl.StartDate, ppl.SalaryBand, ppl.Salary,p.Title FROM People ppl
JOIN TaskPersons tp On tp.PersonId = ppl.Id
JOIN Tasks t ON t.Id = tp.TaskId
JOIN Projects p ON p.Id = t.ProjectId
GROUP BY ppl.Name, ppl.Name, ppl.Birthdate, ppl.Title, ppl.Role, ppl.Status, ppl.Warehouse, ppl.StartDate, ppl.SalaryBand, ppl.Salary, p.Title

Try putting GROUP BY in a sub-select.

SELECT  personTaskProject.TaskCount,
        ppl.Name,
        ppl.Birthdate,
        ppl.Title,
        ppl.Role,
        ppl.Status,
        ppl.Warehouse,
        ppl.StartDate,
        ppl.SalaryBand,
        ppl.Salary,
        personTaskProject.Title
    FROM
        People ppl,
        (SELECT COUNT(t.id) TaskCount, tp.PersonId, p.Title
            FROM TaskPersons tp
                JOIN Tasks t
                    ON t.Id = tp.TaskId
                JOIN Projects p
                    ON p.Id = t.ProjectId
            GROUP BY tp.PersonId, p.Title) AS personTaskProject
    WHERE ppl.Id = personTaskProject.PersonId;

SQL Fiddle


Simple React component to view cached data and test result from API This is only the front end side of things which I have at the moment, as the back end is still being worked on. But as this is my first attempt at a react component, I'd like to know if there is anything I can improve on it.

The component has a text area for viewing the current data stored in the cache within our application, another text area for viewing the result when we send that data off to our API and get a transformed version of the data back, as well as two buttons: one for loading the cached data into one of the text areas and one for calling the API with the cached data. This component is used in development/UAT environment for testing purposes.

class DataRevealer extends React.Component {
    constructor(props) {
        super(props);
        this.state = {input: '', result: ''};

        this.getCachedDataHandler = this.getCachedDataHandler.bind(this);
        this.transformDataHandler = this.transformDataHandler.bind(this);
        this.inputDataChanged = this.inputDataChanged.bind(this);
    }

    getCachedDataHandler() {
      let cachedData = DataRevealActions.getCachedData();
      this.setState({input: cachedData});
    }

    transformDataHandler() {
      if (this.state.input) {
        let transformedData = DataRevealActions.transformData(this.state.input);
        this.setState({result: transformedData});
      } else {
        alert('No data to transform');
      }
    }

    inputDataChanged(event) {
      this.setState({input: event.target.value});
    }

    get getCachedDataButtonText() {
      //TODO: localise string
      return 'Get Cached Data';
    }

    get transformDataButtonText() {
        //TODO: localise string
        return 'Transform Data';
    }

    render() {
        return (

                <textarea value={ this.state.input }
                          className='data-revealer-textarea centred-control'
                          onChange={ this.inputDataChanged }
                />

                  <Button onClick={ this.getCachedDataHandler }
                          className='data-revealer-button centred-control'>
                      { this.getCachedDataButtonText }
                  </Button>
                  <Button onClick={ this.transformDataHandler }
                          className='data-revealer-button centred-control' >
                      { this.transformDataButtonText }
                  </Button>

              <textarea value={ this.state.result }
                        className='data-revealer-textarea centred-control'
              />

        );
    }
}

export default DataRevealer;

Couple of things to note:

The Button is one of our custom controls, it acts as a normal button but with pre-implemented styling tweaks.
The TODO may not actually be needed, as the strings may not need to be localised.
DataRevealActions are where the back-end code will go, once that work is done.

You can just remove the TODO until it's needed. The code will be easier to follow and if someone needs to localize later, they can figure it out themselves. It's clutter.
DataRevealActions would probably be nice to pass in as properties, so the component is configurable. You could pass it in as now, or just have two different properties, getCachedData and transformData.

Otherwise, looks great!


Parsing Excel file into CSV file EDIT: Thanks everyone for your answers, this is moving quickly now and it's always essential to get better with design and best practice!

I am loading in an excel file to a dataset using ExcelDataReader. I then need to create a *.csv file with the data.

Currently for the excel sheet I'm working with I have about 40 columns and 30k rows of data. The approach I'm taking takes about 2 minutes to complete.

The second snippet is the offender.

public static Stream SaveAsCsv(Stream excelFile, string filename)
{
    MemoryStream newCSV = new MemoryStream();

    string test = excelFile.ToString();

    IExcelDataReader reader = null;
    if (filename.EndsWith(".xls"))
    {
        reader = ExcelReaderFactory.CreateBinaryReader(excelFile);
    }
    else if (filename.EndsWith(".xlsx"))
    {
        reader = ExcelReaderFactory.CreateOpenXmlReader(excelFile);
    }

    //Read was empty so return a null stream in return
    if (reader == null)
        return newCSV;

    var headers = new List<string>();

    var ds = reader.AsDataSet(new ExcelDataSetConfiguration()
    {
        ConfigureDataTable = (tableReader) => new ExcelDataTableConfiguration()
        {
            UseHeaderRow = true,

            ReadHeaderRow = rowReader =>
            {
                for (var i = 0; i < rowReader.FieldCount; i++)
                    headers.Add(Convert.ToString(rowReader.GetValue(i)));
            },

            FilterColumn = (columnReader, columnIndex) =>
            !headers[columnIndex].ToString().ToUpper().Contains("SKIP")
        }
    });
    var csvContent = string.Empty;
    int colCount = ds.Tables[0].Columns.Count;

This for operation is what takes two minutes to complete, everything else is not a problem:

    for (int row_no = 0; row_no < ds.Tables[0].Rows.Count; row_no++)
    {
        var arr = new List<string>();
        if (row_no == 0)
        {
            for (int i = 0; i < colCount; i++)
            {
                arr.Add(ds.Tables[0].Columns[i].ColumnName);
            }
        }
        else
        {
            object[] objarr = ds.Tables[0].Rows[row_no].ItemArray;

            arr = ((IEnumerable)objarr).Cast<object>()
                         .Select(x => x.ToString())
                         .ToList();
        }
        csvContent += string.Join("|", arr) + "\n";
    }

    StreamWriter csv = new StreamWriter(newCSV);
    csv.Write(csvContent);
    csv.Flush();
    csv.BaseStream.Seek(0, SeekOrigin.Begin);

    return csv.BaseStream;
}

You should definitely follow @Henrik Hansen's suggestion about separating your method into multiple APIs with one exception.

The save method should just save the stream. There is no need for it return any streams. You should already know this stream from the CreateCsv method.

Other things that you can improve are...

String concatenation

csvContent += string.Join("|", arr) + "\n";

I'm pretty sure this is the bottleneck of you application. Strings are immutable so in order to concatenate strings, the runtime has to copy the old one and appaned something new to it. Usually this is not a big issue but since you have about 40 columns and 30k rows of data this is really a lot of copying. And the string is growing, so each copy operation has to handle larger and larger strings.

For frequent string manipulation, especially in loops, you should use the StringBuilder that does not requrie copying. There are numerous pages about this topic so if you are interested in its internals, just google for it.

As far as strings are concerned there is one more piece of code that isn't efficient yet.

arr = ((IEnumerable)objarr).Cast<object>()
             .Select(x => x.ToString())
             .ToList();

You're materializing the objarr by calling ToList. This is not necessary at this point.

string.Join("|", arr)

Join can take of enumerating it later only once. Currently your string is being created twice.

The new code could look this:

var csvBuilder = new StringBuilder();

for (int row_no = 0; row_no < ds.Tables[0].Rows.Count; row_no++)
{
    if (row_no == 0)
    {
        var headers = new List<string>();
        for (int i = 0; i < colCount; i++)
        {
            headers.Add(ds.Tables[0].Columns[i].ColumnName);
        }
        csvBuilder.AppendLine(string.Join("|", headers));
    }
    else
    {
        var items = ds.Tables[0].Rows[row_no].ItemArray;
        var values = ((IEnumerable)items).Cast<object>().Select(x => x.ToString());
        csvBuilder.AppendLine(string.Join("|", values));
    }
}

Always dispose streams

Another bad habit of yours is to not dispose streams. If you call this method a couple of time you'll be wasting a lot of memory. You should always do it at some point either with the using statement or by calling Dispose() in a try/finally block.

Naming

You should pay more attention to your variable names. arr is a terrible name because it can stand for just anything. It harms code readability. Code should document itself. This one doesn't.

You should also be more consistent. If you name most of your variables correctly with camelCase then don't name others with _snake_case_ like row_no.

Use always strong names and as precise as you can. Not too long (if possible) but also not to short, avoid abbreviations. columnIndex is a good example. row_no should be rowIndex. Currently it looks like more then one person was writing this code. Each one with different coding style.

{}

for (var i = 0; i < rowReader.FieldCount; i++)
               headers.Add(Convert.ToString(rowReader.GetValue(i)));

Always use {}. You can save youself hours of debugging because without them it's very easy to make a mistake.

Explicit types vs var

You mix explit types and var. You use them interchangeably. This makes your code look very unprovessional and messy. Pick one and stick to it. (I suggest picking var).


Extracting a list of objects from a repository as JSON Currently, I am using the below code to make JSON data from list of objects.

public IActionResult getWardsFromVDC(long vdc_id)
{
    try
    {
        List<Ward> wards = wardRepo.getByVdcId(vdc_id).ToList();
        List<Dictionary<string, string>> values = new List<Dictionary<string, string>>();

        foreach (var ward in wards)
        {
            Dictionary<string, string> value = new Dictionary<string, string>();
            value["ward_id"] = ward.ward_id.ToString();
            value["ward_name"] = ward.ward_name;
            values.Add(value);
        }
        Dictionary<string, object> data = new Dictionary<string, object>();
        data["data"] = values;

        var json = JsonConvert.SerializeObject(data);
        return Json(json);
    }
    catch (Exception ex)
    {
        return Json(ex.Message);
    }
}

List<Ward> wards = wardRepo.getByVdcId(vdc_id).ToList();

is getting list of Ward class from database. Then a loop is used to put all the values in List of Dictionaries .

 Dictionary<string, object> data = new Dictionary<string, object>();
 data["data"] = values;

 var json = JsonConvert.SerializeObject(data);

is putting all those values in "data" key in another dictionary and then value is serialized and returned as JSON

The JSON data formed is :

{
  "data": [
    {
      "ward_id": "132",
      "ward_name": "1"
    },
    {
      "ward_id": "133",
      "ward_name": "2"
    },
    {
      "ward_id": "134",
      "ward_name": "3"
    },
    {
      "ward_id": "135",
      "ward_name": "4"
    },
    {
      "ward_id": "136",
      "ward_name": "5"
    },
    {
      "ward_id": "137",
      "ward_name": "6"
    },
    {
      "ward_id": "138",
      "ward_name": "7"
    },
    {
      "ward_id": "139",
      "ward_name": "8"
    },
    {
      "ward_id": "140",
      "ward_name": "9"
    },
    {
      "ward_id": "141",
      "ward_name": "10"
    },
    {
      "ward_id": "142",
      "ward_name": "11"
    },
    {
      "ward_id": "143",
      "ward_name": "12"
    }
  ]
}

To build even a simple data , I am creating dictionary and list of dictionaries and again adding data key .

Is there any simple way to build JSON in same format but in simplest way?

Anonymous types are going to be your friend here.  You don't need to have list or dictionary at all.

First you don't need to call ToList() from your repository.

Now we can just project out the properties you want like so

wards.Select(w => new
{
     ward_id = w.ward_id.ToString(),
     w.ward_name
})

JsonConvert will serialize an IEnumerable as an array.  this will get your square bracket around the "Wards" and have the two properties you would like and having the Id converted to a string.

Now you just need the "data" object to contain the IEnumerable.  Again making an anonymous type.

var data = new
{
    data = wards.Select(w => new
    {
        ward_id = w.ward_id.ToString(),
        w.ward_name
    })
};

var json = JsonConvert.SerializeObject(data);

That's it.  That should get you what you want.